[PATCH] branch: implement shortcut to delete last branch

[PATCH] branch: implement shortcut to delete last branch

[Aaron Greenberg]

With the approvals listed in [*1*] and in accordance with the
guidelines set out in Documentation/SubmittingPatches, I am submitting
this patch to be applied upstream.

After work on this patch is done, I'll look into picking up where the
prior work done in [*2*] left off.

Is there anything else that needs to be done before this can be
accepted?

[Reference]

*1* https://public-inbox.org/git/1521844835-23956-2-git-send-email-p@aaronjgreenberg.com/
*2* https://public-inbox.org/git/1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com/

[PATCH] branch: implement shortcut to delete last branch

[Aaron Greenberg]

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>
---
 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}";
+
 		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)" &&
-- 
2.7.4

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

[Jonathan Nieder]

Hi,

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.

Thanks for a clear example.

[...]
>  builtin/branch.c  | 3 +++
>  t/t3200-branch.sh | 8 ++++++++
>  2 files changed, 11 insertions(+)
[...]
> With the approvals listed in [*1*] and in accordance with the
> guidelines set out in Documentation/SubmittingPatches, I am submitting
> this patch to be applied upstream.
>
> After work on this patch is done, I'll look into picking up where the
> prior work done in [*2*] left off.
>
> Is there anything else that needs to be done before this can be
> accepted?
>
> [Reference]
>
> *1* https://public-inbox.org/git/1521844835-23956-2-git-send-email-p@aaronjgreenberg.com/
> *2* https://public-inbox.org/git/1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com/

For the future, please don't use a separate cover letter message in a
single-patch series like this one.  Instead, please put any discussion
that you don't want to go in the commit message after the three-dash
divider in the same message as the patch, like the diffstat.  See the
section "Sending your patches" in Documentation/SubmittingPatches for
more details:

| You often want to add additional explanation about the patch,
| other than the commit message itself.  Place such "cover letter"
| material between the three-dash line and the diffstat.  For
| patches requiring multiple iterations of review and discussion,
| an explanation of changes between each iteration can be kept in
| Git-notes and inserted automatically following the three-dash
| line via `git format-patch --notes`.

That makes it easier for reviewers to see all the information in one
place and in particular can help them in fleshing out the commit
message if it is missing details.

[...]
> 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}";
> +
>  		strbuf_branchname(&bname, argv[i], allowed_interpret);

This makes me wonder: should the "-" shortcut be handled in
strbuf_branchname itself?  That would presumably simplify callers like
this one.

[...]
> --- 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 &&

This naming scheme feels likely to conflict with other patches.
How about something like

	git checkout -B previous &&
	git checkout -B new-branch &&
	git show-ref --verify refs/heads/previous &&
	git branch -d - &&
	test_must_fail git show-ref --verify refs/heads/previous

?

> +	git checkout  - &&
> +	test_path_is_file .git/refs/heads/my7.1 &&
> +	git branch -d - &&
> +	test_path_is_missing .git/refs/heads/my7.1

not specific to this test, but this is relying on low-level details
and means that an implementation that e.g. deleted a loose ref but
kept a packed ref would pass the test despite being broken.

Some of the other tests appear to use show-ref, so that might work
well.

No need to act on this, since what you have here is at least
consistent with some of the other tests in the file.  In other words,
it might be even better to address this throughout the file in a
separate patch.

> +'
> +

A few questions that the tests leave unanswered for me:

 1. Does "git branch -d -" refuse to delete an un-merged branch
    like "git branch -d topic" would?  (That seems like a valuable
    thing to test for typo protection reasons.)

 2. What happens if there is no previous branch, as in e.g. a new
    clone?

 3. What does the error message look like when it cannot delete the
    previous branch for whatever reason?  Does it identify the branch
    that can't be deleted?

>  test_expect_success 'test --track without .fetch entries' '
>  	git branch --track my8 &&
>  	test "$(git config branch.my8.remote)" &&

Thanks and hope that helps,
Jonathan

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

[Ævar Arnfjörð Bjarmason]

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

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

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 27 2018, Ævar Arnfjörð Bjarmason wrote:

> [...]With that, some comments on the change below:

Also, didn't mean to gang up on you. I only saw Jonathan's E-Mail after
I sent mine, and it covered some of the same stuff.

[PATCH] branch: implement shortcut to delete last branch

[Aaron Greenberg]

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. One of my common workflows
is to do some work on a local topic branch and push it to a remote,
where it gets merged in to 'master'. Then, I switch back to my local
master, fetch the remote master, and delete the previous topic branch.

$ git checkout -b topic-a
$ # Do some work...
$ git commit -am "Implement feature A"
$ git push origin topic-a

# 'origin/topic-a' gets merged into 'origin/master'

