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

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 [off-list ref] wrote:

On Fri, Mar 13, 2015 at 2:18 PM, Sudhanshu Shekhar
[off-list ref] wrote:

quoted

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 [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Helped-by: Matthieu Moy [off-list ref]
Signed-off-by: Sudhanshu Shekhar <redacted>
---
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.

quoted

        /*
         * 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 {"

quoted

+                       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).

quoted

                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".

quoted

                } 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);

quoted

        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