Re: [PATCH] branch: delete now accepts '-' as branch name

[Junio C Hamano <gitster@pobox.com>]

> From: "Erlend E. Aasland" <erlend.aasland@innova.no>
>
> This makes it easy to get rid of short-lived branches:
>
> $ git switch -c experiment
> $ git switch -
> $ git branch -D -

Or you can use @{-1} directly.  Or short-lived experiment can
directly be done on HEAD without any branch ;-)

In any case, the above is sufficient demonstration.  I do not think
you'd need a second one that is identical---having two may help you
demonstrate there are multiple ways to start a short-lived branch
that you want to get rid of quickly, but it does not help convince
others how useful "branch -" is in such situations.

> $ gh pr checkout nnn
> $ make && make test
> $ git switch -
> $ git branch -D -
>
> Signed-off-by: Erlend E. Aasland <erlend.aasland@innova.no>
> ---


> diff --git a/builtin/branch.c b/builtin/branch.c
> index 4ce2a247542..f6c876c09d2 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -236,7 +236,11 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  		char *target = NULL;
>  		int flags = 0;
>  
> -		strbuf_branchname(&bname, argv[i], allowed_interpret);
> +		if (!strcmp(argv[i], "-")) {
> +			strbuf_branchname(&bname, "@{-1}", allowed_interpret);
> +		} else {
> +			strbuf_branchname(&bname, argv[i], allowed_interpret);
> +		}
>  		free(name);
>  		name = mkpathdup(fmt, bname.buf);

You do not need if/else here.

+	if (!strcmp(argv[i], "-"))
+		argv[i] = "@{-1}";
	strbuf_branchname(&bname, argv[i], allowed_interpret);

should be sufficient.  If you want to avoid assigning to argv[i],
if/else is OK, but then lose the unnecessary {pair of braces} around
single-statement blocks.

> +test_expect_success 'git branch -D - should delete branch @{-1}' '
> +	git switch -c j &&
> +	git switch - &&
> +	git branch -D - &&
> +	test_path_is_missing .git/refs/heads/j &&

These days we try hard *NOT* to assume how refs are stored in the
system.  Please don't peek into .git/ at filesystem level, when you
can achieve what you want to in some other way.

    test_must_fail git show-ref --verify refs/heads/j

would be a more direct way.

> +	test_must_fail git reflog exists refs/heads/j

We should try not to assume that reflog for ref X is lost when ref X
itself goes away in newer tests and update such assumptions in older
tests.  Ideally we would want to be able to say "at time X, branch
'j' was removed, and at time X+Y, branch 'j' was recreated", and
adding more of these will make such an improvement harder to make.

This script is riddled with instances of these existing problems.
Please don't make it even worse by adding on top of them.

The added ones is a positive test.  A new feature works as expected
in a situation where it should kick in.  There needs an accompanying
negative test or two, making sure it fails sensibly in a situation
where it should fail.  Off the top of my head, negative tests that
are appropriate for this new feature are:

    * In a freshly created repository, possibly creating a few
      commits on the initial branch without ever switching to a
      different branch, what should happen when you use the new
      "branch -d -" feature?  What error message, if any, should
      the user see?  Do we show that expected message?

    * Switch to a new branch, create a commit on top, switch back to
      the original branch and run "branch -d -".  This should fail
      because you'd be losing the commit created on the branch.
      What error message should the user see?  Do we show the name
      of the branch correctly?  Do we show "@{-1}", or "-"?

    * After running "branch -D -" to successfully remove the
      previous branch, run "branch -D -" again.  This should fail
      because the branch you are trying to remove no longer exists.
      What error message should the user see?

Thanks for trying to make Git better.