Re: [PATCH 1/2] Adding - shorthand for @{-1} in RESET command

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 10, 2015, 06:54 UTC
Message-ID: <CAPig+cRAB-LQctj6UOKUXps-MEh2C_EbSp_3=wfgxtWx6xCbhw@mail.gmail.com>
In-Reply-To: <1425934010-8780-1-git-send-email-dyoucme@gmail.com>

On Mon, Mar 9, 2015 at 4:46 PM, Sundararajan R <dyoucme@gmail.com> wrote:

> Please give feedback and suggest things I may have missed out on.
> I hope I have incorporated all the suggestions.

If you haven't already, read Documentation/SubmittingPatches. Pay particular attention to section #2 which explains how to write a good commit message, and to section #4 to learn where to place patch commentary not intended as part of the permanent commit history. The above lines are commentary, not meant as part of the commit message. Place such commentary below the '---' line just before the diffstat.

> Subject: Adding - shorthand for @{-1} in RESET command

Prefix the first line of the commit message with the module or command you re changing. Drop capitalization. Write in imperative mood. For instance:

    reset: add '-' shorthand for '@{-1}'

> Signed-off-by: Sundararajan R <dyoucme@gmail.com>
> Thanks-to: Junio C Hamano

Place your sign-off last. Use Helped-by: rather than Thanks-to: and include the person's full name and email address.

> ---

Here, just below the '---' line is where you should place commentary not intended for the permanent commit record.

> I have attempted to resolve the ambiguity when there exists a file named -
> by communicating to the user that he/she can use ./- when he/she wants to refer
> to the - file. I perform this check using the check_filename() function.

This is important information for the commit message itself above the '---' line, though you would want to rephrase it just to state the facts. No need to mention "I did this" or "I did that" since the patch itself implies that you made the changes.

Show 12 quoted lines

>  builtin/reset.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 4c08ddc..2bdd5cd 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -192,6 +192,7 @@ static void parse_args(struct pathspec *pathspec,
>  {
>         const char *rev = "HEAD";
>         unsigned char unused[20];
> +       int file_named_minus=0;

Style: Here and elsewhere, add a space around '='.

Show 9 quoted lines

>         /*
>          * Possible arguments are:
>          *
> @@ -205,6 +206,12 @@ static void parse_args(struct pathspec *pathspec,
>          */
>
>         if (argv[0]) {
> +               if (!strcmp(argv[0], "-") && !argv[1]) {
> +                       if(!check_filename(prefix,"-"))

Style: Here and elsewhere, add space after 'if'.
Style: Add space after comma.

Show 17 quoted lines

> +                               argv[0]="@{-1}";
> +                       else
> +                               file_named_minus=1;
> +               }
>                 if (!strcmp(argv[0], "--")) {
>                         argv++; /* reset to HEAD, possibly with paths */
>                 } else if (argv[1] && !strcmp(argv[1], "--")) {
> @@ -226,7 +233,14 @@ static void parse_args(struct pathspec *pathspec,
>                         rev = *argv++;
>                 } else {
>                         /* Otherwise we treat this as a filename */
> -                       verify_filename(prefix, argv[0], 1);
> +                       if(file_named_minus) {
> +                               die(_("ambiguous argument '-': both revision and filename\n"
> +                                       "Use ./- for file named -\n"
> +                                       "Use '--' to separate paths from revisions, like this:\n"
> +                                       "'git <command> [<revision>...] -- [<file>...]'"));

This seems odd. If arguments following '--' are unconditionally treated as paths, why is it be necessary to tell the user to spell out file '-' as './-'? Shouldn't "git reset -- -" be sufficient?

Show 8 quoted lines

> +                       }
> +                       else
> +                               verify_filename(prefix, argv[0], 1);
>                 }
>         }
>         *rev_ret = rev;
> --
> 2.1.0