Re: [PATCH 0/2] Reorganize read-tree

From: Daniel Barkalow <barkalow@iabervon.org>
Date: Sep 5, 2005, 02:55 UTC
Message-ID: <Pine.LNX.4.63.0509042238420.23242@iabervon.org>
In-Reply-To: <7virxg6zj3.fsf@assigned-by-dhcp.cox.net>

On Sun, 4 Sep 2005, Junio C Hamano wrote:

Show 14 quoted lines

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > I got mostly done with this before Linus mentioned the possibility of
> > having multiple index entries in the same stage for a single path. I
> > finished it anyway, but I'm not sure that we won't want to know which of
> > the common ancestors contributed which, and, if some of them don't have a
> > path, we wouldn't be able to tell. The other advantages I see to this
> > approach are:
> 
> I've finished reading your patch, after beating it reasonably
> heavily by feeding combinations of nonsense trees to make sure
> it produces the same result as the original implementation.  I
> have not found any regression from the read-tree in "master"
> branch, after you fixed the path ordering issues.

Good.

Show 6 quoted lines

> > There are various potential refinements, plus removing a bunch of memory
> > leaks, still to do, but I think this is sufficiently close to review.
> 
> I am not so worried about the leaks right now; they are
> something that could be fixed before it hits the "master"
> branch.

Right.

Show 12 quoted lines

> I like your approach of reading the input trees, along with the
> existing index contents, and re-populating the index one path at
> a time.  It probably is more readable.
> 
> I further think that you can get the best of both worlds, by
> inventing a convention that mode=0 entry means 'this path does
> not exist in this tree'. This would allow you to have multiple
> entries at the same stage and still tell which one came from
> which tree.  Instead of calling fn in unpack_trees(), you could
> make it only unpack the tree into the index, and then after
> unpacking is done, call fn() repeatedly to resolve the resulting
> index.

I think that almost all of the benefit actually comes from calling fn() in unpack_trees() and not putting anything in the index before merging. Without that, you need the complex index management and the complicated search for DF conflicts. The main point of not reading everything into the index before calling fn() on stuff is that the index is actually really difficult to deal with in this situation (because you are simultaneously moving through it, removing and modifying entries, and searching it for conflicts). The improvement in readability comes from not doing this.

	-Daniel
*This .sig left intentionally blank*