On Tue, Mar 10, 2015 at 6:52 AM, Sudhanshu Shekhar
<sudshekhar02@xxxxxxxxx> wrote:
> 'git reset -' will reset to the previous branch. It will behave similar
> to @{-1} except when a file named '@{-1}' is present. To refer to a file
> named '-', use ./- or the -- 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>
> ---
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 4c08ddc..8abd300 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -205,6 +206,10 @@ static void parse_args(struct pathspec *pathspec,
> */
>
> if (argv[0]) {
> + if (!strcmp(argv[0], "-")) {
> + argv[0] = "@{-1}";
It's somewhat ugly to modify an element of argv[] since you don't own
the array and the contract does not particularly suggest that is
modifiable by you. A more serious concern is that doing so creates a
tighter binding between parse_args() and its caller since parse_args()
doesn't know how the caller will be disposing of argv[] when finished
with it. Say, for instance, that the caller disposes of each argv[]
element by invoking free(argv[n]). The literal "@{-1}" you've assigned
here is not allocated on the heap, so free()ing it would be wrong.
However, some of the other commands which alias "-" to "@{-1}" also
modify argv[] in this way, so we'll let it slide for the moment.
> + substituted_minus = 1;
> + }
> if (!strcmp(argv[0], "--")) {
> argv++; /* reset to HEAD, possibly with paths */
> } else if (argv[1] && !strcmp(argv[1], "--")) {
> @@ -222,15 +227,21 @@ static void parse_args(struct pathspec *pathspec,
> * Ok, argv[0] looks like a commit/tree; it should not
> * be a filename.
> */
> + if (substituted_minus)
> + argv[0] = "-";
> verify_non_filename(prefix, argv[0]);
> + if (substituted_minus)
> + argv[0] = "@{-1}";
Do you find it ugly that you have to undo and then redo your argv[]
change from a few lines above? Rather than jumping through such hoops,
can't you instead define a new variable which holds the appropriate
value ("@{-1}"), or rework the logic to avoid such kludges altogether?
> rev = *argv++;
> } else {
> + /* We were treating "-" as a commit and not a file */
> + if (substituted_minus)
> + argv[0] = "-";
> /* Otherwise we treat this as a filename */
> verify_filename(prefix, argv[0], 1);
> }
> }
> *rev_ret = rev;
> -
Mentioned by Matthieu: Don't sneak in unrelated changes. This change
was probably unintentional, but serves as a good reminder that you
should review the patch yourself before sending it. (And, if you do
have a need to make cleanups, it's typically best to do so as a
separate preparatory patch.)
> if (read_cache() < 0)
> die(_("index file corrupt"));
>
> --
> 2.3.1.279.gd534259
--
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