Re: [BUG] - git-read-tree segfaults

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 12, 2009, 05:57 UTC
Message-ID: <7vtz5zjnai.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <alpine.DEB.1.00.0903111300330.10279@pacific.mpi-cbg.de>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 24 quoted lines

> On Wed, 11 Mar 2009, Jiri Olsa wrote:
>
>> On Tue, Mar 10, 2009 at 9:21 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>
>> > On Tue, 10 Mar 2009, Jiri Olsa wrote:
>> >
>> >> mb=$($GIT merge-base HEAD yyy)
>> >> $GIT read-tree $mb HEAD yyy
>> >
>> > While I agree that it is a bad thing for Git to segfault, I think this 
>> > here is a pilot error.  You try to read 3 trees at the same time, but 
>> > not perform a merge.  IMHO you want to add -m at least.
>> 
>> agreed, I've already said I executed it like this by an accident...
>
> Hey, you did the right thing!  And you even provided a script to recreate, 
> so that it was really easy to see what is happening.
>
>> it was easy to recreate so I shared... I'm not saying it is a show 
>> stopper :)
>
> Well, Git should not crash.  But read-tree is real low-level, so I am 
> torn.  OTOH, something like this may be the correct thing to do:

That's a bogus "fix".

"git read-tree" without "-m" is supposed to behave as an overlay of the given trees. Try it with any git older than 1.5.5 and you should see one entry for xxx and yyy each from Jiri's example. Somewhere between 1.5.4 and 1.5.5 we seem to have broken it.

Having said that, I do not think "read-tree A B C" to overlay these trees has never worked reliably. For one thing, I do not think the code never tried to avoid D/F conflicts in the index, and because of that, you can end up with a bogus index that looks like this:

	$ git ls-files -s
        100644 58b2ca10b6b032c114cb934c012c3743e34e0e7a 0	xxx
        100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	xxx/zzz
        100644 363a9e8d3b0b423eab41fd12ebed489004ab3c2e 0	yyy

and trying to write it out as a tree object will produce an even more bogus result.

I think the attached patch fixes the segfault, and also fixes the longstanding lack of D/F conflict detection, but it needs a bit of commentary.

The first hunk fixes the D/F conflict issue. After reading three trees that has (xxx), (xxx, yyy) and (xxx/zzz, yyy) in this order, the resulting index should look like this with this patch, instead of the broken index depicted above:

	$ git ls-files -s
        100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	xxx/zzz
        100644 363a9e8d3b0b423eab41fd12ebed489004ab3c2e 0	yyy

I suspect that the reason add_entry() passed SKIP_DFCHECK was to work around an old bug in add_index_entry() that considered it a D/F conflict if you have a file D at stage N and a file D/F at stage M when N and M are different. I think such a bug has been fixed long time ago, and there is no reason for such a workaround. Besides, OK_TO_REPLACE only makes sense when you do check D/F conflict ("replace" in the name of the flag means "If you want to add 'xxx/zzz' when the index has 'xxx', it is Ok to drop the existing 'xxx' in order to add your 'xxx/zzz''); it makes no sense to give it if you are giving SKIP_DFCHECK at the same time.

The second hunk removes a noop increment of n with o->merge (at this point we know o->merge is zero) and then makes sure we only send an existing entry taken from the tree to add_entry().

A NULL src[i] entry is obviously a missing entry from i-th tree, and an entry that is o->df_conflict_entry is just a stand-in phantom entry the unpack machinery uses when i-th tree has "xxx/zzz" (hence it cannot have "xxx") and the unpacker is looking at path "xxx". In either case, i-th tree does not have an entry "xxx" and we should skip add_entry() for such a src[i].

 unpack-trees.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index e547282..5820ce4 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -49,7 +49,7 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 	memcpy(new, ce, size);
 	new->next = NULL;
 	new->ce_flags = (new->ce_flags & ~clear) | set;
-	add_index_entry(&o->result, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|ADD_CACHE_SKIP_DFCHECK);
+	add_index_entry(&o->result, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);
 }
 
 /* Unlink the last component and attempt to remove leading
@@ -286,9 +286,9 @@ static int unpack_nondirectories(int n, unsigned long mask,
 	if (o->merge)
 		return call_unpack_fn(src, o);
 
-	n += o->merge;
 	for (i = 0; i < n; i++)
-		add_entry(o, src[i], 0, 0);
+		if (src[i] && src[i] != o->df_conflict_entry)
+			add_entry(o, src[i], 0, 0);
 	return 0;
 }