Re: [PATCH 3/3] read-tree -m 3-way: handle more trivial merges internally

On Thu, 9 Jun 2005, Junio C Hamano wrote:

This patch teaches "read-tree -m O A B" that some more trivial
cases can be handled internally.

No, I think this is quite possibly wrong for several reasons.

For one, it makes the rest of the system unaware of the deleted files, so 
nothing ever deletes them from the working directory.

And that's not entirely trivial to fix either, since the obvious fix 
(which is to just do a

	if (update)
		unlink(b->name);

or something like that) is wrong. It's wrong because we must not do the
update until the very end, when we've either merged all entries or we've
failed on an entry that couldn't be merged (that's why I did the extra
CE_UPDATE flag, instead of updating as we go along).

Now, you could fix that by creating a separate list of files to be 
deleted (so this is not fundamental, it's just more complicated than the 
trivial case), but that doesn't help, because there's _another_ reason why 
read-tree shouldn't handle these cases.

Namely that read-tree doesn't have a frigging clue about renames, and 
shouldn't have.

But a real merge program _could_ have a frigging clue, and might notice 
patterns like

 - file got modified in one branch, removed in the other
 - a file got added in the other branch
 - "Hey, that added file looks like the removed one!"
 - Let's merge the modifications from the first branch into the move of 
   the second branch!

See? Now, git-read-tree won't handle the first case anyway, but your 
change _does_ make it handle the "file got added" case, which means that 
now the added file is invisible the the "smart merger", and the smart 
merger can't really tell that it was a rename any more.

So our current stupid file-by-file "git-merge-cache" will never do this, 
but that's a limitation of me being less than the intellectual giant I 
wish I was. So I just do the stupid merges. But I _know_ they are stupid, 
and I would like to leave the door open for somebody else to fix up the 
cases I don't handle.

You're basically closing that door.

Now, you can (validly) argue that you could still just look at the
original trees ("git-diff-tree -C $O $M") and grep for copies/movement and
do it by hand _there_ instead of looking at the result of the read-tree, 
and you may well be right. So again, this is not a _fundamental_ problem, 
although it's a bit more fundamental than the first one. 

So if you want to convince me that it's better to do the rename detection
outside of the index file, go wild. Alternatively, you can argue that we
can always undo this later, when once we _do_ have rename and copy
detection and can try to merge things automatically (what _do_ you do if a
file is copied in one branch and modified in the other? Just warn the poor
user, I guess).

So I just need a little convincing that this is a good idea.

		Linus