$ git checkout master
$ git branch -d topic-a
$ # With this patch, a user could simply type
$ git branch -d -

I think it's a useful shortcut for cleaning up a just-merged branch
(or a just switched-from branch.)

[PATCH] branch: implement shortcut to delete last branch

[Aaron Greenberg]

Add support for using the "-" shortcut to delete the last checked-out
branch. This functionality already exists for git-merge, git-checkout,
and git-revert.

Signed-off-by: Aaron Greenberg <p@aaronjgreenberg.com>
---
 builtin/branch.c  | 3 +++
 t/t3200-branch.sh | 9 +++++++++
 2 files changed, 12 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}";
+
 		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..a3ffd54 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -776,6 +776,15 @@ 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.2 &&
+	git checkout  - &&
+	sha1=$(git rev-parse my7 | cut -c 1-7) &&
+	echo "Deleted branch my7.2 (was $sha1)." >expect &&
+	git branch -d - >actual 2>&1 &&
+	test_i18ncmp expect actual
+'
+
 test_expect_success 'test --track without .fetch entries' '
 	git branch --track my8 &&
 	test "$(git config branch.my8.remote)" &&
-- 
2.7.4

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

[Jeff King]

On Fri, Mar 23, 2018 at 02:09:25AM +0000, 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. One of my common workflows
> is to do some work on a local topic branch and push it to a remote,
> where it gets merged in to 'master'. Then, I switch back to my local
> master, fetch the remote master, and delete the previous topic branch.
> 
> $ git checkout -b topic-a
> $ # Do some work...
> $ git commit -am "Implement feature A"
> $ git push origin topic-a
> 
> # 'origin/topic-a' gets merged into 'origin/master'
> 
> $ git checkout master
> $ git branch -d topic-a
> $ # With this patch, a user could simply type
> $ git branch -d -
> 
> I think it's a useful shortcut for cleaning up a just-merged branch
> (or a just switched-from branch.)

I don't use "-" myself, but I can see how this would be useful. Do note
that in a discussion last year there was some hesitation about allowing
"-" for destructive commands:

  https://public-inbox.org/git/vpqh944eof7.fsf@anie.imag.fr/

I don't really have a strong opinion either way.

The details in this cover letter probably should go into the commit
message. The diff itself looks OK (the assumption of a 7-char
abbreviation in the test is a little gross, but I see you're just
following existing convention in the file).

-Peff

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

[Jeff King]

[resending; I cc'd Matthieu on his address from that old thread, but it
 bounced]

On Fri, Mar 23, 2018 at 04:56:36AM -0400, Jeff King wrote:

> On Fri, Mar 23, 2018 at 02:09:25AM +0000, 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. One of my common workflows
> > is to do some work on a local topic branch and push it to a remote,
> > where it gets merged in to 'master'. Then, I switch back to my local
> > master, fetch the remote master, and delete the previous topic branch.
> > 
> > $ git checkout -b topic-a
> > $ # Do some work...
> > $ git commit -am "Implement feature A"
> > $ git push origin topic-a
> > 
> > # 'origin/topic-a' gets merged into 'origin/master'
> > 
> > $ git checkout master
> > $ git branch -d topic-a
> > $ # With this patch, a user could simply type
> > $ git branch -d -
> > 
> > I think it's a useful shortcut for cleaning up a just-merged branch
> > (or a just switched-from branch.)
> 
> I don't use "-" myself, but I can see how this would be useful. Do note
> that in a discussion last year there was some hesitation about allowing
> "-" for destructive commands:
> 
>   https://public-inbox.org/git/vpqh944eof7.fsf@anie.imag.fr/
> 
> I don't really have a strong opinion either way.
> 
> The details in this cover letter probably should go into the commit
> message. The diff itself looks OK (the assumption of a 7-char
> abbreviation in the test is a little gross, but I see you're just
> following existing convention in the file).
> 
> -Peff

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

[Aaron Greenberg]

I updated the commit message to include my first email's cover letter
and cleaned up the test.

Copying Junio, since he also had good comments in the conversation you
linked.

I can appreciate Matthieu's points on the use of "-" in destructive
commands. As of this writing, git-merge supports the "-" shorthand,
which while not destructive, is at least _mutative_. Also,
"git branch -d" is not destructive in the same way that "rm -rf" is
destructive since you can recover the branch using the reflog.

One thing to consider is that approval of this patch extends the
implementation of the "-" shorthand in a piecemeal, rather than
consistent, way (implementing it in a consistent way was the goal of
the patch set you mentioned in your previous email.) Is that okay? Or
is it better to pick up the consistent approach where it was left?

[PATCH] branch: implement shortcut to delete last branch

[Aaron Greenberg]

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>
---
 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}";
+
 		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)" &&
-- 
2.7.4