login
Header Space

 
 

Re: git pull opinion

Score:
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
To: Aghiles <aghilesk@...>
Cc: Bill Lear <rael@...>, Junio C Hamano <gitster@...>, <git@...>
Date: Tuesday, November 6, 2007 - 12:36 pm

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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
git pull opinion, Aghiles, (Mon Nov 5, 5:52 pm)
Re: git pull opinion, Junio C Hamano, (Mon Nov 5, 7:33 pm)
Re: git pull opinion, Aghiles, (Tue Nov 6, 12:04 am)
Re: git pull opinion, Steven Grimm, (Mon Nov 5, 8:37 pm)
Re: git pull opinion, Bill Lear, (Mon Nov 5, 8:36 pm)
Re: git pull opinion, Andreas Ericsson, (Mon Nov 5, 8:54 pm)
Re: git pull opinion, Johannes Schindelin, (Mon Nov 5, 9:16 pm)
Re: git pull opinion, Andreas Ericsson, (Tue Nov 6, 4:59 am)
Re: git pull opinion, Johannes Schindelin, (Tue Nov 6, 8:05 am)
Re: git pull opinion, Andreas Ericsson, (Tue Nov 6, 8:08 am)
Re: git pull opinion, Aghiles, (Tue Nov 6, 2:30 am)
Re: git pull opinion, Linus Torvalds, (Tue Nov 6, 12:36 pm)
Re: git pull opinion, Aghiles, (Wed Nov 7, 5:25 pm)
Re: git pull opinion, Linus Torvalds, (Fri Nov 9, 8:36 pm)
Re: git pull opinion, Johannes Schindelin, (Thu Nov 8, 11:27 am)
Re: git pull opinion, Alex Riesen, (Tue Nov 6, 3:40 am)
Re: git pull opinion, Pierre Habouzit, (Mon Nov 5, 8:46 pm)
Re: git pull opinion, Alex Riesen, (Tue Nov 6, 3:38 am)
Re: git pull opinion, Pierre Habouzit, (Tue Nov 6, 4:31 am)
Re: git pull opinion, Pascal Obry, (Tue Nov 6, 2:07 pm)
Re: git pull opinion, Uwe , (Wed Nov 7, 3:06 am)
Re: git pull opinion, Pascal Obry, (Wed Nov 7, 3:40 am)
Re: git pull opinion, Miklos Vajna, (Mon Nov 5, 7:40 pm)
Re: git pull opinion, Aghiles, (Tue Nov 6, 12:16 am)
Re: git pull opinion, Benoit Sigoure, (Tue Nov 6, 1:29 am)
Re: git pull opinion, Aghiles, (Tue Nov 6, 3:45 am)
Re: git pull opinion, Pierre Habouzit, (Tue Nov 6, 4:51 am)
[PATCH] Mark 'git stash [message...]' as deprecated, Brian Downing, (Tue Nov 6, 8:26 pm)
Re: [PATCH] Mark 'git stash [message...]' as deprecated, Pierre Habouzit, (Wed Nov 7, 4:23 am)
Re: [PATCH] Mark 'git stash [message...]' as deprecated, Junio C Hamano, (Wed Nov 7, 4:02 am)
Re: [PATCH] Mark 'git stash [message...]' as deprecated, Johannes Sixt, (Wed Nov 7, 4:00 am)
Re: [PATCH] Mark 'git stash [message...]' as deprecated, Wincent Colaiuta, (Wed Nov 7, 4:12 am)
Re: git pull opinion, Ralf Wildenhues, (Tue Nov 6, 3:34 am)
Re: git pull opinion, Johannes Schindelin, (Tue Nov 6, 7:59 am)
Re: git pull opinion, Ralf Wildenhues, (Tue Nov 6, 4:22 pm)
Re: git pull opinion, Alex Riesen, (Mon Nov 5, 6:49 pm)
speck-geostationary