Re: git pull opinion

On Tue, 6 Nov 2007, Aghiles wrote:

Git does merge with a dirty directory too, but refuses to merge if it needs to *change* any individual dirty *files*.

And that actually comes from one of the great strengths of git: in git (unlike just about any other SCM out there) you can - and are indeed expected to - resolve merges sanely in the working tree using normal filesystem accesses (ie your basic normal editors and other tools).

That means that if there is a unresolved merge, you're actually expected to edit things in the same place where they are dirty. Which means that the merge logic doesn't want to mix up your dirty state and whatever merged state, because that is then not sanely resolvable.

Now, I do think that we could relax the rule so that "files that are modified must be clean in the working tree" could instead become "files that actually don't merge _trivially_ must be clean in the working tree". But basically, if it's not a trivial merge, then since it's done in the working tree, the working tree has to be clean (or the merge would overwrite it).

Doing a four-way merge is just going to confuse everybody.

So we *could* probably make unpack-trees.c: treeway_merge() allow this. It's not totally trivial, because it requires that the CE_UPDATE be replaced with something more ("CE_THREEWAY"): instead of just writing the new result, it should do another three-way merge.

So it's within the range of possible, but it's actually pretty subtle. The reason: we cannot (and *must*not*!) actually do the three-way merge early. We need to do the full tree merge in stage 1, and then only if all files are ok can we then check out the new tree. And we currently don't save the merge information at all.

So to do this, we'd need to:

 - remove the "verify_uptodate(old, o); invalidate_ce_path(old);" in 
   "merged_entry()", and actually *leave* the index with all three stages 
   intact, but set CE_UPDATE *and* return success.

 - make check_updates() do the three-way merge of "original index, working 
   tree, new merged state" instead of just doing a "unlink_entry() + 
   checkout_entry()".

It doesn't actually look *hard*, but it's definitely subtle enough that I'd be nervous about doing it. We're probably talking less than 50 lines of actual diffs (this whole code uses good data structures, and we can fairly easily represent the problem, and we already have the ability to do a three-way merge!), but we're talking some really quite core code and stuff that absolutely must not have any chance what-so-ever of ever breaking!

To recap:
 - it's probably a fairly simple change to just two well-defined places 
   (merge_entry() and check_updates())
 - but dang, those two places are critical and absolutely must not be 
   screwed up, and while both of those functions are pretty simple, this 
   is some seriously core functionality.

If somebody wants to do it, I'll happily look over the result and test it out, but it really needs to be really clean and obvious and rock solid. And in the absense of that, I'll take the current safe code that just says: don't confuse the merge and make it any more complex than it needs to be.

		Linus