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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 22, 2020, 14:10 UTC
Message-ID: <nycvar.QRO.7.76.6.2001221508180.46@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqpnfc8o39.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Tue, 21 Jan 2020, Junio C Hamano wrote:

Show 21 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > On Mon, Jan 20, 2020 at 3:04 PM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> >> On Mon, 20 Jan 2020, Eric Sunshine wrote:
> >> > On Fri, Jan 17, 2020 at 6:38 PM Johannes Schindelin via GitGitGadget
> >> > <gitgitgadget@gmail.com> wrote:
> >> > > +       test $colliding_sha1 = "$(git rev-parse HEAD | cut -c 1-7)" &&
> >> >
> >> > How much do we care that this is introducing new code with git
> >> > upstream of a pipe (considering recent efforts to eradicate such
> >> > usage)? Same question regarding several other new instances introduce
> >> > by this patch.
> >>
> >> I would argue that the test case will fail if the `git` call fails. So I
> >> am not overly concerned if that `git` call is upstream of a pipe.
> >
> > Unless the git command crashes _after_ it produces the correct output...
>
> 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`.

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 ;-)

Ciao, Dscho