Re: [PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- Mar 21, 2016, 17:24 UTC
- Message-ID
- <vpqfuvju4km.fsf@anie.imag.fr>
- In-Reply-To
- <1458573317-15532-1-git-send-email-elena.petrashen@gmail.com>
Hi, and welcome!
Elena Petrashen <elena.petrashen@gmail.com> writes:
Show 5 quoted lines
> 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.
Show 5 quoted lines
> * 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?
Show 8 quoted lines
> --- 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().
Show 6 quoted lines
> 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.
Show 12 quoted lines
> --- 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/