On Fri, Jul 26, 2019 at 1:11 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Elijah Newren <newren@xxxxxxxxx> writes:
>
> > diff --git a/cache-tree.c b/cache-tree.c
> > index 706ffcf188..99144b1704 100644
> > --- a/cache-tree.c
> > +++ b/cache-tree.c
> > @@ -613,14 +613,19 @@ int write_index_as_tree(struct object_id *oid, struct index_state *index_state,
> > int entries, was_valid;
> > struct lock_file lock_file = LOCK_INIT;
> > int ret = 0;
> > + int access_disk = !(flags & WRITE_TREE_FROM_MEMORY);
>
> Shouldn't we go one step futher and make the bulk of in-core index
> processing into a new helper function, while making
> write_index_as_tree() a thin-wrapper around it, i.e.
>
> write_index_as_tree()
> {
> lock the index for update;
> read the on-disk index;
> call that new helper function to write a tree;
> update the on-disk index;
> }
>
> and reuse the helper from
> merge-recursive.c::write_tree_from_memory() while keeping the call
> to the latter in merge_trees_internal()? Wouldn't that approach
> let you do this without adding an extra flag bit?
I thought about that briefly yesterday, but the fact that the
write_locked_index() call only happens if !cache_tree_fully_valid()
meant refactoring slightly more to get the helper to also return that
boolean value, and since I was a little unsure of myself with
cache-tree stuff in general I wanted to propose what looked like the
minimally invasive changes first (by which I mean smallest patch).
I'll take a closer look at this path.
> Also, there used to be a check to ensure that the in-core index fed
> to write_tree_from_memory() is fully merged and otherwise dump the
> unmerged entries with BUG(). Can we simply lose it? I know you
> return with "error building trees" from merge_trees_internal() but
> it does not BUG().
I thought about that yesterday and decided, "Nah, it's a developer
only debug message used during development. I've _never_ seen anyone
report those messages, and I only saw them when I was making bad
changes during development when I first started a decade ago." Then
Emily posts a report today showing that exact BUG message being hit
with git-2.22.0, and how she wouldn't have been able to get all that
extra information in her analysis without that bit of information
being reported.
So, yeah, I need to put something from those BUG() messages back in;
they clearly helped with that issue, and might help again in the
future.
