a small branch API clean-up
cover a small branch API clean-up
This started as a little clean-up for a NEEDSWORK comment in branch.h, but it ended up adding a rather useful safety feature.
Junio C Hamano (3):
branch: streamline "attr_only" handling in validate_new_branchname()
branch: split validate_new_branchname() into two
branch: forbid refs/heads/HEAD
branch.c | 44 ++++++++++++++++++++++++++++++-------------- branch.h | 27 ++++++++++++--------------- builtin/branch.c | 8 ++++---- builtin/checkout.c | 10 +++++----- sha1_name.c | 2 +- t/t1430-bad-ref-name.sh | 8 ++++++++ 6 files changed, 60 insertions(+), 39 deletions(-)
Signature · 1 line
2.15.0-rc1-158-gbd035ae683
PATCH 1/3 branch: streamline "attr_only" handling in validate_new_branchname()
The function takes a parameter "attr_only" (which is a name that is hard to reason about, which will be corrected soon) and skips some checks when it is set. Reorganize the conditionals to make it move obvious that some checks are never made when this parameter is set.
--- branch.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
branch.c +12 -8
@@ -181,21 +181,25 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)int validate_new_branchname (const char *name,struct strbuf *ref,int force,int attr_only) { +const char *head; +if (strbuf_check_branch_ref (ref, name))die (_ ("'%s' is not a valid branch name." ), name);if (!ref_exists (ref->buf))return 0 ; -else if (!force && !attr_only) -die (_ ("A branch named '%s' already exists." ), ref->buf +strlen ("refs/heads/" )); -if (!attr_only) { -const char *head; +if (attr_only) +return 1 ; + +if (!force) +die (_ ("A branch named '%s' already exists." ), + ref->buf +strlen ("refs/heads/" )); + + head =resolve_ref_unsafe ("HEAD" ,0 ,NULL ,NULL ); +if (!is_bare_repository () && head && !strcmp (head, ref->buf)) +die (_ ("Cannot force update the current branch." )); - head =resolve_ref_unsafe ("HEAD" ,0 ,NULL ,NULL ); -if (!is_bare_repository () && head && !strcmp (head, ref->buf)) -die (_ ("Cannot force update the current branch." )); - }return 1 ; }
Signature · 1 line
2.15.0-rc1-158-gbd035ae683
Re: [PATCH 1/3] branch: streamline "attr_only" handling in validate_new_branchname()
Quoted 3 lines · Junio C Hamano
The function takes a parameter "
attr_only" (which is a name that is hard to reason about, which will be corrected soon) and skips some checks when it is set. Reorganize the conditionals to make it move
s/move/more/
Quoted 3 lines
obvious that some checks are never made when this parameter is set.
Signed-off-by Junio C Hamano <[email protected]>
PATCH 2/3 branch: split validate_new_branchname() into two
In reply to Junio C HamanoChecking if a proposed name is appropriate for a branch is strictly a subset of checking if we want to allow creating or updating a branch with such a name. The mysterious sounding 'attr_only' parameter to validate_new_branchname() is used to switch the function between these two roles.
Instead, split the function into two, and adjust the callers. A new helper validate_branchname() only checks the name and reports if the branch already exists.
This loses one NEEDSWORK from the branch API.
--- branch.c | 34 +++++++++++++++++++++++----------- branch.h | 27 ++++++++++++--------------- builtin/branch.c | 8 ++++---- builtin/checkout.c | 10 +++++----- 4 files changed, 44 insertions(+), 35 deletions(-)
branch.c +23 -11
@@ -178,19 +178,31 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)return 0 ; } -int validate_new_branchname (const char *name,struct strbuf *ref, -int force,int attr_only) +/* +* Check if 'name' can be a valid name for a branch; die otherwise. +* Return 1 if the named branch already exists; return 0 otherwise. +* Fill ref with the full refname for the branch. +*/ +int validate_branchname (const char *name,struct strbuf *ref) { -const char *head; -if (strbuf_check_branch_ref (ref, name))die (_ ("'%s' is not a valid branch name." ), name); -if (!ref_exists (ref->buf)) -return 0 ; +return ref_exists (ref->buf); +} -if (attr_only) -return 1 ; +/* +* Check if a branch 'name' can be created as a new branch; die otherwise. +* 'force' can be used when it is OK for the named branch already exists. +* Return 1 if the named branch already exists; return 0 otherwise. +* Fill ref with the full refname for the branch. +*/ +int validate_new_branchname (const char *name,struct strbuf *ref,int force) +{ +const char *head; + +if (!validate_branchname (name, ref)) +return 0 ;if (!force) die(_("A branch named '%s' already exists."),
@@ -246,9 +258,9 @@ void create_branch(const char *name, const char *start_name,if (track ==BRANCH_TRACK_EXPLICIT || track ==BRANCH_TRACK_OVERRIDE ) explicit_tracking =1 ; -if (validate_new_branchname (name, &ref, force, - track ==BRANCH_TRACK_OVERRIDE || - clobber_head)) { +if ((track ==BRANCH_TRACK_OVERRIDE || clobber_head) + ?validate_branchname (name, &ref) + :validate_new_branchname (name, &ref, force)) {if (!force) dont_change_ref =1 ;else
branch.h +12 -15
@@ -23,22 +23,19 @@ void create_branch(const char *name, const char *start_name, int clobber_head, int quiet, enum branch_track track);/* -* Validates that the requested branch may be created, returning the -* interpreted ref in ref, force indicates whether (non-head) branches -* may be overwritten. A non-zero return value indicates that the force -* parameter was non-zero and the branch already exists. -* -* Contrary to all of the above, when attr_only is 1, the caller is -* not interested in verifying if it is Ok to update the named -* branch to point at a potentially different commit. It is merely -* asking if it is OK to change some attribute for the named branch -* (e.g. tracking upstream). -* -* NEEDSWORK: This needs to be split into two separate functions in the -* longer run for sanity. -* +* Check if 'name' can be a valid name for a branch; die otherwise. +* Return 1 if the named branch already exists; return 0 otherwise. +* Fill ref with the full refname for the branch. +*/ +extern int validate_branchname (const char *name,struct strbuf *ref); + +/* +* Check if a branch 'name' can be created as a new branch; die otherwise. +* 'force' can be used when it is OK for the named branch already exists. +* Return 1 if the named branch already exists; return 0 otherwise. +* Fill ref with the full refname for the branch. */ -int validate_new_branchname (const char *name,struct strbuf *ref,int force,int attr_only); +extern int validate_new_branchname (const char *name,struct strbuf *ref,int force);/* * Remove information about the state of working on the current
builtin/branch.c +4 -4
@@ -463,7 +463,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, intstruct strbuf oldref =STRBUF_INIT , newref =STRBUF_INIT , logmsg =STRBUF_INIT ;struct strbuf oldsection =STRBUF_INIT , newsection =STRBUF_INIT ;int recovery =0 ; -int clobber_head_ok;if (!oldname) {if (copy)
@@ -487,9 +486,10 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int* A command like "git branch -M currentbranch currentbranch" cannot * cause the worktree to become inconsistent with HEAD, so allow it. */ - clobber_head_ok = !strcmp (oldname, newname); - -validate_new_branchname (newname, &newref, force, clobber_head_ok); +if (!strcmp (oldname, newname)) +validate_branchname (newname, &newref); +else +validate_new_branchname (newname, &newref, force);reject_rebase_or_bisect_branch (oldref.buf);
builtin/checkout.c +5 -5
@@ -1289,11 +1289,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)if (opts.new_branch) {struct strbuf buf =STRBUF_INIT ; - opts.branch_exists = -validate_new_branchname (opts.new_branch, &buf, - !!opts.new_branch_force, - !!opts.new_branch_force); - +if (opts.new_branch_force) + opts.branch_exists =validate_branchname (opts.new_branch, &buf); +else + opts.branch_exists = +validate_new_branchname (opts.new_branch, &buf,0 );strbuf_release (&buf); }
Signature · 1 line
2.15.0-rc1-158-gbd035ae683
Re: [PATCH 2/3] branch: split validate_new_branchname() into two
Quoted 26 lines · Junio C Hamano
branch.c +7
-6@@ -178,19 +178,31 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)return 0 ; } -int validate_new_branchname (const char *name,struct strbuf *ref, -int force,int attr_only) +/* +* Check if 'name' can be a valid name for a branch; die otherwise. +* Return 1 if the named branch already exists; return 0 otherwise. +* Fill ref with the full refname for the branch. +*/ +int validate_branchname (const char *name,struct strbuf *ref) { -const char *head; -if (strbuf_check_branch_ref (ref, name))die (_ ("'%s' is not a valid branch name." ), name); -if (!ref_exists (ref->buf)) -return 0 ; +return ref_exists (ref->buf);
branch.c int read_branch_desc(struct strbuf *buf, const char *branch_name) · line 199
Quoted 10 lines
branch.c +7
-2+} -if (attr_only) -return 1 ; +/* +* Check if a branch 'name' can be created as a new branch; die otherwise. +* 'force' can be used when it is OK for the named branch already exists. +* Return 1 if the named branch already exists; return 0 otherwise. +* Fill ref with the full refname for the branch. +*/
I guess it's better to avoid repeating the comments in both the .h and .c file as they might quite easily become stale. I would prefer keeping it in the header file alone.
Quoted 12 lines
branch.c +6
-0+int validate_new_branchname (const char *name,struct strbuf *ref,int force) +{ +const char *head; + +if (!validate_branchname (name, ref)) +return 0 ;if (!force) die(_("A branch named '%s' already exists."),@@ -246,9 +258,9 @@ void create_branch(const char *name, const char *start_name,if (track ==BRANCH_TRACK_EXPLICIT || track ==BRANCH_TRACK_OVERRIDE ) explicit_tracking =1 ;
branch.c void create_branch(const char *name, const char *start_name, · line 266
Quoted 10 lines
branch.c +3
-3-if (validate_new_branchname (name, &ref, force, - track ==BRANCH_TRACK_OVERRIDE || - clobber_head)) { +if ((track ==BRANCH_TRACK_OVERRIDE || clobber_head) + ?validate_branchname (name, &ref) + :validate_new_branchname (name, &ref, force)) {if (!force) dont_change_ref =1 ;
The change was simple by splitting the function into two and calling the right function as required at every call site! As far as I could see this doesn't seem to be reducing the confusion that the 'attr_only' parameter caused. That's because the new validate_branchname function seems to be called in some cases when the 'force' parameter is true and in other cases the 'force' parameter is sent to the 'validate_new_branchname' function. So, I think consistency is lacking in this change. That's just my opinion, of course.
Signature · 1 line
Kaartic
Re: [PATCH 2/3] branch: split validate_new_branchname() into two
Quoted 10 lines · Kaartic Sivaraam
Quoted 6 lines
+/*
+ * Check if a branch 'name' can be created as a new branch; die otherwise.
+ * 'force' can be used when it is OK for the named branch already exists.
+ * Return 1 if the named branch already exists; return 0 otherwise.
+ * Fill ref with the full refname for the branch.
+ */I guess it's better to avoid repeating the comments in both the .h and .c file as they might quite easily become stale. I would prefer keeping it in the header file alone.
True. I wrote this while designing the code, so the copy in .c file came first, and then that was copied to .h file; the one in .c file can go.
Quoted 4 lines
The change was simple by splitting the function into two and calling the right function as required at every call site! As far as I could see this doesn't seem to be reducing the confusion that the '
attr_only' parameter caused.
The confusion primarily was the way the parameter was named. "forcing" does not have strong connection to marking "this is only asking about attributes". And removing that confusion, by separating the caller and making it explicit that the caller needs two separate behaviours depending on the condition, and naming the functions more appropriately (i.e. is this about creating a new one that must not exist already?), is the focus of this step.
PATCH 3/3 branch: forbid refs/heads/HEAD
In reply to Junio C Hamanostrbuf_check_branch_ref() is the central place where many codepaths see if a proposed name is suitable for the name of a branch. It was designed to allow us to get stricter than the check_refname_format() check used for refnames in general, and we already use it to reject a branch whose name begins with a '-'.
Use it to also reject "HEAD" as a branch name.
--- sha1_name.c | 2 +- t/t1430-bad-ref-name.sh | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-)
sha1_name.c +1 -1
@@ -1332,7 +1332,7 @@ 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 ] =='-' ) +if (*name =='-' || !strcmp (name,"HEAD" ))return -1 ;strbuf_splice (sb,0 ,0 ,"refs/heads/" ,11 );return check_refname_format (sb->buf,0 );
t/t1430-bad-ref-name.sh +8 -0
@@ -331,4 +331,12 @@ 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_expect_success 'checkout -b rejects HEAD as a branch name' ' + test_must_fail git checkout -B HEAD HEAD^ +' + test_done
Signature · 1 line
2.15.0-rc1-158-gbd035ae683
Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
Quoted 7 lines · Junio C Hamano
strbuf_check_branch_ref()is the central place where many codepaths see if a proposed name is suitable for the name of a branch. It was designed to allow us to get stricter than thecheck_refname_format()check used for refnames in general, and we already use it to reject a branch whose name begins with a '-'.Use it to also reject "HEAD" as a branch name.
Heh, I just pointed somebody to this a day or two ago as #leftoverbit. I guess it's taken now. :)
Related to this: should we do a better job of confirming that the refname is available for use?
If you do:
git branch foo
git checkout -b foo/bar
then "foo/bar" is not available. And for "checkout -b", we'd notice when we tried to create the ref. But for:
git checkout --orphan foo/bar
we'd update HEAD with a non-viable name, and only find out later during "git commit". That's not the end of the world, but it might be nice to complain when writing the symlink.
Largely orthogonal to the problem you're solving here, but I suspect it may touch the same code, so it might be worth thinking about while we're here.
Quoted 5 lines
t/t1430-bad-ref-name.sh +0
-0
t/t1430-bad-ref-name.sh test_expect_success 'update-ref --stdin -z fails delete with bad ref name' ' · line 340
Quoted 10 lines
t/t1430-bad-ref-name.sh +7
-0@@ -331,4 +331,12 @@ 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_expect_success 'checkout -b rejects HEAD as a branch name' ' + test_must_fail git checkout -B HEAD HEAD^ +'
Should we test that:
git update-ref refs/heads/HEAD HEAD^
continues to work?
-Peff
Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
Quoted 8 lines · Jeff King
Quoted 6 lines · Junio C Hamano
strbuf_check_branch_ref()is the central place where many codepaths see if a proposed name is suitable for the name of a branch. It was designed to allow us to get stricter than thecheck_refname_format()check used for refnames in general, and we already use it to reject a branch whose name begins with a '-'.
Quoted 4 lines
Quoted 1 line
Use it to also reject "HEAD" as a branch name.
Heh, I just pointed somebody to this a day or two ago as #leftoverbit. I guess it's taken now. :)
Didn't notice /remember it; sorry about that. I can retract it if you want, but perhaps they cannot unsee it ;-)
Quoted 18 lines
Quoted 5 lines
t/t1430-bad-ref-name.sh +0
-0t/t1430-bad-ref-name.sh test_expect_success 'update-ref --stdin -z fails delete with bad ref name' ' · line 340
Quoted 10 lines
t/t1430-bad-ref-name.sh +7
-0@@ -331,4 +331,12 @@ 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_expect_success 'checkout -b rejects HEAD as a branch name' ' + test_must_fail git checkout -B HEAD HEAD^ +'Should we test that:
Quoted 3 lines
git update-ref refs/heads/HEAD HEAD^
continues to work?
Perhaps. Also we may want to make sure that "git branch -D HEAD" still works as a way to recover from historical mistakes.
Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
Quoted 8 lines · Junio C Hamano
Quoted 5 lines
Should we test that:
git update-ref refs/heads/HEAD HEAD^
continues to work?
Perhaps. Also we may want to make sure that "git branch -D HEAD" still works as a way to recover from historical mistakes.
The only difference is improved tests where we use show-ref to make sure refs/heads/HEAD does not exist when it shouldn't, exercise update-ref to create and delete refs/heads/HEAD, and also make sure it can be deleted with "git branch -d".
-- >8 --strbuf_check_branch_ref() is the central place where many codepaths see if a proposed name is suitable for the name of a branch. It was designed to allow us to get stricter than the check_refname_format() check used for refnames in general, and we already use it to reject a branch whose name begins with a '-'.
Use it to also reject "HEAD" as a branch name.
--- sha1_name.c | 2 +- t/t1430-bad-ref-name.sh | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-)
sha1_name.c +1 -1
@@ -1332,7 +1332,7 @@ 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 ] =='-' ) +if (*name =='-' || !strcmp (name,"HEAD" ))return -1 ;strbuf_splice (sb,0 ,0 ,"refs/heads/" ,11 );return check_refname_format (sb->buf,0 );
t/t1430-bad-ref-name.sh +23 -0
@@ -331,4 +331,27 @@ 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_done
Signature · 1 line
2.15.0-rc1-158-gbd035ae683
Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
Quoted 8 lines · Junio C Hamano
Quoted 6 lines · Junio C Hamano
Quoted 5 lines
Should we test that:
git update-ref refs/heads/HEAD HEAD^
continues to work?
Quoted 7 lines
Quoted 2 lines
Perhaps. Also we may want to make sure that "git branch -D HEAD" still works as a way to recover from historical mistakes.
The only difference is improved tests where we use show-ref to make sure refs/heads/HEAD does not exist when it shouldn't, exercise update-ref to create and delete refs/heads/HEAD, and also make sure it can be deleted with "git branch -d".
Thanks, this looks good to me.
-Peff
Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
In reply to Junio C HamanoQuoted 9 lines · Junio C Hamano
Quoted 3 lines
Perhaps. Also we may want to make sure that "git branch -D HEAD" still works as a way to recover from historical mistakes.
The only difference is improved tests where we use show-ref to make sure refs/heads/HEAD does not exist when it shouldn't, exercise update-ref to create and delete refs/heads/HEAD, and also make sure it can be deleted with "git branch -d".
In which case you might also like to ensure that it's possible to "rename" the branch with a name "HEAD" to recover from historical mistakes.
sha1_name.c void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed) · line 1336
Quoted 11 lines
sha1_name.c +1
-1@@ -1332,7 +1332,7 @@ 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 ] =='-' ) +if (*name =='-' || !strcmp (name,"HEAD" ))return -1 ;
I guess this makes the check for "HEAD" in builtin/branch::cmd_branch()
(line 796) redundant. May be it could be removed?
Quoted 24 lines
sha1_name.c +0
-0strbuf_splice (sb,0 ,0 ,"refs/heads/" ,11 );return check_refname_format (sb->buf,0 );t/t1430-bad-ref-name.sh +14
-0@@ -331,4 +331,27 @@ 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 &&
t/t1430-bad-ref-name.sh test_expect_success 'update-ref --stdin -z fails delete with bad ref name' ' · line 357
Quoted 10 lines
t/t1430-bad-ref-name.sh +9
-0+ 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_done
So, may be the following test could also be added (untested yet),
test_expect_success 'branch -m can rename refs/heads/HEAD' '
git update-ref refs/heads/HEAD HEAD^ && git branch -m HEAD head && test_must_fail git show-ref refs/heads/HEAD
'
Signature · 1 line
Kaartic
Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
Quoted 8 lines · Kaartic Sivaraam
Quoted 4 lines
The only difference is improved tests where we use show-ref to make sure refs/heads/HEAD does not exist when it shouldn't, exercise update-ref to create and delete refs/heads/HEAD, and also make sure it can be deleted with "git branch -d".
In which case you might also like to ensure that it's possible to "rename" the branch with a name "HEAD" to recover from historical mistakes.
Perhaps. I didn't think it was all that needed---as long as you can delete, you can recreate at the same commit with a more desirable name, and it is not like users have tons of repositories with misnamed branches that they need to fix. The code may already handle it, or there may need even more code to support the rename; I didn't check.
Quoted 9 lines
sha1_name.c void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed) · line 1336
Quoted 6 lines
int
strbuf_check_branch_ref(struct strbuf *sb, const char *name){sha1_name.c +1
-1strbuf_branchname (sb, name,INTERPRET_BRANCH_LOCAL ); -if (name[0 ] =='-' ) +if (*name =='-' || !strcmp (name,"HEAD" ))return -1 ;I guess this makes the check for "HEAD" in builtin/branch::cmd_branch()
(line 796) redundant. May be it could be removed?
Perhaps. But I think that is better done as a follow-up "now the lower level consistently handles, let's remove the extra check that has become unnecessary" separate patch.
Quoted 7 lines
So, may be the following test could also be added (untested yet),
test_expect_success'branch -m can rename refs/heads/HEAD' 'git update-ref refs/heads/HEAD HEAD^ && git branch -m HEAD head && test_must_fail git show-ref refs/heads/HEAD'
Yeah, that would be a good material for that separate follow-up patch.
Thanks.
Re: [PATCH 3/3] branch: forbid refs/heads/HEAD
Quoted 4 lines · Junio C Hamano
... The code may already
handle it, or there may need even more code to support the rename; I didn't check.
As far as I could see there the code does seem to already handle the rename. This part of builtin/branch.c is what seems to be ensuring that,
if (strbuf_check_branch_ref(&oldref, oldname)) {
/*
* Bad name --- this could be an attempt to rename a
* ref that we used to allow to be created by accident.
*/
if (ref_exists(oldref.buf))
recovery = 1;
else
die(_("Invalid branch name: '%s'"), oldname);
}Signature · 1 line
Kaartic
Quoted 3 lines · Junio C Hamano
This started as a little clean-up for a NEEDSWORK comment in
branch.h, but it ended up adding a rather useful safety feature.
Part of this series seems to duplicate the work done in part of my series that tries to give more useful error messages when renaming a branch.
https://public-inbox.org/git/%[email protected]%3E/
Any reasons for this?
Signature · 1 line
Kaartic
Quoted 5 lines · Kaartic Sivaraam
Quoted 3 lines · Junio C Hamano
This started as a little clean-up for a NEEDSWORK comment in
branch.h, but it ended up adding a rather useful safety feature.
Quoted 7 lines
Part of this series seems to duplicate the work done in part of my series that tries to give more useful error messages when renaming a branch.
https://public-inbox.org/git/%[email protected]%3E/
Any reasons for this?
Sorry, but I am not sure what you are asking.
I was looking at the code, noticed NEEDSWORK comment and worked on cleaning things up---you seem to feel that I need a reason, or perhaps even your permission, when I try improving the codebase? That starts to sound a bit ridiculous.
As different developers are multiple people, it just happens that areas somebody changes overlap an area somebody else wants to change. It's not like anybody owns a piece of source file when s/he expresses an interest to work on something that may or may not affect that area.
Quoted 7 lines · Junio C Hamano
Sorry, but I am not sure what you are asking.
I was looking at the code, noticed NEEDSWORK comment and worked on cleaning things up---you seem to feel that I need a reason, or perhaps even your permission, when I try improving the codebase? That starts to sound a bit ridiculous.
Nothing like that, sorry. I was thinking that you would "remember" the fact that I was trying to clean up the NEEDSWORK in the patch series mentioned before as you have reviewed/commented on it. So, I thought there would be something my patch series was doing wrong which made you send another series that cleans up the NEEDSWORK. I just wanted to know that specific reason (the reason which made you send a series cleaning up the NEEDSWORK when you saw another series doing the same thing)?
Of course that's assuming that you remembered my series cleaning up the NEEDSWORK, in the first place. If that's not the case then there's no reason you could give :-)
Signature · 1 line
Kaartic
PATCH v2 1/2 branch: forbid refs/heads/HEAD
In reply to Junio C HamanoFrom: Junio C Hamano <[email protected]>
strbuf_check_branch_ref() is the central place where many codepaths see if a proposed name is suitable for the name of a branch. It was designed to allow us to get stricter than the check_refname_format() check used for refnames in general, and we already use it to reject a branch whose name begins with a '-'.
Use it to also reject "HEAD" as a branch name.
---
Changes in v2:
* Fixed the issue of .git/HEAD being renamed when trying to do,
$ git branch -m HEAD head
This also allows to rename a branch named HEAD that was created by accident.
cf. <[email protected]>
* Added a test to ensure that it is possible to rename a branch named HEAD.
sha1_name.c | 8 +++++++- t/t1430-bad-ref-name.sh | 29 +++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-)
sha1_name.c +7 -1
@@ -1438,9 +1438,15 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)strbuf_branchname (sb, name,INTERPRET_BRANCH_LOCAL ); elsestrbuf_addstr (sb ,name ); -if (name[0 ] =='-' ) +if (*name =='-' )return -1 ; +strbuf_splice (sb,0 ,0 ,"refs/heads/" ,11 ); + +/* HEAD is not to be used as a branch name */ +if (!strcmp (sb->buf,"refs/heads/HEAD" )) +return -1 ; +return check_refname_format (sb->buf,0 ); }
t/t1430-bad-ref-name.sh +29 -0
@@ -331,4 +331,33 @@ 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 head && + test_must_fail git show-ref refs/heads/HEAD +' + test_done
Signature · 1 line
2.15.0.rc2.397.geff0134c7.dirty
PATCH v2 2/2 builtin/branch: remove redundant check for HEAD
The lower level code has been made to handle this case for the sake of consistency. This has made this check redundant.
So, remove the redundant check.
--- builtin/branch.c | 3 --- 1 file changed, 3 deletions(-)
builtin/branch.c +0 -3
@@ -793,9 +793,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix) }else if (argc >0 && argc <=2 ) {struct branch *branch =branch_get (argv[0 ]); -if (!strcmp (argv[0 ],"HEAD" )) -die (_ ("it does not make sense to create 'HEAD' manually" )); -if (!branch)die (_ ("no such branch '%s'" ), argv[0 ]);
Signature · 1 line
2.15.0.rc2.397.geff0134c7.dirty
Re: [PATCH v2 1/2] branch: forbid refs/heads/HEAD
In reply to Kaartic SivaraamI should have been a little more clear with the numbering, sorry. The correct prefix should have been as follows,
* [PATCH v2 1/2] --> [PATCH v2 3/3]
* [PATCH v2 1/2] --> [PATCH v2 4/3]
Sorry for the inconvenience.
---
Kaartic
Re: [PATCH v2 1/2] branch: forbid refs/heads/HEAD
Quoted 8 lines · Kaartic Sivaraam
I should have been a little more clear with the numbering, sorry. The correct prefix should have been as follows,
* [PATCH v2 1/2] --> [PATCH v2 3/3]
* [PATCH v2 1/2] --> [PATCH v2 4/3]
Sorry for the inconvenience.
I assume that the second one above actually talks about what was sent as "v2 2/2" (not "v2 1/2") being "4/3"?
Are these two patches follow-up fixes (replacement of 3/3 plus an extra patch) to jc/branch-name-sanity topic?
Thanks for working on these.
Re: [PATCH v2 1/2] branch: forbid refs/heads/HEAD
Quoted 5 lines · Junio C Hamano
Quoted 3 lines · Kaartic Sivaraam
I should have been a little more clear with the numbering, sorry. The correct prefix should have been as follows,
Quoted 9 lines
Quoted 5 lines
* [PATCH v2 1/2] --> [PATCH v2 3/3]
* [PATCH v2 1/2] --> [PATCH v2 4/3]
Sorry for the inconvenience.
I assume that the second one above actually talks about what was sent as "v2 2/2" (not "v2 1/2") being "4/3"?
Yeah. Copy paste error, sorry.
Quoted 3 lines
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.
Quoted 2 lines
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.
Thanks,
Kaartic
PATCH 3/4 branch: correctly reject refs/heads/{-dash,HEAD}
strbuf_check_branch_ref() is the central place where many codepaths see if a proposed name is suitable for the name of a branch. It was designed to allow us to get stricter than the check_refname_format() check used for refnames in general, and we already use it to reject a branch whose name begins with a '-'. The function gets a strbuf and a string "name", and returns non-zero if the name is not appropriate as the name for a branch. When the name is good, it places the full refname for the branch with the proposed name in the strbuf before it returns.
However, it turns out that one caller looks at what is in the strbuf even when the function returns an error. Make the function populate the strbuf even when it returns an error. That way, when "-dash" is given as name, "refs/heads/-dash" is placed in the strbuf when returning an error to copy_or_rename_branch(), which notices that the user is trying to recover with "git branch -m -- -dash dash" to rename "-dash" to "dash".
While at it, use the same mechanism to also reject "HEAD" as a branch name.
---
Kaartic Sivaraam <[email protected]> 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'.
Thanks again.
sha1_name.c | 14 ++++++++++++-- t/t1430-bad-ref-name.sh | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-)
sha1_name.c +12 -2
@@ -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" )) +return -1 ; +return check_refname_format (sb->buf,0 ); }
t/t1430-bad-ref-name.sh +43 -0
47 lines not on this page · raw message
Signature · 1 line
2.15.0-358-g6c105002b3
Re: [PATCH 3/4] branch: correctly reject refs/heads/{-dash,HEAD}
Quoted 13 lines · Junio C Hamano
Kaartic Sivaraam <[email protected]> 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.
Quoted 6 lines
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.
Quoted 15 lines
sha1_name.c | 14 ++++++++++++-- t/t1430-bad-ref-name.sh | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-)sha1_name.c +1
-2@@ -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 ; +
sha1_name.c void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed) · line 1345
Quoted 10 lines
sha1_name.c +9
-0+/* +* 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 ;-)
Quoted 58 lines
sha1_name.c +2
-0+return -1 ; +return check_refname_format (sb->buf,0 ); }t/t1430-bad-ref-name.sh +43
-0@@ -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
Re: [PATCH 3/4] branch: correctly reject refs/heads/{-dash,HEAD}
Quoted 4 lines · Kaartic Sivaraam
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 ;-)
Quoted 4 lines
sha1_name.c void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed) · line 1345
Quoted 2 lines
sha1_name.c +2
-0+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
Quoted 2 lines
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}
Quoted 7 lines · Junio C Hamano
Quoted 4 lines · Kaartic Sivaraam
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.
Quoted 7 lines
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.
Quoted 3 lines
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). :-)
Quoted 5 lines
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.shscript may help this process).
Sure thing.
Quoted 21 lines
Quoted 4 lines
sha1_name.c void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed) · line 1345
Quoted 2 lines
sha1_name.c +2
-0+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
Quoted 2 lines
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.
Quoted 8 lines
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 ;-)