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