Re: [PATCH] branch: implement shortcut to delete last branch

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Mar 27 2018, Aaron Greenberg wrote:

> 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:

>  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".