From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sun, 06 Mar 2016 19:35:55 GMT
Subject: Re: [PATCH] Allow "-" as a short-hand for "@{-1}" in "git branch -d @{-1}"
Message-ID: <CAPig+cTHBLGjn_tnAKE2i_zv9TRxdVbNfONpxg=ZvRSz9-4t=Q@mail.gmail.com>
In-Reply-To: <1457268494-8394-2-git-send-email-dpdineshp2@gmail.com>

On Sun, Mar 6, 2016 at 7:48 AM, Dinesh Polathula <dpdineshp2@gmail.com> wrote:
> From: Dinesh <dpdineshp2@gmail.com>

You can drop this line and let git-am pick up your name and address
from the email envelope.

> Allow "-" as a short-hand for "@{-1}" in "git branch -d @{-1}"

    branch: -d/-D: recognize "-" as short-hand for @{-1}

> The "-" shorthand can be used as a replacement for "@{-1}" to refer
> to the previous branch the user was on in the "git branch -d @{-1}"
> command.

Does/should this also apply to -D?

> Replace "-" argument with "@{-1}" when the command line arguments
> are parsed.

This final sentence isn't really needed, as it's just repeating what
the patch itself already says.

> Signed-off-by: Dinesh Polathula<dpdineshp2@gmail.com>
> ---
>  builtin/branch.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

Does this need a documentation update? Is "-" documented for other
commands which recognize it specially or is knowledge of "-" implicit?

This change probably does deserve a new test or two.

> diff --git a/builtin/branch.c b/builtin/branch.c
> index 7b45b6b..98d2c4b 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -24,7 +24,7 @@
>  static const char * const builtin_branch_usage[] = {
>         N_("git branch [<options>] [-r | -a] [--merged | --no-merged]"),
>         N_("git branch [<options>] [-l] [-f] <branch-name> [<start-point>]"),
> -       N_("git branch [<options>] [-r] (-d | -D) <branch-name>..."),
> +       N_("git branch [<options>] [-r] (-d | -D) [-] <branch-name>..."),
>         N_("git branch [<options>] (-m | -M) [<old-branch>] <new-branch>"),
>         N_("git branch [<options>] [-r | -a] [--points-at]"),
>         NULL
> @@ -658,8 +658,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>         filter.abbrev = -1;
>
>         if (argc == 2 && !strcmp(argv[1], "-h"))
> -               usage_with_options(builtin_branch_usage, options);
> -
> +       {
> +               usage_with_options(builtin_branch_usage, options);
> +       }

Why add unnecessary braces (making the diff noisier, as well)?

> +       if (argc == 3 && !strcmp(argv[2], "-"))

The commit message talks about this applying only to -d (and
presumably -D), however, there doesn't seem to be any constraint
enforcing that.

Won't this logic fail if the user passes other options accepted by
git-branch, such as --quite (for instance, "git branch --quiet -d -")?
How do other Git commands which recognize "-" as alias for @{-1} deal
with this? Have you checked their implementations?

To address these issues, it seems like a more correct place to
recognize "-" as an alias would be somewhere within
builtin/branch.d:delete_branches().

> +       {
> +           argv[2] = "@{-1}";
> +       }

Style: Unnecessary braces.

>         git_config(git_branch_config, NULL);
>
>         track = git_branch_track;
> --
> 2.8.0.rc0