Re: [PATCH/RFC v2 0/16] Introduce index file format version 5

From: Thomas Rast <trast@student.ethz.ch>
Date: Aug 7, 2012, 22:31 UTC
Message-ID: <87393yz64x.fsf@thomas.inf.ethz.ch>
In-Reply-To: <7vehnjzzfd.fsf@alter.siamese.dyndns.org>

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

Show 19 quoted lines

>> Then of course you need to split the second patch into several logical
>> patches again. We can drop _v5 suffix in read-cache-v5.c (I haven't
>> done that). When we add partial read/write for v5, we can add more
>> func pointers to index_ops and implement them in v2 (probably as no-op
>> or assertion)
>
> The index_ops abstraction is a right way to go, and I like it, but I
> think the split illustrated in this patch might turn out to be at
> wrong levels (and it is OK, as I understand this is a illustration
> of concept patch).
>
> For example, add_to_index() interface may be a good candidate to
> have in index_ops.  Because your in-core index may not be holding
> everything in a flat array, "find the location in the flat array the
> entry would sit, replace the existing one if there is any, otherwise
> insert" cannot be a generic way to add a new entry.  If you make the
> whole thing an abstract API entry point, a sparse implementation of
> the in-core index could still implement it without bringing the
> untouched and irrelevant parts of the index to core.

[...]

Show 6 quoted lines

> I wish that the development of this topic was done more in a
> top-down direction, instead of bottom-up, so that it identified the
> necessary access patterns to the in-core index early and come up
> with a good set of abstract API first, and then only after that is
> done, came up with in-core and on-disk format to support the
> necessary operations.

I like the general idea, too, but I think there is a long way ahead, and we shouldn't hold up v5 on this.

Thomas and me -- it was mostly my bad idea -- spent some time going through all the loops that iterate over the index. You can get some taste of it with 'git grep ce_stage', mostly because many of them either skip unmerged entries or specifically look for them. There are subtle differences between the loops on many points: what do they do when they hit an unmerged entry? Or a CE_REMOVED or CE_VALID one?

I gave up after treating half of them and horribly breaking the test suite. I suppose eventually we will have to classify these loops by properties like how they treat unmerged entries, and replace them by some clever for_each_cache_entry macro.

It would open some interesting possibilities. For example, for v5 it would be far better if conflicted and resolve-undo entries were a property of the normal index entry, instead of something that so happens to be consecutive entries and in a completely different place, respectively.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch