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

Christian Couder [off-list ref] writes:

From: Junio C Hamano <redacted>

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.  

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