Invalid refs should throw

[inverted-capital]

In this code, provision is made for new branches with no commits by assigning the special null tree hash:

isomorphic-git/src/models/GitWalkerRepo.js

Lines 17 to 25 in fb8a29a

	let oid
	try {
	oid = await GitRefManager.resolve({ fs, gitdir, ref })
	} catch (e) {
	if (e instanceof NotFoundError) {
	// Handle fresh branches with no commits
	oid = '4b825dc642cb6eb9a060e54bf8d69288fbee4904'
	}
	}

This does not cover the case where a completely invalid ref has been passed in, and worse, this fails silently.

I am unsure how to correct this, but I think an error should be thrown, or at least a warning emitted ?

[jcubic]

I'm not sure what to say. Do you have a real case where this is a problem?

[inverted-capital]

I don't have a repro yet, but I will make one soon. The exact issue I had was that I was sending in a ref as a string with a newline at the end. The check for valid sha simply checks if the length is 40 characters, which failed, and then the special oid representing an empty tree 4b825... was used instead, which caused a silent fail in my case.

Where is the reference material that you guys use to define behaviours for what isomorphic-git should do in response to given parameters ? Basically, where should I go to discern what the correct behaviour in this instance is ?

[jcubic]

I see. This need to be refactored then. I think it's kind of stupid that the library return some random OID of it can't find a ref. The comment say that it's for fresh branch, but it can happen when user put garbage there. Maybe ref manager should throw different error just for this case from the comment and throw any rethrow any other error.

Do you want to contribute and maybe fix the issue? This project don't have main author that left the project and almost all changes are done by individual contributors. So if you will not fix the issue, there is a big chance that this issue will never get fixed.

[jcubic]

for your usecase this can be fixed by updating this regex:

isomorphic-git/src/managers/GitRefManager.js

Line 48 in fb8a29a

if (!value.match(/[0-9a-f]{40}/)) {

It should be /^[0-9a-f]{40}$/

[inverted-capital]

Good spotting. But what is the correct behavior for when the ref is for a new branch with no commits ? Where do we get the canonical behavior from ? the git.exe file that is published by https://git-scm.com/ ?

Now that I go looking, there doesn't appear to be an actual spec for git 🤷

[jcubic]

I wonder what will happen when await resolveTree get this fake oid. Maybe this is not needed at all. This oid is also used in different places.

This requires some investigation, what happen if you run the code in empty repo and on invalid ref.

I think that the best way to get canonical behavior, is to just use the canonical git and test. I personally use Linux and git came from the official repository.

[inverted-capital]

Ok thanks. For reference, the oid 4b825dc642cb6eb9a060e54bf8d69288fbee4904 is special, in that it is the oid of an empty tree, just in case that wasn't clear.

[jcubic]

But is it part of the git spec?

[inverted-capital]

yes, 100% part of the reference git implementation as this is the result of doing a sha1 hash on an empty tree object.

[jcubic]

Ok, so if you want to contribute and change the behavior or throwing an error when invalid ref is used and detect empty tree, you're more than welcome.

[inverted-capital]

Thank you - I will get to it when I can.