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

[Thomas Rast <trast@student.ethz.ch>]

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

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