On Fri, Mar 13, 2015 at 2:18 PM, Sudhanshu Shekhar
<sudshekhar02@xxxxxxxxx> wrote:
> 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@xxxxxxxxx>
> Helped-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
> Helped-by: Matthieu Moy <Matthieu.Moy@xxxxxxxxxxxxxxx>
> Signed-off-by: Sudhanshu Shekhar <sudshekhar02@xxxxxxxxx>
> ---
> 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.
> /*
> * 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).
> 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".
> } 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html