Skip to content

Commit 1616928

Browse files
gitstergitster
committed
Merge branch 'jc/branch-name-sanity'
"git branch" and "git checkout -b" are now forbidden from creating a branch whose name is "HEAD". * jc/branch-name-sanity: builtin/branch: remove redundant check for HEAD branch: correctly reject refs/heads/{-dash,HEAD} branch: split validate_new_branchname() into two branch: streamline "attr_only" handling in validate_new_branchname()
2 parents 5f9953d + 662a4c8 commit 1616928

6 files changed

Lines changed: 106 additions & 43 deletions

File tree

‎branch.c‎

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -178,24 +178,40 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
178178
return 0;
179179
}
180180

181-
int validate_new_branchname(const char *name, struct strbuf *ref,
182-
int force, int attr_only)
181+
/*
182+
* Check if 'name' can be a valid name for a branch; die otherwise.
183+
* Return 1 if the named branch already exists; return 0 otherwise.
184+
* Fill ref with the full refname for the branch.
185+
*/
186+
int validate_branchname(const char *name, struct strbuf *ref)
183187
{
184188
if (strbuf_check_branch_ref(ref, name))
185189
die(_("'%s' is not a valid branch name."), name);
186190

187-
if (!ref_exists(ref->buf))
191+
return ref_exists(ref->buf);
192+
}
193+
194+
/*
195+
* Check if a branch 'name' can be created as a new branch; die otherwise.
196+
* 'force' can be used when it is OK for the named branch already exists.
197+
* Return 1 if the named branch already exists; return 0 otherwise.
198+
* Fill ref with the full refname for the branch.
199+
*/
200+
int validate_new_branchname(const char *name, struct strbuf *ref, int force)
201+
{
202+
const char *head;
203+
204+
if (!validate_branchname(name, ref))
188205
return 0;
189-
else if (!force && !attr_only)
190-
die(_("A branch named '%s' already exists."), ref->buf + strlen("refs/heads/"));
191206

192-
if (!attr_only) {
193-
const char *head;
207+
if (!force)
208+
die(_("A branch named '%s' already exists."),
209+
ref->buf + strlen("refs/heads/"));
210+
211+
head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
212+
if (!is_bare_repository() && head && !strcmp(head, ref->buf))
213+
die(_("Cannot force update the current branch."));
194214

195-
head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
196-
if (!is_bare_repository() && head && !strcmp(head, ref->buf))
197-
die(_("Cannot force update the current branch."));
198-
}
199215
return 1;
200216
}
201217

