Re: [PATCH v2 4/4] builtin/branch: strip refs/heads/ using skip_prefix

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 22, 2017, 02:09 UTC
Message-ID: <xmqqeforxdx6.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAPig+cRK8+pzf3EWuoG+36CRNaaGv6CQk27d4CrzTv=8beoQGg@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 19 quoted lines

> On Tue, Nov 21, 2017 at 2:12 PM, Kaartic Sivaraam
> <kaartic.sivaraam@gmail.com> wrote:
>> On Wednesday 22 November 2017 12:08 AM, Eric Sunshine wrote:
>>> The original code unconditionally uses "+ 11", which says that the
>>> prefix is _always_ present. This commit message muddies the waters [...]
>>
>> That muddiness of that statement is a consequence of my recent encounter[1]
>> in which the assumption (that the prefix(refs/heads/ always exists) of that
>> code failed. I had a little suspicion, when I wrote that commit message,
>> that there might be other cases in which assumption might fail. The issue
>> has been resolved only in 3/4 of jc/branch-name-sanity but that was only
>> after I wrote the commit message initially.  So, it does make sense to
>> remove that muddiness now. Thanks for noting that.
>>
>> [1]: Note the 'warning: ' message in the following mail. That ugly '|�?' was
>> a consequence of the assumption that the 'prefix' always existed!
>> https://public-inbox.org/git/1509209933.2256.4.camel@gmail.com/
>
> Thanks for the explanation and history reference.

I have a suspicion that it wasn't the case. The ugly uninitialized byte sequence came from an error codepath of a function that is asked to fill a strbuf with "refs/heads/$something" returning early when it found an error in the input, without realizing that the caller still looks at the strbuf even when it got an error. That caller-callee pair was updated.

It is just a bug and +11 is always correct; passing the data that does not begin with "refs/heads/" there, including the case where an uninitialized buffer is passed, is simply a bug.

In other words, skip_prefix() is a good change, but if we are to use it, we should also check the result and error out if we do not see "refs/heads/" there--you already bothered to spend extra cycles for that.