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

From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Date: Nov 22, 2017, 17:45 UTC
Message-ID: <b5083b36-c91b-f3c7-2a22-237a1e08f993@gmail.com>
In-Reply-To: <xmqqeforxdx6.fsf@gitster.mtv.corp.google.com>

On Wednesday 22 November 2017 07:39 AM, Junio C Hamano wrote:

Show 29 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>> 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.
>

You seem to be right when viewing from the perspective of the callee (strbuf_check_branch_ref). IOW, you *seem* to be telling that the "callee" should have known that the caller was expecting the 'prefix' even in case of an error and should have "inserted" it regardless of the error (I thought the strbuf was initialized and contained the result of strbuf_branchname even in the case of an error) ?

I thought that the 'caller' should have known that the 'callee' would not insert the prefix when there was an error in the branch name thus it should have anticipated that there would be a case in which the prefix (refs/heads/) doesn't exist in the buf (the assumption).

> 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.

Let me see if I could correlate your statement with that error. AFAIK, I don't think the buffer was uninitialized in the case of that error. It had in it the result of interpretation of 'strbuf_branchname', didn't it? So that clears one case and leads me to the interpretation that,

"Passing the data (at an offset of 11 bytes from the beginning of the buf) that does not begin with "refs/heads/" is a bug"

Which seems to be in line with my statement that it was wrong (in the part of the "caller") to assume that "strbuf_check_branch_ref" always returns a buf with the prefix "refs/heads/" ?

Show 6 quoted lines

> 
> 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.
>

Makes sense. I should have better done this initially instead of giving a muddy justification for not doing it.

Thanks, Kaartic