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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 12, 2009, 07:01 UTC
Message-ID: <7vfxhjjkcm.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <7vtz5zjnai.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 9 quoted lines

> 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 ancient D/F conflict detection bug in add_cache_entry() I had in mind was fixed by b155725 ([PATCH] Fix oversimplified optimization for add_cache_entry()., 2005-06-25). The use of SKIP_DFCHECK turns out to have nothing to do with working that ancient bug around.

The real culprit seems to be 34110cd (Make 'unpack_trees()' have a separate source and destination index, 2008-03-06).

Before that commit:

 * merge_entry() that records the final tree-level 3-way merge decision
   used to pass OK_TO_REPLACE without SKIP_DFCHECK.

 * unpack_nondirectories() codepath for non-merge multi-tree read-tree
   (the one under discussion in this thread) used to pass SKIP_DFCHECK but
   did not pass OK_TO_REPLACE.

The fact that even merge_entry() side passes SKIP_DFCHECK these days does not appear to be a workaround for an old bug in D/F conflict detection code after all; it simply is a bug in the refactoring done with the said commit.

The unpack_nondirectories() codepath passed SKIP_DFCHECK from ee6566e (Rewrite read-tree, 2005-09-05), which is the very original implementation of the modern "read-tree A B C" code. The ancient bug in the D/F conflict detection code was killed way before that commit, and SKIP_DFCHECK in the commit is not a workaround either; it also simply is a bug.

	Side note: I was somewhat surprised that "make test" of that old
	commit dates from September 2005 runs _much_ faster than the test
	suite we have these days.

The only sane use of SKIP_DFCHECK is when the caller knows the addition is not introducing D/F conflict in the index to avoid the overhead.