Re: [PATCH] branch: implement shortcut to delete last branch
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Mar 27, 2018, 19:23 UTC
- Message-ID
- <87tvt1wce3.fsf@evledraar.gmail.com>
- In-Reply-To
- <1522176390-646-2-git-send-email-p@aaronjgreenberg.com>
On Tue, Mar 27 2018, Aaron Greenberg wrote:
Show 23 quoted lines
> This patch gives git-branch the ability to delete the previous > checked-out branch using the "-" shortcut. This shortcut already exists > for git-checkout, git-merge, and git-revert. A common workflow is > > 1. Do some work on a local topic-branch and push it to a remote. > 2. 'remote/topic-branch' gets merged in to 'remote/master'. > 3. Switch back to local master and fetch 'remote/master'. > 4. Delete previously checked-out local topic-branch. > > $ git checkout -b topic-a > $ # Do some work... > $ git commit -am "Implement feature A" > $ git push origin topic-a > > $ git checkout master > $ git branch -d topic-a > $ # With this patch, a user could simply type > $ git branch -d - > > "-" is a useful shortcut for cleaning up a just-merged branch > (or a just switched-from branch.) > > Signed-off-by: Aaron Greenberg <p@aaronjgreenberg.com>
So just a tip on this E-Mail chain/patch submission. When you submit a v2 make the subject "[PATCH v2] ...", see Documentation/SubmittingPatches, also instead of sending two mails with the same subject better to put any comments not in the commit message...
> ---
...right here, below the triple dash, and CC the people commenting on the initial thread. With that, some comments on the change below:
Show 15 quoted lines
> builtin/branch.c | 3 +++
> t/t3200-branch.sh | 8 ++++++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 6d0cea9..9e37078 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -221,6 +221,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> char *target = NULL;
> int flags = 0;
>
> + if (!strcmp(argv[i], "-"))
> + argv[i] = "@{-1}";
> +If we just do this, then when I do the following:
1. be on the 'foo' branch
2. checkout 'bar', commit
3. checkout 'foo'
4. git branch -d -I get this message:
error: The branch 'bar' is not fully merged.
If you are sure you want to delete it, run 'git branch -D bar'While that works, I think it's better UI for us to suggest what's actually the important alternation to the user's command, i.e. replace -d with -D, otherwise they'll think "oh '-' doesn't work, let's try to name the branch", only to get the same error. I.e. this on top:
diff --git a/builtin/branch.c b/builtin/branch.c index cdf2de4f1d..081a4384ce 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -157,17 +157,18 @@ static int branch_merged(int kind, const char *name, static int check_branch_commit(const char *branchname, const char *refname, const struct object_id *oid, struct commit *head_rev, - int kinds, int force) + int kinds, int force, int resolved_dash) { struct commit *rev = lookup_commit_reference(oid); if (!rev) { - error(_("Couldn't look up commit object for '%s'"), refname); + error(_("Couldn't look up commit object for '%s'"), resolved_dash ? "-" : refname); return -1; } if (!force && !branch_merged(kinds, branchname, rev, head_rev)) { error(_("The branch '%s' is not fully merged.\n" "If you are sure you want to delete it, " - "run 'git branch -D %s'."), branchname, branchname); + "run 'git branch -D %s'."), branchname, + resolved_dash ? "-" : branchname); return -1; } return 0; @@ -220,9 +221,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, for (i = 0; i < argc; i++, strbuf_reset(&bname)) { char *target = NULL; int flags = 0; + int resolved_dash = 0; - if (!strcmp(argv[i], "-")) + if (!strcmp(argv[i], "-")) { argv[i] = "@{-1}"; + resolved_dash = 1; + } strbuf_branchname(&bname, argv[i], allowed_interpret); free(name); @@ -255,7 +259,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, if (!(flags & (REF_ISSYMREF|REF_ISBROKEN)) && check_branch_commit(bname.buf, name, &oid, head_rev, kinds, - force)) { + force, resolved_dash)) { ret = 1; goto next; } There are other error messages there, but as far as I can tell it's best if those just talk about the "bar" branch, but have a look. A test for that with i18ngrep left as an exercise... > strbuf_branchname(&bname, argv[i], allowed_interpret); > free(name); > name = mkpathdup(fmt, bname.buf); > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh > index 6c0b7ea..78c25aa 100755 > --- a/t/t3200-branch.sh > +++ b/t/t3200-branch.sh > @@ -776,6 +776,14 @@ test_expect_success 'deleting currently checked out branch fails' ' > test_must_fail git branch -d my7 > ' > > +test_expect_success 'test deleting last branch' ' > + git checkout -b my7.1 && > + git checkout - && > + test_path_is_file .git/refs/heads/my7.1 && > + git branch -d - && > + test_path_is_missing .git/refs/heads/my7.1 > +' > + > test_expect_success 'test --track without .fetch entries' ' > git branch --track my8 && > test "$(git config branch.my8.remote)" && I don't know how much this applies to the existing commands you mentioned (looks like not), but for "branch" specifically this looks very incomplete, in particular: * There's other modes where it takes commit-ish, e.g. "git branch foo bar" to create a new branch foo starting at bar, but with your patch "git branch foo -" won't work, even though there's no reason not to think it does. * There's no docs here to explain this difference, or TODO tests for maybe making that work later. Your patch would be a lot easier to review if you went through t/t3200-branch.sh, found the commit-ish occurances you don't support, and added failing tests for those, and explain in the commit message something to the effect of "I'm only making *this* work, here's the cases that *don't* work".