| Message ID | 0ac8cd48-08d7-9bdd-b074-c8d5ded522f6@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 77e7267e47aac247244dc4fb3538baff7463261f |
| Headers | show |
| Series | branch: error copying or renaming a detached HEAD | expand |
Rubén Justo <rjusto@gmail.com> writes: > Let's remove the condition in copy_or_rename_branch() (the current > function name) and check for HEAD before calling it, dying with the > original intended error if we're in a detached HEAD. Makes sense. > @@ -827,24 +820,19 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > strbuf_release(&buf); > > return ret; > - } else if (copy) { > - if (!argc) > - die(_("branch name required")); > - else if (argc == 1) > - copy_or_rename_branch(head, argv[0], 1, copy > 1); > - else if (argc == 2) > - copy_or_rename_branch(argv[0], argv[1], 1, copy > 1); > - else > - die(_("too many branches for a copy operation")); > - } else if (rename) { > + } else if (copy || rename) { > if (!argc) > die(_("branch name required")); > + else if ((argc == 1) && filter.detached) > + die(copy? _("cannot copy the current branch while not on any.") > + : _("cannot rename the current branch while not on any.")); Missing " " before "?". > else if (argc == 1) > - copy_or_rename_branch(head, argv[0], 0, rename > 1); > + copy_or_rename_branch(head, argv[0], copy, copy + rename > 1); The third argument being 'copy' makes sense (as the original has 1 for copy and 0 for rename). As we reject if rename and copy are both set, here we have either copy > 0 or rename > 0 but not both, so the fourth argument makes sense, too. > else if (argc == 2) > - copy_or_rename_branch(argv[0], argv[1], 0, rename > 1); > + copy_or_rename_branch(argv[0], argv[1], copy, copy + rename > 1); > else > - die(_("too many arguments for a rename operation")); > + die(copy? _("too many branches for a copy operation") > + : _("too many arguments for a rename operation")); Ditto. > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh > index 7d8edff9c3..38c57de71b 100755 > --- a/t/t3200-branch.sh > +++ b/t/t3200-branch.sh > @@ -268,6 +268,17 @@ test_expect_success 'git branch -M topic topic should work when main is checked > git branch -M topic topic > ' > > +test_expect_success 'git branch -M and -C fail on detached HEAD' ' > + git checkout HEAD^{} && > + test_when_finished git checkout - && > + echo "fatal: cannot rename the current branch while not on any." >expect && > + test_must_fail git branch -M must-fail 2>err && > + test_cmp expect err && > + echo "fatal: cannot copy the current branch while not on any." >expect && > + test_must_fail git branch -C must-fail 2>err && > + test_cmp expect err > +' Excellent. I'll do the whitespace tweak while queueing, so there is no need to resend only to fix them. Thanks.
diff --git a/builtin/branch.c b/builtin/branch.c index 407517ba68..c964ac7bb4 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -520,13 +520,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int const char *interpreted_newname = NULL; int recovery = 0; - if (!oldname) { - if (copy) - die(_("cannot copy the current branch while not on any.")); - else - die(_("cannot rename the current branch while not on any.")); - } - if (strbuf_check_branch_ref(&oldref, oldname)) { /* * Bad name --- this could be an attempt to rename a @@ -827,24 +820,19 @@ int cmd_branch(int argc, const char **argv, const char *prefix) strbuf_release(&buf); return ret; - } else if (copy) { - if (!argc) - die(_("branch name required")); - else if (argc == 1) - copy_or_rename_branch(head, argv[0], 1, copy > 1); - else if (argc == 2) - copy_or_rename_branch(argv[0], argv[1], 1, copy > 1); - else - die(_("too many branches for a copy operation")); - } else if (rename) { + } else if (copy || rename) { if (!argc) die(_("branch name required")); + else if ((argc == 1) && filter.detached) + die(copy? _("cannot copy the current branch while not on any.") + : _("cannot rename the current branch while not on any.")); else if (argc == 1) - copy_or_rename_branch(head, argv[0], 0, rename > 1); + copy_or_rename_branch(head, argv[0], copy, copy + rename > 1); else if (argc == 2) - copy_or_rename_branch(argv[0], argv[1], 0, rename > 1); + copy_or_rename_branch(argv[0], argv[1], copy, copy + rename > 1); else - die(_("too many arguments for a rename operation")); + die(copy? _("too many branches for a copy operation") + : _("too many arguments for a rename operation")); } else if (new_upstream) { struct branch *branch; struct strbuf buf = STRBUF_INIT; diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 7d8edff9c3..38c57de71b 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -268,6 +268,17 @@ test_expect_success 'git branch -M topic topic should work when main is checked git branch -M topic topic ' +test_expect_success 'git branch -M and -C fail on detached HEAD' ' + git checkout HEAD^{} && + test_when_finished git checkout - && + echo "fatal: cannot rename the current branch while not on any." >expect && + test_must_fail git branch -M must-fail 2>err && + test_cmp expect err && + echo "fatal: cannot copy the current branch while not on any." >expect && + test_must_fail git branch -C must-fail 2>err && + test_cmp expect err +' + test_expect_success 'git branch -v -d t should work' ' git branch t && git rev-parse --verify refs/heads/t &&
In c847f53712 (Detached HEAD (experimental), 2007-01-01) an error condition was introduced in rename_branch() to prevent renaming, later also copying, a detached HEAD. The condition used was checking for NULL in oldname, the source branch to rename/copy. That condition cannot be satisfied because if no source branch is specified, HEAD is going to be used in the call. The error issued instead is: fatal: Invalid branch name: 'HEAD' Let's remove the condition in copy_or_rename_branch() (the current function name) and check for HEAD before calling it, dying with the original intended error if we're in a detached HEAD. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- builtin/branch.c | 28 ++++++++-------------------- t/t3200-branch.sh | 11 +++++++++++ 2 files changed, 19 insertions(+), 20 deletions(-)