On Tue, Mar 10, 2015 at 6:03 PM, Sudhanshu Shekhar
<sudshekhar02@xxxxxxxxx> wrote:
> [PATCH v3 1/2] reset: enable '-' short-hand for previous branch
This should be v4, I think, not v3.
> git reset -' will reset to the previous branch. It will behave similar
> to @{-1} except when a file named '@{-1}' is present.
The way this is phrased, the reader is left hanging: "What happens
when a file named @{-1} is present?"
> To refer to a file named '-', use ./- or the -- flag.
A documentation update to reflect this change would be appropriate.
See, for instance, 696acf45 (checkout: implement "-" abbreviation, add
docs and tests; 2009-01-17).
> 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>
> ---
> Eric, I have added a user_input variable to record the input entered
> by the user. This way I can avoid the multiple 'if' clauses. Thank you
> for the suggestion.
>
> I have also removed the unrelated change that I had unintentionally
> committed. I am sending this patch on the thread for further review.
> Once both the patches are reviewed and accepted, I will create a new
> mail for it. Hope that is okay.
Please wrap commentary to about 72 columns, just as you would the
commit message, rather than typing it all on a single line. (I wrapped
it manually here in order to reply to it.)
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 4c08ddc..b428241 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -192,6 +192,8 @@ static void parse_args(struct pathspec *pathspec,
> {
> const char *rev = "HEAD";
> unsigned char unused[20];
> + int substituted_minus = 0;
> + char *user_input = argv[0];
You're assigning a 'const char *' to a 'char *'. The compiler should
have warned about it.
This variable name is not as expressive as it could be. Try to name
the variable after what you expect it to represent, for instance
"maybe_rev" or "rev_or_path" or something more suitable. Even
"orig_argv0" would be more informative than "user_input".
> /*
> * Possible arguments are:
> *
> @@ -205,6 +207,10 @@ static void parse_args(struct pathspec *pathspec,
> */
>
> if (argv[0]) {
> + if (!strcmp(argv[0], "-")) {
> + argv[0] = "@{-1}";
> + substituted_minus = 1;
> + }
> if (!strcmp(argv[0], "--")) {
> argv++; /* reset to HEAD, possibly with paths */
> } else if (argv[1] && !strcmp(argv[1], "--")) {
> @@ -222,9 +228,12 @@ static void parse_args(struct pathspec *pathspec,
> * Ok, argv[0] looks like a commit/tree; it should not
> * be a filename.
> */
> - verify_non_filename(prefix, argv[0]);
> + verify_non_filename(prefix, user_input);
> rev = *argv++;
> } else {
> + /* We were treating "-" as a commit and not a file */
> + if (substituted_minus)
> + argv[0] = "-";
In my review of the previous version[1], I mentioned that it was ugly
to muck with argv[0]; moreover that it was particularly ugly to have
to muck with it multiple times, undoing and redoing assignments.
Although you've eliminated some reassignments via 'user_input', my
intent, by asking if you could rework the logic, was to prompt you to
take a couple steps back and examine the overall logic of the function
more closely. The munging of argv[0] is effectively a fragile and
undesirable band-aid approach. It is possible to support '-' as an
alias for '@{-1}' without having to resort to such kludges at all.
One other concern: When there is no previous branch, and no file named
"-", then 'git reset -' misleadingly complains "bad flag '-' used
after filename", rather than the more appropriate "ambiguous argument
'-': unknown revision or path".
[1]: http://article.gmane.org/gmane.comp.version-control.git/265255
> /* Otherwise we treat this as a filename */
> verify_filename(prefix, argv[0], 1);
> }
> --
> 2.3.1.278.ge5c7b1f.dirty
--
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