On Thursday 16 November 2017 08:27 PM, Junio C Hamano wrote:
Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx> 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.
Thanks for the explanation. Seems I still have to become accustomed to
the workflow.
Make it a habit to build on 'master' or older and then merge to
higher integration branches to make sure it fits with others.
Though I don't clearly understand how to do that, I'll let experience
teach me the same (if possible). :-)
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).
Sure thing.
+ 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").
Good point. This is better for a follow-up patch as there's a
possibility that we might be introducing new bugs which weren't present
previously as a consequence of changing that conditional (bug-to-bug
compatibility, good term that (possibly) summarizes my long-winded
explanation ;-)
