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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mar 27, 2018, 19:11 UTC
Message-ID: <20180327191139.GD4343@aiede.svl.corp.google.com>
In-Reply-To: <1522176390-646-2-git-send-email-p@aaronjgreenberg.com>

Hi,

Aaron Greenberg wrote:

Show 8 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.

Thanks for a clear example.

[...]

>  builtin/branch.c  | 3 +++
>  t/t3200-branch.sh | 8 ++++++++
>  2 files changed, 11 insertions(+)

[...]

Show 14 quoted lines

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

[...]

Show 12 quoted lines

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

[...]

Show 8 quoted lines

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