Thank you all for your reviews and feedback. I will try and submit
this patch by taking my time to think about the various solutions and
coming up with the best one. I will also try and see if I can assist
Junio with his JFF patch.
I have learned a lot during the process of implementing this micro
project and believe that if I get selected for a gsoc under git, it
will help me become a better developer overall.
I have gone over the ideas page and I am interested in the "git bisect
--first-parent" and "git bisect fixed/unfixed" projects. I am looking
the source code at git-bisect.sh and will try and come up with a
proposal for review by the git community.
Thank you all for you time and patience.
Regards,
Sudhanshu
On Sat, Mar 14, 2015 at 2:18 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> 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