Re: [PATCH v5 1/2] reset: enable '-' short-hand for previous branch
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Mar 13, 2015, 20:48 UTC
- Message-ID
- <CAPig+cTuCtS6EKr9AqGkH_dDkxdYtk9ZfkpCLDLY9XYQO04ANA@mail.gmail.com>
- In-Reply-To
- <1426270716-22405-1-git-send-email-sudshekhar02@gmail.com>
On Fri, Mar 13, 2015 at 2:18 PM, Sudhanshu Shekhar <sudshekhar02@gmail.com> wrote:
Show 27 quoted lines
> git reset '-' will reset to the previous branch. To reset a file named
> "-" use either "git reset ./-" or "git reset -- -".
>
> Change error message to treat single "-" as an ambigous revision or
> path rather than a bad 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>
> ---
> I have changed the logic to ensure argv[0] isn't changed at any point.
> Creating a modified_argv0 variable let's me do that.
>
> I couldn't think of any other way to achieve this, apart from changing things
> directly in the sha1_name.c file (like Junio's changes). Please let me know
> if some further changes are needed.
>
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 4c08ddc..bc50e14 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];
> + const char *modified_argv0 = argv[0];This variable is only needed inside the 'if (argv[0])' block below, so its declaration should be moved there. Also the initialization to argv[0] is wasted since the assignment is overwritten below.
The variable name itself could be better. Unlike a name such as 'orig_arg0', "modified" doesn't tell us much. Modified how? Modified to be what? Consideration where and how the variable is used, we can see that it will be holding a value that _might_ be a "rev". This suggests a better name such as 'maybe_rev' or something similar.
Show 11 quoted lines
> /*
> * Possible arguments are:
> *
> @@ -205,10 +206,17 @@ static void parse_args(struct pathspec *pathspec,
> */
>
> if (argv[0]) {
> + if (!strcmp(argv[0], "-")) {
> + modified_argv0 = "@{-1}";
> + }
> + else {Style: cuddle the 'else' and braces: "} else {"> + modified_argv0 = argv[0]; > + }
The unnecessary braces make this harder to read than it could be since it is so spread out vertically. Dropping the braces would help. The ternary operator ?: might improve readability (though it might also make it worse).
Show 24 quoted lines
> if (!strcmp(argv[0], "--")) {
> argv++; /* reset to HEAD, possibly with paths */
> } else if (argv[1] && !strcmp(argv[1], "--")) {
> - rev = argv[0];
> + rev = modified_argv0;
> argv += 2;
> }
> /*
> @@ -216,14 +224,15 @@ static void parse_args(struct pathspec *pathspec,
> * has to be unambiguous. If there is a single argument, it
> * can not be a tree
> */
> - else if ((!argv[1] && !get_sha1_committish(argv[0], unused)) ||
> - (argv[1] && !get_sha1_treeish(argv[0], unused))) {
> + else if ((!argv[1] && !get_sha1_committish(modified_argv0, unused)) ||
> + (argv[1] && !get_sha1_treeish(modified_argv0, unused))) {
> /*
> * Ok, argv[0] looks like a commit/tree; it should not
> * be a filename.
> */
> verify_non_filename(prefix, argv[0]);
> - rev = *argv++;
> + rev = modified_argv0;
> + argv++;Good. This is much better than the previous rounds, and is the sort of solution I had hoped to see when prodding you in previous reviews to avoid argv[] kludges. Unlike the previous band-aid approach, this demonstrates that you took the time to understand the overall logic flow rather than merely plopping in a "quick fix".
Show 13 quoted lines
> } else {
> /* Otherwise we treat this as a filename */
> verify_filename(prefix, argv[0], 1);
> diff --git a/setup.c b/setup.c
> index 979b13f..b621b62 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -200,7 +200,7 @@ void verify_filename(const char *prefix,
> int diagnose_misspelt_rev)
> {
> if (*arg == '-')
> - die("bad flag '%s' used after filename", arg);
> + die("ambiguous argument '%s': unknown revision or path", arg);The conditional is only checking if the first character of 'arg' is hyphen; it's not checking if 'arg' is exactly "-". It's purpose is to recognize -flags or --flags, so it's inappropriate to change the error message like this. I think this also doesn't help the case when there really is a file named "-", since this conditional will just claim that it's ambiguous.
It might or might not be appropriate to add a special case here to allow an exact "-" to fall through to the check_filename() call below, however, it would be necessary to thoroughly check for possible repercussions first. (I haven't checked.) If all else fails, you could change parse_args() to do something a bit ugly like this:
const char *f = strcmp(argv[0], "-") ? argv[0] : "./-";
verify_filename(prefix, f);> if (check_filename(prefix, arg)) > return; > die_verify_filename(prefix, arg, diagnose_misspelt_rev); > --
By now, you've had a taste of what it's like to participate in the Git project and what will be expected of you, and GSoC mentors have (hopefully) formed an opinion of your abilities and how you interact with reviewers, so I'm not sure that it makes sense for you to resubmit again. Junio's proposal[1] to generalize "-" recognition as an alias for @{-1} may be worth pursing by someone, but may be too large for a micro-project.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/265260