Re: [PATCH v2 2/3] rebase -i: re-fix short SHA-1 collision

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 22, 2020, 18:15 UTC
Message-ID: <xmqqr1zr73yh.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.2001221508180.46@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 7 quoted lines

>> True.  Doesn't rev-parse have an appropriate option for this kind of
>> thing that gets rid of the need for "cut" in the first place?
>
> You mean `git rev-parse --short=4`? That does something _sligthly_
> different: it tries to shorten the OID to 4 characters _unless that would
> be ambiguous_. In our case, it _will_ be ambiguous. That's why I use
> `cut`.

Ah, yes of course; we want ambiguous prefix. I think a more thorough test would be to see that the output with --short=$n (where n is the length of the abbreviated object name in $colliding_sha1) is longer than $colliding_sha1 and the output prefix-matches $colliding_sha1 iow, something like

	abbreviated=$(git rev-parse --short=7 HEAD) &&
	case "$abbreviated" in
	"$colliding_sha1"?*) : happy ;;
	*) false ;;
	esac &&
	...

which would make sure that we are testing colliding case.

> As to the crash in `rev-parse` _after_ printing out the OID: yes, there is
> a possibility for that. But that regression test is not about `rev-parse`,
> so it is actually a good thing that it would not trigger on such a bug ;-)

No, I do not think this test should be about rev-parse working correctly---just that if it is easy enough to make the test robust enough against such a breakage, it would be nice to do so, that's all.

I'm not Eric but I suspect his primary point was not about worrying about rev-parse crashing but more about avoiding to add a pattern less experienced developers can copy&paste without thinking enough to realize why it would be OK here and not OK in the context of the tests they are adding. That would be what I would worry about more than rev-parse crashing in the part of the test under discussion.

Thanks.