Re: [PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Hi, and welcome!

Elena Petrashen <elena.petrashen@gmail.com> writes:

> Signed-off-by: Elena Petrashen <elena.petrashen@gmail.com>
>
> ---
> This micro-patch is meant to allow “-“ as a short-hand for
> “@{-1} for branch -D (Cf. $gmane/230828):

Is it a good thing to do?

I find "git checkout -" to be a very nice shortcut, because users very
often want to get back to the branch they used to be before.

But I'm not sure how often people want to delete (force-delete according
to your message) the branch they just come from. It might be less
dangerous to give incentive to the user to spell the branch name
completely to avoid mistake. As analogy, my shell knows "cd -" but I
can't "rm -fr -" and I'm happy about it.

Not a strong objection, but I think you can motivate the change better
in the commit message, or give it up if I convinced you that it wasn't a
good idea.

> * git branch (-d | -D) is not supposed to accept any other
> arguments except for branch name so it makes sense to replace
> the argv[i] with @{-1}. We will not lose the opportunity to
> use it for something different for other git branch uses if
> we will decide it’s required.

This could go inside the commit message, not below the ---.

> +As a special case, the "@{-N}" syntax for the N-th last branch
> +deletes the specified branch. 

It's not really a special case. The @{-N} syntax works everywhere,
doesn't it?

> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -169,6 +169,8 @@ static int check_branch_commit(const char *branchname, const char *refname,
>  	return 0;
>  }
>  
> +
> +

Don't include useless changes in your patches. Using "git add -p" and
"git commit" without -a helps. Review your patch carefully and eliminate
such artefacts before sending: they only distract reviewers.

> +static void allow_dash_as_prev_branch_alias(const char **argv, int dash_position)

The function name looks overly verbose and I'm not sure it describes
exactly what it does: it does not really "allow", but it "expands" it.
I'd call it expand_dash_shortcut().

>  		if (!target) {
> -			error(remote_branch
> -			      ? _("remote-tracking branch '%s' not found.")
> -			      : _("branch '%s' not found."), bname.buf);
> +			error(!strcmp(bname.buf, "@{-1}")
> +				? _("There is no previous branch that could be referred to at the moment.")

This is not directly related to your change: the user could already
write "@{-1}" and may already have wanted a better error message. I
think this could/should be split into a separate patch.

> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -403,6 +403,16 @@ test_expect_success 'test deleting branch without config' '
>  	test_i18ncmp expect actual
>  '
>  
> +test_expect_success 'test deleting "-" deletes previous branch' '
> +	git checkout -b prev &&
> +	test_commit prev &&
> +	git checkout master &&
> +	git branch -D - >actual &&
> +	sha1=$(git rev-parse prev | cut -c 1-7) &&

git rev-parse --short avoids this "cut".

Regards,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/