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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wednesday 22 November 2017 07:39 AM, Junio C Hamano wrote:
Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:

On Tue, Nov 21, 2017 at 2:12 PM, Kaartic Sivaraam
<kaartic.sivaraam@xxxxxxxxx> 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@xxxxxxxxx/

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/" ?



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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux