Re: [PATCH v3 3/4] reset: add option "--merge-safe" to "git reset"

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

Christian Couder <chriscool@tuxfamily.org> writes:

> From: Stephan Beyer <s-beyer@gmx.net>
>
> This option is nearly like "--merge" except that it is
> safer.

Do you still want to have this description after the last round?

> The table below shows what happens when running
> "git reset --option target" to reset the HEAD to another
> commit (as a special case "target" could be the same as
> HEAD) in the cases where "--merge" and "--merge-safe"
> (abreviated --m-s) behave differently.
>
> working index HEAD target         working index HEAD
>   B      B     A     A   --m-s      B      A     A
>                          --merge    A      A     A
>   B      B     A     C   --m-s       (disallowed)
>                          --merge    C      C     C

I'd like to see at least the following rows filled as well.

    X      U     A     A   --m-s      ???    ???   ???
                           --merge    ???    ???   ???
    X      U     B     A   --m-s      ???    ???   ???
                           --merge    ???    ???   ???

> In this table, A, B and C are some different states of a file.

... and X is "don't care", and U is "unmerged index".

> The code comes from the sequencer GSoC project:
>
> git://repo.or.cz/git/sbeyer.git
>
> (at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)
>
> But in the sequencer project the "reset" flag was set in the "struct
> unpack_trees_options" passed to "unpack_trees()". With this flag the
> changes in the working tree were discarded if the file was different
> between HEAD and the reset target.

If you need to have four lines worth of description here, is this still
Stephan's patch, or would it be more appropriate to say "This is based on
an earlier GSoC work by Stephan in git://repo.or.cz/git/sbeyer.git
repository." and you take all the credit and blame?

>  static const char * const git_reset_usage[] = {
> -	"git reset [--mixed | --soft | --hard | --merge] [-q] [<commit>]",
> +	"git reset [--mixed | --soft | --hard | --merge | --merge-safe] [-q] [<commit>]",
>  	"git reset [--mixed] <commit> [--] <paths>...",
>  	NULL
>  };

As we established in the previous round, this is _different_ from --merge,
but *not* in the sense that --merge is more dangerous and users should be
using this new option instead, but in the sense that --merge perfectly
works well for its intended use case, and this new option triggers a mode
of operation that is meant to be used in a completely different use case,
which is unspecified in this series without documentation.

In that light, is --merge-safe still a good name for the option, or merely
a misleading one?

As I said in the previous round, --merge discards the modified index state
when switching, and it is absolutely _the right thing to do_ in the use
case it was intended for.  It is _not_ dangerous, and using --merge-safe
in that scenario is not being _safe_ but is actively a _wrong_ thing to do.

> @@ -95,6 +98,16 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
>  
>  	read_cache_unmerged();
>  
> +	if (reset_type == MERGE_SAFE) {
> +		unsigned char *head_sha1;
> +		if (get_sha1("HEAD", head_sha1))
> +			return error("You do not have a valid HEAD.");
> +		if (parse_and_init_tree_desc(head_sha1, desc))
> +			return error("Failed to find tree of HEAD.");
> +		nr++;
> +		opts.fn = twoway_merge;
> +	}

get_sha1() takes an allocated buffer, does not allocate space on its own.
I think you meant "unsigned char head_sha1[20];" here.