@@ -242,9 +258,9 @@ void create_branch(const char *name, const char *start_name,
242258
if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
243259
explicit_tracking = 1;
244260

245-
if (validate_new_branchname(name, &ref, force,
246-
track == BRANCH_TRACK_OVERRIDE ||
247-
clobber_head)) {
261+
if ((track == BRANCH_TRACK_OVERRIDE || clobber_head)
262+
? validate_branchname(name, &ref)
263+
: validate_new_branchname(name, &ref, force)) {
248264
if (!force)
249265
dont_change_ref = 1;
250266
else

‎branch.h‎

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -23,22 +23,19 @@ void create_branch(const char *name, const char *start_name,
2323
int clobber_head, int quiet, enum branch_track track);
2424

2525
/*
26-
* Validates that the requested branch may be created, returning the
27-
* interpreted ref in ref, force indicates whether (non-head) branches
28-
* may be overwritten. A non-zero return value indicates that the force
29-
* parameter was non-zero and the branch already exists.
30-
*
31-
* Contrary to all of the above, when attr_only is 1, the caller is
32-
* not interested in verifying if it is Ok to update the named
33-
* branch to point at a potentially different commit. It is merely
34-
* asking if it is OK to change some attribute for the named branch
35-
* (e.g. tracking upstream).
36-
*
37-
* NEEDSWORK: This needs to be split into two separate functions in the
38-
* longer run for sanity.
39-
*
26+
* Check if 'name' can be a valid name for a branch; die otherwise.
27+
* Return 1 if the named branch already exists; return 0 otherwise.
28+
* Fill ref with the full refname for the branch.
29+
*/
30+
extern int validate_branchname(const char *name, struct strbuf *ref);
31+
32+
/*
33+
* Check if a branch 'name' can be created as a new branch; die otherwise.
34+
* 'force' can be used when it is OK for the named branch already exists.
35+
* Return 1 if the named branch already exists; return 0 otherwise.
36+
* Fill ref with the full refname for the branch.
4037
*/
41-
int validate_new_branchname(const char *name, struct strbuf *ref, int force, int attr_only);
38+
extern int validate_new_branchname(const char *name, struct strbuf *ref, int force);
4239

4340
/*
4441
* Remove information about the state of working on the current

‎builtin/branch.c‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -463,7 +463,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
463463
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;
464464
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
465465
int recovery = 0;
466-
int clobber_head_ok;
467466

468467
if (!oldname) {
469468
if (copy)
@@ -487,9 +486,10 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
487486
* A command like "git branch -M currentbranch currentbranch" cannot
488487
* cause the worktree to become inconsistent with HEAD, so allow it.
489488
*/
490-
clobber_head_ok = !strcmp(oldname, newname);
491-
492-
validate_new_branchname(newname, &newref, force, clobber_head_ok);
489+
if (!strcmp(oldname, newname))
490+
validate_branchname(newname, &newref);
491+
else
492+
validate_new_branchname(newname, &newref, force);
493493

494494
reject_rebase_or_bisect_branch(oldref.buf);
495495

@@ -793,9 +793,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
793793
} else if (argc > 0 && argc <= 2) {
794794
struct branch *branch = branch_get(argv[0]);
795795

796-
if (!strcmp(argv[0], "HEAD"))
797-
die(_("it does not make sense to create 'HEAD' manually"));
798-
799796
if (!branch)
800797
die(_("no such branch '%s'"), argv[0]);
801798

‎builtin/checkout.c‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1287,11 +1287,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
12871287
if (opts.new_branch) {
12881288
struct strbuf buf = STRBUF_INIT;
12891289

1290-
opts.branch_exists =
1291-
validate_new_branchname(opts.new_branch, &buf,
1292-
!!opts.new_branch_force,
1293-
!!opts.new_branch_force);
1294-
1290+
if (opts.new_branch_force)
1291+
opts.branch_exists = validate_branchname(opts.new_branch, &buf);
1292+
else
1293+
opts.branch_exists =
1294+
validate_new_branchname(opts.new_branch, &buf, 0);
12951295
strbuf_release(&buf);
12961296
}
12971297

‎sha1_name.c‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1438,9 +1438,19 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
14381438
strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
14391439
else
14401440
strbuf_addstr(sb, name);
1441-
if (name[0] == '-')
1442-
return -1;
1441+
1442+
/*
1443+
* This splice must be done even if we end up rejecting the
1444+
* name; builtin/branch.c::copy_or_rename_branch() still wants
1445+
* to see what the name expanded to so that "branch -m" can be
1446+
* used as a tool to correct earlier mistakes.
1447+
*/
14431448
strbuf_splice(sb, 0, 0, "refs/heads/", 11);
1449+
1450+
if (*name == '-' ||
1451+
!strcmp(sb->buf, "refs/heads/HEAD"))
1452+
return -1;
1453+
14441454
return check_refname_format(sb->buf, 0);
14451455
}
14461456

‎t/t1430-bad-ref-name.sh‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -331,4 +331,47 @@ test_expect_success 'update-ref --stdin -z fails delete with bad ref name' '
331331
grep "fatal: invalid ref format: ~a" err
332332
'
333333

334+
test_expect_success 'branch rejects HEAD as a branch name' '
335+
test_must_fail git branch HEAD HEAD^ &&
336+
test_must_fail git show-ref refs/heads/HEAD
337+
'
338+
339+
test_expect_success 'checkout -b rejects HEAD as a branch name' '
340+
test_must_fail git checkout -B HEAD HEAD^ &&
341+
test_must_fail git show-ref refs/heads/HEAD
342+
'
343+
344+
test_expect_success 'update-ref can operate on refs/heads/HEAD' '
345+
git update-ref refs/heads/HEAD HEAD^ &&
346+
git show-ref refs/heads/HEAD &&
347+
git update-ref -d refs/heads/HEAD &&
348+
test_must_fail git show-ref refs/heads/HEAD
349+
'
350+
351+
test_expect_success 'branch -d can remove refs/heads/HEAD' '
352+
git update-ref refs/heads/HEAD HEAD^ &&
353+
git branch -d HEAD &&
354+
test_must_fail git show-ref refs/heads/HEAD
355+
'
356+
357+
test_expect_success 'branch -m can rename refs/heads/HEAD' '
358+
git update-ref refs/heads/HEAD HEAD^ &&
359+
git branch -m HEAD tail &&
360+
test_must_fail git show-ref refs/heads/HEAD &&
361+
git show-ref refs/heads/tail
362+
'
363+
364+
test_expect_success 'branch -d can remove refs/heads/-dash' '
365+
git update-ref refs/heads/-dash HEAD^ &&
366+
git branch -d -- -dash &&
367+
test_must_fail git show-ref refs/heads/-dash
368+
'
369+
370+
test_expect_success 'branch -m can rename refs/heads/-dash' '
371+
git update-ref refs/heads/-dash HEAD^ &&
372+
git branch -m -- -dash dash &&
373+
test_must_fail git show-ref refs/heads/-dash &&
374+
git show-ref refs/heads/dash
375+
'
376+
334377
test_done

0 commit comments

Comments
 (0)