On Thu, Sep 17, 2009 at 7:15 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Christian Couder <chriscool@xxxxxxxxxxxxx> writes:
>
>> From: Stephan Beyer <s-beyer@xxxxxxx>
>>
>> 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.
--
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