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

From: Junio C Hamano <junkio@cox.net>
Date: Sep 5, 2005, 02:04 UTC
Message-ID: <7virxg6zj3.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.63.0508302317380.23242@iabervon.org>

Daniel Barkalow <barkalow@iabervon.org> writes:

Show 6 quoted lines

> 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.

> 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.

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. Of course the semantics of merge_fn_t needs to change but if you feed N trees, the caller of a merge_fn_t function needs to pick up the first N entries (because you use mode=0 entry to mean 'missing from this tree', each path will always have N entries) and feed them to the merge function in one call, then pick up the next N entries and feed them in the next call, and so on. I think that would simplify that part of the code even further.

So if you are making an octopus of 4 trees (one being our current branch) using 2 merge bases, your intermediate index would look like:

    mode   SHA1   stage path
    100644 XXXXX  0     foo	from original index
    000000 0{40}  1     foo	merge base #1 did not have foo
    100644 ZZZZZ  1     foo	merge base #2 has it
    100644 XXXXX  2     foo	our current head
    100644 ZZZZZ  3     foo	other head #1 being merged into us
    100644 YYYYY  3     foo	other head #2 being merged into us
    100644 ZZZZZ  3     foo	other head #3 being merged into us

We cannot write something like this out without breaking backward compatibility, but I personally think this breakage is OK, because what is being broken is the index file format, not tree object format, and the index file is by definition local to a repository. IOW, it is not too much to ask people not to use old tools to read new index file they created using new tools.