Re: [PATCH 3/4] branch: correctly reject refs/heads/{-dash,HEAD}

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 16, 2017, 14:57 UTC
Message-ID: <xmqqlgj6uv8k.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <471c7b6e-d3a0-4aee-8f80-282ff0b8d972@gmail.com>

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

> I guess this series is not yet ready for 'next'. When I tried to apply
> this patch it doesn't seem to be applying cleanly. I get some
> conflicts in 'sha1_name.c' possibly as a consequence of the changes to
> the file that aren't accounted by the patch.

Oh, it is totally expected that this patch (and others) may not apply on 'next' without conflict resolution, as this topic, as all the other topics, are designed to apply cleanly to either 'master' or 'maint' or one of the older 'maint-*' series, if it is a bugfix topic. A patch series that only applies cleanly to 'next' would be useless---it would mean all the topics that are already in 'next' that interacts with it must graduate first before it can go in. Make it a habit to build on 'master' or older and then merge to higher integration branches to make sure it fits with others.

What you could do is to inspect origin/pu branch after you fetch, and look at the commit that merges this topic to learn how the conflicts are resolved (the contrib/rerere-train.sh script may help this process).

Your inability to resolve merge conflicts does not have much to do with the readiness of a topic, as long as somebody else can resolve them ;-)

>> +	if (*name == '-' ||
>> +	    !strcmp(sb->buf, "refs/heads/HEAD"))
>
> I guess this check should be made more consistent. Possibly either of,

Among these two, this one

> 	if (starts_with(sb->buf, "refs/heads/-") ||
> 	    !strcmp(sb->buf, "refs/heads/HEAD"))

has more chance to be correct. Also, if we were to check the result of expansion in sb->buf[], I would think that we should keep a separate variable that points at &sb->buf[11] and compare the remainder against fixed strings, as we already know sb->buf[] starts with "refs/heads/" due to our splicing in the fixed string.

Because the point of using strbuf_branchname() is to allow us expand funny notations like @{-1} to refs/heads/foo, and the result of expansion is what eventually matters, checking name[] is wrong, I would think, even though I haven't thought things through.

In any case, I would say thinking this part through should be left as a theme for a follow-on patch, and not within the scope of this one. After all, checking *name against '-' was part of the original code, so it is safer to keep the thing we are not touching bug-to-bug compatible and fixing things one step at a time (the one fix we made with this patch is to make sure we store refs/heads/-dash in sb when we reject name=="-dash").