Re: [PATCH 4/9] repository: add repo reference to index_state

>
> On 1/20/2021 12:46 PM, Elijah Newren wrote:
> > On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>
> >> From: Derrick Stolee <dstolee@microsoft.com>
> >>
> >> It will be helpful to add behavior to index opertations that might
> >
> > s/opertations/operations/
>
> Thanks.
>
> >> trigger an object lookup. Since each index belongs to a specific
> >> repository, add a 'repo' pointer to struct index_state that allows
> >> access to this repository.
> >>
> >> This will prevent future changes from needing to pass an additional
> >> 'struct repository *repo' parameter and instead rely only on the 'struct
> >> index_state *istate' parameter.
> >>
> >> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> >> ---
> >>  cache.h      | 1 +
> >>  repository.c | 4 ++++
> >>  2 files changed, 5 insertions(+)
> >>
> >> diff --git a/cache.h b/cache.h
> >> index 71097657489..f9c7a603841 100644
> >> --- a/cache.h
> >> +++ b/cache.h
> >> @@ -328,6 +328,7 @@ struct index_state {
> >>         struct ewah_bitmap *fsmonitor_dirty;
> >>         struct mem_pool *ce_mem_pool;
> >>         struct progress *progress;
> >> +       struct repository *repo;
> >>  };
> >>
> >>  /* Name hashing */
> >> diff --git a/repository.c b/repository.c
> >> index a4174ddb062..67a4c1da2d9 100644
> >> --- a/repository.c
> >> +++ b/repository.c
> >> @@ -264,6 +264,10 @@ int repo_read_index(struct repository *repo)
> >>         if (!repo->index)
> >>                 repo->index = xcalloc(1, sizeof(*repo->index));
> >>
> >> +       /* Complete the double-reference */
> >> +       if (!repo->index->repo)
> >> +               repo->index->repo = repo;
> >> +
> >>         return read_index_from(repo->index, repo->index_file, repo->gitdir);
> >>  }
> >>
> >> --
> >> gitgitgadget
> >
> > Since we have repo->index and we have index->repo, which are intended
> > to be circular...what if they aren't?  Do we want or need to add
> > assertions anywhere that repo == repo->index->repo or that index ==
> > index->repo->index ?
>
> Here, we are pairing them together and the loop is complete. I don't
> view that as a permanent thing. This only initializes istate->repo
> when we are parsing an index from a file, but not when we create one
> in memory.
>
> I imagine it will be likely in some cases to have multiple index_state
> instances for a single repository. However, having the pointer "this
> index belongs to this repository" seems helpful (to me).
>
> > My initial implementations of --remerge-diff[1] played around with
> > creating a second repo, with a different primary object store but
> > everything else the same.  The index for the two repository objects
> > was thus the same, and thus clearly would have violated this assumed
> > invariant for one of the two repos.  I discarded that initial
> > implementation (which I didn't quite have working) because I
> > discovered tmp-objdir.h and was able to add some
> > tmp_objdir_make_primary() and tmp_objdir_remove_as_primary() functions
> > that merely altered the existing repo's primary object store, but I'm
> > curious if there might be other cases of folks doing stuff that might
> > have weird failures with this new invariant.
>
> This is an interesting concept, and definitely violates my expectations
> that an index belongs to only one repository. I'd need to know more
> about why this was a good design decision before being convinced that
> the relationship should not be many-to-one (index-to-repo).

> > It's entirely possible that --remerge-diff was just so different, and
> > I was so unfamiliar with repo objects (and still kind of am) that I
> > was just doing weird stuff no one has done before, so perhaps no
> > additional checks are needed -- I'm just throwing my gut question out
> > there as food for thought.
> >
> > [1] I have not yet submitted `--remerge-diff` to the list; you haven't
> > missed anything.  I'm waiting for merge-ort to be submitted, reviewed,
> > and merged first.  It's the remerge-diff branch in my fork on GitHub
> > if anyone is curious, though.
>
> I'm interested in what others might say about this idea. I'd be able
> to do most of what I want to do without this patch, but it just gets
> a lot messier. (istate->repo is used in the very next patch in a way
> that would be less clean without it.)