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

[Christian Couder <christian.couder@gmail.com>]

On Thu, Sep 17, 2009 at 7:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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".

I will have a look, but it seems to me that --m-s and --merge
behave the same in these cases.

>> 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?

As you insist, I will 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?

I agree that it could be misleading. What about "--stash" ?

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

Ok.

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

Yes.

Thanks,
Christian.