Re: [PATCH] branch: add '-' to delete previous branch

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 29, 2020, 18:58 UTC
Message-ID: <20200429185851.GF83442@syl.local>
In-Reply-To: <20200429130133.520981-1-pickfire@riseup.net>

On Wed, Apr 29, 2020 at 09:01:33PM +0800, Ivan Tham wrote:

Show 22 quoted lines

> Add support to delete previous branch from git checkout/switch to have
> feature parity with git switch -.
>
> Signed-off-by: Ivan Tham <pickfire@riseup.net>
> ---
>  Documentation/git-branch.txt | 10 ++++++++++
>  builtin/branch.c             |  6 +++++-
>  t/t3200-branch.sh            |  7 +++++++
>  3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 135206ff4a..37e7cbbc52 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -265,6 +265,10 @@ start-point is either a local or remote-tracking branch.
>  	The new branch name must pass all checks defined by
>  	linkgit:git-check-ref-format[1].  Some of these checks
>  	may restrict the characters allowed in a branch name.
> ++
> +You can use the `@{-N}` syntax to refer to the N-th last branch checked out
> +using "git checkout" operation. You may also specify `-` which is synonymous to
> +`@{-1}`.

Interesting; we're already using strbuf_branchname, so the first part of this documentation was true even before this commit. Would you consider splitting this into two patches?

The first should include the first sentence of this documentation, an additional test in t3200 exercising an explicit 'git branch -D @{-N}' for some 'N', but no changes in builtin/branch.c. The second patch would then make '-' a synonym for '@{-1}'.

Show 47 quoted lines

>  <start-point>::
>  	The new branch head will point to this commit.  It may be
> @@ -334,6 +338,12 @@ $ git branch -D test                                    <2>
>  <2> Delete the "test" branch even if the "master" branch (or whichever branch
>      is currently checked out) does not have all commits from the test branch.
>
> +To delete the previous branch::
> ++
> +------------
> +$ git branch -D -
> +------------
> +
>  Listing branches from a specific remote::
>  +
>  ------------
> diff --git a/builtin/branch.c b/builtin/branch.c
> index d8297f80ff..5537f819a6 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -227,9 +227,13 @@ 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;
> +		const char *arg = argv[i];
>  		int flags = 0;
>
> -		strbuf_branchname(&bname, argv[i], allowed_interpret);
> +		if (!strcmp(arg, "-"))
> +			arg = "@{-1}";
> +
> +		strbuf_branchname(&bname, arg, allowed_interpret);
>  		free(name);
>  		name = mkpathdup(fmt, bname.buf);
>
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 411a70b0ce..6819107c1d 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -1387,4 +1387,11 @@ test_expect_success 'invalid sort parameter in configuration' '
>  	)
>  '
>
> +test_expect_success 'delete previous branch' '
> +	git checkout -b a &&
> +	git checkout -b b &&
> +	git branch -D - &&
> +	test_path_is_missing .git/refs/heads/a

This assertion is a little uncommon in t3200 for this style of test. Please use the more-standard:

  git branch >actual &&
  cat >expect <<-\EOF &&
     master
   * b
  EOF
  test_cmp expect actual

Substituting the body of the heredoc for the actual state of things. This form makes it a little clearer what branches should and shouldn't exist.

Show 6 quoted lines

> +'
> +
>  test_done
> --
> 2.26.2
>

Thanks, Taylor