Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes: >> 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.
