git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Jiri Olsa <olsajiri@gmail.com>, git@vger.kernel.org
Subject: Re: [BUG] - git-read-tree segfaults
Date: Wed, 11 Mar 2009 22:57:41 -0700	[thread overview]
Message-ID: <7vtz5zjnai.fsf@gitster.siamese.dyndns.org> (raw)
In-Reply-To: <alpine.DEB.1.00.0903111300330.10279@pacific.mpi-cbg.de> (Johannes Schindelin's message of "Wed, 11 Mar 2009 13:04:15 +0100 (CET)")

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

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

  reply	other threads:[~2009-03-12  5:59 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-03-10 19:34 [BUG] - git-read-tree segfaults Jiri Olsa
2009-03-10 19:50 ` Johannes Schindelin
2009-03-10 20:21 ` Johannes Schindelin
2009-03-11  7:59   ` Jiri Olsa
2009-03-11 12:04     ` Johannes Schindelin
2009-03-12  5:57       ` Junio C Hamano [this message]
2009-03-12  7:01         ` Junio C Hamano
2009-03-12  7:29           ` [PATCH] read-tree A B C: do not create a bogus index and do not segfault Junio C Hamano
2009-03-12 14:46             ` Daniel Barkalow
2009-03-12 16:34               ` Linus Torvalds
2009-03-14  6:41                 ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7vtz5zjnai.fsf@gitster.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=olsajiri@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).