Re: Re* [PATCH] cmd_reset: don't trash uncommitted changes unless told to

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> "Avery Pennarun" <apenwarr@gmail.com> writes:
>
>> On 6/25/08, Junio C Hamano <gitster@pobox.com> wrote:
>>> "Avery Pennarun" <apenwarr@gmail.com> writes:
>>> >>   * You say "git checkout -- file" when you want to "check out the file
>>>  >>    from the index";
>>>  >
>>>  > The real question here is the --.  Is it strictly needed?  It's
>>>  > optional in things like git-diff, which just do their best to guess
>>>  > what you mean if you don't use the --.
>>>
>>> No, I wrote -- only for clarity, because you can happen to have a branch
>>>  whose name is the same as the file.  Otherwise you can safely omit it,
>>>  just like git-diff and any other commands that follow the -- convention.
>>
>> Oops, I got mixed up.  Only git-reset requires the --.  Would it make
>> sense to bring git-reset into line with everything else, then?
>
> Ah, interesting.  It appears that the current "reset in C" inherited that
> bug from the scripted version.  It works most of the time without --
> except for one place.
>
> 	# prove that the work tree is clean...
> 	$ git reset --hard
> 	HEAD is now at 7b7f39e Fix use after free() in builtin-fetch
> 	$ git diff
>         $ git diff --cached
>
> 	# what's different since HEAD^?
>         $ git diff --name-only HEAD^
>         builtin-fetch.c
>
>         # reset the path
>         $ git reset HEAD^ builtin-fetch.c
> 	builtin-fetch.c: needs update
>
> 	# prove that HEAD did not move
> 	$ git rev-parse HEAD
> 	7b7f39eae6ab0bbcc68d3c42a5b23595880e528f
> 	# prove that work tree did not change
>         $ git diff HEAD
>         # prove that index has old version
> 	$ git diff --cached HEAD^
>
> Reset is about resetting the index and --hard option tells it to propagate
> the change down to the work tree as well.
>
> There is no "reset to the index", so "reset -- path" would be a redundant
> way to spell "reset HEAD path" or "reset HEAD -- path" which is even more
> redundant.
>
> As long as builti-fetch.c is not a valid ref, you should be able to get
> out of the above mess by any one of:
>
> 	$ git reset builtin-fetch.c
>         $ git reset -- builtin-fetch.c
>         $ git reset HEAD builtin-fetch.c
>
> but the first one complains, saying builtin-fetch.c is not a valid ref.
>
> This may help.
>
> diff --git a/builtin-reset.c b/builtin-reset.c
> index f34acb1..c7d60f5 100644
> --- a/builtin-reset.c
> +++ b/builtin-reset.c
> @@ -194,9 +194,21 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>  	reflog_action = args_to_str(argv);
>  	setenv("GIT_REFLOG_ACTION", reflog_action, 0);
>  
> -	if (i < argc && strcmp(argv[i], "--"))
> -		rev = argv[i++];
> -
> +	/*
> +	 * Possible arguments are:
> +	 *
> +	 * git reset <rev> <paths>...
> +	 * git reset <rev> -- <paths>...
> +	 * git reset -- <paths>...
> +	 * git reset <paths>...
> +	 */
> +	if (i < argc && strcmp(argv[i], "--")) {
> +		/* could be "git reset <path>" */
> +		if (get_sha1(argv[i+1], sha1))

typofix: s/i+1/i/;

> +			;
> +		else
> +			rev = argv[i++];
> +	}
>  	if (get_sha1(rev, sha1))
>  		die("Failed to resolve '%s' as a valid ref.", rev);
>