Re: [PATCH v3 1/2] reset: enable '-' short-hand for previous branch

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 10, 2015, 19:18 UTC
Message-ID: <CAPig+cQdSHQTVwOwb9hee6On_Bv4qrh0MhiR-aRDMXqhw5L9wQ@mail.gmail.com>
In-Reply-To: <1425984728-27996-1-git-send-email-sudshekhar02@gmail.com>

On Tue, Mar 10, 2015 at 6:52 AM, Sudhanshu Shekhar <sudshekhar02@gmail.com> wrote:

Show 19 quoted lines

> 'git reset -' will reset to the previous branch. It will behave similar
> to @{-1} except when a file named '@{-1}' is present. To refer to a file
> named '-', use ./- or the -- flag.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
> ---
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 4c08ddc..8abd300 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -205,6 +206,10 @@ static void parse_args(struct pathspec *pathspec,
>          */
>
>         if (argv[0]) {
> +               if (!strcmp(argv[0], "-")) {
> +                       argv[0] = "@{-1}";

It's somewhat ugly to modify an element of argv[] since you don't own the array and the contract does not particularly suggest that is modifiable by you. A more serious concern is that doing so creates a tighter binding between parse_args() and its caller since parse_args() doesn't know how the caller will be disposing of argv[] when finished with it. Say, for instance, that the caller disposes of each argv[] element by invoking free(argv[n]). The literal "@{-1}" you've assigned here is not allocated on the heap, so free()ing it would be wrong.

However, some of the other commands which alias "-" to "@{-1}" also modify argv[] in this way, so we'll let it slide for the moment.

Show 14 quoted lines

> +                       substituted_minus = 1;
> +               }
>                 if (!strcmp(argv[0], "--")) {
>                         argv++; /* reset to HEAD, possibly with paths */
>                 } else if (argv[1] && !strcmp(argv[1], "--")) {
> @@ -222,15 +227,21 @@ static void parse_args(struct pathspec *pathspec,
>                          * Ok, argv[0] looks like a commit/tree; it should not
>                          * be a filename.
>                          */
> +                       if (substituted_minus)
> +                               argv[0] = "-";
>                         verify_non_filename(prefix, argv[0]);
> +                       if (substituted_minus)
> +                               argv[0] = "@{-1}";

Do you find it ugly that you have to undo and then redo your argv[] change from a few lines above? Rather than jumping through such hoops, can't you instead define a new variable which holds the appropriate value ("@{-1}"), or rework the logic to avoid such kludges altogether?

Show 11 quoted lines

>                         rev = *argv++;
>                 } else {
> +                       /* We were treating "-" as a commit and not a file */
> +                       if (substituted_minus)
> +                               argv[0] = "-";
>                         /* Otherwise we treat this as a filename */
>                         verify_filename(prefix, argv[0], 1);
>                 }
>         }
>         *rev_ret = rev;
> -

Mentioned by Matthieu: Don't sneak in unrelated changes. This change was probably unintentional, but serves as a good reminder that you should review the patch yourself before sending it. (And, if you do have a need to make cleanups, it's typically best to do so as a separate preparatory patch.)

Show 5 quoted lines

>         if (read_cache() < 0)
>                 die(_("index file corrupt"));
>
> --
> 2.3.1.279.gd534259