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

Johannes Schindelin [off-list ref] writes:

On Wed, 11 Mar 2009, Jiri Olsa wrote:

quoted

On Tue, Mar 10, 2009 at 9:21 PM, Johannes Schindelin
[off-list ref] wrote:

quoted

On Tue, 10 Mar 2009, Jiri Olsa wrote:

quoted

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.

quoted

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;
 }