Re: [PATCH 3/4] branch: correctly reject refs/heads/{-dash,HEAD}
On Thursday 16 November 2017 08:27 PM, Junio C Hamano wrote:
Kaartic Sivaraam 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 ;-)
Re: [PATCH 3/4] branch: correctly reject refs/heads/{-dash,HEAD}
Kaartic Sivaraam 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").
Re: [PATCH 3/4] branch: correctly reject refs/heads/{-dash,HEAD}
On Thursday 16 November 2017 03:44 AM, Junio C Hamano wrote:
Kaartic Sivaraam writes:
>> Are these two patches follow-up fixes (replacement of 3/3 plus an
>> extra patch) to jc/branch-name-sanity topic?
>
> Yes, that's right.
>
>> Thanks for working on these.
>
> You're welcome. Please do be sure I haven't broken anything in
> v2. These patches should cleanly apply on 'next', if they don't let me
> know.
OK, so here is a replacement for your replacement, based on an
additional analysis I did while I was reviewing your changes.
The final 4/4 is what you sent as [v2 2/2] (which was meant to
be [v2 4/3]). I think with these updates, the resulting 4-patch
series is good for 'next'.
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. As to which change,
$ git whatchanged jch/jc/branch-name-sanity..origin/next sha1_name.c
lists at least 5 of them, so there's possibly a lot of change that
hasn't been taken into account by this patch. Particularly, the function
'strbuf_check_branch_ref' itself is found at line 1435 in the version
found in 'next' though this patch expects it to be near line 1332, I guess.
Further comment inline.
sha1_name.c | 14 --
t/t1430-bad-ref-name.sh | 43 +++
2 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index c7c5ab376c..67961d6e47 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1332,9 +1332,19 @@ void strbuf_branchname(struct strbuf *sb, const char
*name, unsigned allowed)
int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
{
strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
- if (name[0] == '-')
- return -1;
+
+ /*
+* This splice must be done even if we end up rejecting the
+* name; builtin/branch.c::copy_or_rename_branch() still wants
+* to see what the name expanded to so that "branch -m" can be
+* used as a tool to correct earlier mistakes.
+*/
strbuf_splice(sb, 0, 0, "refs/heads/", 11);
+
+ if (*name == '-' ||
+ !strcmp(sb->buf, "refs/heads/HEAD"))
I guess this check should be made more consistent. Possibly either of,
if (starts_with(sb->buf, "refs/heads/-") ||
!strcmp(sb->buf, "refs/heads/HEAD"))
or,
if (*name == '-' ||
!strcmp(name, "HEAD"))
might make them consistent (at least from my perspective).
I tried to reproduce this patch manually and other than the above this
one LGTM. Though I can't be very sure as I couldn't apply it (I did it
"manually" to some extent, you see ;-)
+ return -1;
+
return check_refname_format(sb->buf, 0);
}
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index e88349c8a0..c7878a60ed 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -331,4 +331,47 @@ test_expect_success 'update-ref --stdin -z fails delete
with bad ref name' '
grep "fatal: invalid ref format: ~a" err
'
+test_expect_success 'branch rejects HEAD as a branch name' '
+ test_must_fail git branch HEAD HEAD^ &&
+ test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'checkout -b rejects HEAD as a branch name' '
+ test_must_fail git checkout -B HEAD HEAD^ &&
+ test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'update-ref can operate on refs/heads/HEAD' '
+ git update-ref refs/heads/HEAD HEAD^ &&
+ git show-ref refs/heads/HEAD &&
+ git update-ref -d refs/heads/HEAD &&
+ test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'branch -d can remove refs/heads/HEAD' '
+ git update-ref refs/heads/HEAD HEAD^ &&
+ git branch -d HEAD &&
+ test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'branch -m can rename refs/heads/HEAD' '
+ git update-ref refs/heads/HEAD HEAD^ &&
+ git branch -m HEAD tail &&
+ test_must_fail git show-ref refs/heads/HEAD &&
+ git show-ref refs/heads/tail
+'
+
+test_expect_success 'branch -d can remove refs/heads/-dash' '
+ git update-ref refs/heads/-dash HEAD^ &&
+ git branch -d -- -dash &&
+ test_must_fail git show-ref refs/heads/-dash
+'
+
+test_expect_success 'branch -m can rename refs/heads/-dash' '
+ git update-ref refs/heads/-dash HEAD^ &&
+ git branch -m -- -dash dash &&
+ test_must_fail git show-ref refs/heads/-dash &&
+ git show-ref refs/heads/dash
+'
+
test_done
