From: Christian Couder Date: Thu, 17 Sep 2009 12:25:40 GMT Subject: Re: [PATCH v3 3/4] reset: add option "--merge-safe" to "git reset" Message-ID: In-Reply-To: <7vk4zykv7o.fsf@alter.siamese.dyndns.org> On Thu, Sep 17, 2009 at 7:15 AM, Junio C Hamano wrote: > Christian Couder writes: > >> From: Stephan Beyer >> >> 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] []", >> +     "git reset [--mixed | --soft | --hard | --merge | --merge-safe] [-q] []", >>       "git reset [--mixed] [--] ...", >>       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.