Re: [PATCH v3 3/4] reset: add option "--merge-safe" to "git reset"
- From
Christian Couder <christian.couder@gmail.com>
- Date
- Sep 17, 2009, 12:25 UTC
- Message-ID
- <c07716ae0909170525h21ab26f5y84b0fbce2192c69@mail.gmail.com>
- In-Reply-To
- <7vk4zykv7o.fsf@alter.siamese.dyndns.org>
On Thu, Sep 17, 2009 at 7:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 31 quoted lines
> 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.
Show 15 quoted lines
>> 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.
Show 16 quoted lines
>> 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.
Show 16 quoted lines
>> @@ -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.