Re: [RFC/PATCH 1/5] reset: make "reset --merge" discard work tree changes on unmerged entries

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 2, 2010, 19:46 UTC
Message-ID: <7vzl4w8f2y.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20100102053934.30066.95746.chriscool@tuxfamily.org>

Christian Couder <chriscool@tuxfamily.org> writes:

Show 13 quoted lines

> From: Junio C Hamano <gitster@pobox.com>
>
> Commit 9e8ecea (Add 'merge' mode to 'git reset', 2008-12-01) disallowed
> "git reset --merge" when there was unmerged entries. It acknowledged
> that this is not the best possible behavior, and that it would be better
> if unmerged entries were reset as if --hard (instead of --merge) has
> been used.
>
> Recently another commit (reset: use "unpack_trees()" directly instead of
> "git read-tree", 2009-12-30) changed the behavior of --merge to accept
> resetting unmerged entries if they are reset to a different state than
> HEAD, but it did not reset the changes in the work tree. So the behavior
> was kind of improved, but it was not yet as if --hard has been used.

It would be more honest if we said something like:

	It changed a safer "I can't do as asked, please do it by hand"
	into a more dangerous "I pretend that I did so but I didn't do the
	full job; you need to fix up the result but I am not telling you
	that you have to", which is a lot worse.

here instead (that is one reason why I said my fix-up was "squashable").

But that is a minor issue.

I have been thinking about two issues on this --merge change.

Show 6 quoted lines

>  - Updates merged_entry() and deleted_entry() so that they pay attention
>    to cache entries with null sha1 (note that we _might_ want to use a
>    new in-core ce->ce_flags instead of using the null-sha1 hack).  They
>    are previously unmerged entries, and the files in the work tree that
>    correspond to them are resetted away by oneway_merge() to the version
>    from the tree we are resetting to.

One is the use of ce_flags instead of relying on the 20-byte comparison I said above, for both performance (minor) and future maintainability (much bigger) concern. I have a feeling that we will regret later that we used the null_sha1 trick here, when we want to express another "special" kind of cache entry in unrelated situations. The use of null_sha1 hack was expedient but I fell victim of the same mentality of declaring that this is the _last_ such kind of special index entry and closing the door to others who want to extend the system later with different kind of special cache entry, which I often complain about myself to patches from other people.

Another is that it _might_ make sense to use two-tree form of read-tree machinery (but using a different version of unpack-trees.c::twoway_merge() function), instead of the one-tree form of "we don't bother checking if the index is consistent with HEAD and assume it is, and jump to the target."

"git reset --merge $there" is about the situation where you started a "mergy" operation (e.g. "git merge", "git am -3", "git rebase", ...) while you had unrelated local changes in the work tree, and you want to go back to the state before that operation $there (which is HEAD if the mergy operation is "merge", but is different from HEAD if it was "am -3" and you have successfully applied a patch or more already). Linus may know and won't use "reset --merge" in a situation where it is not suitable, but not everybody is Linus. Even though "reset --merge" may "correctly" work with respect to the table you added to "git reset" documentation, it would do something that may not "make sense" from the end-user's point of view when used in a situation that it wasn't designed for. Using the two-tree form allows the machinery to inspect the difference between HEAD and the index, and detect cases where "reset --merge" was attempted when it shouldn't. For example, if stage #2 of an unmerged path does not match HEAD, we know there is something wrong.

This latter issue is much bigger and needs a lot more thought, and I don't think it should block the series from going forward at all. But I think it is worth keeping in the back of our heads.