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
