Re: [PATCH v2 1/3] repository: fix repo_read_index with submodules

[Brandon Williams <bmwill@google.com>]

On 12/18, Thomas Gummerer wrote:
> On 12/18, Brandon Williams wrote:
> > On 12/17, Thomas Gummerer wrote:
> > > repo_read_index calls read_index_from, which takes an path argument for
> > > the location of the index file.  For the split index however it relies
> > 
> > > on the current working directory to construct the path using git_path.
> > 
> > This line isn't actually true and should probably be fixed.  git_path
> > doesn't rely on the CWD but rather it relies on the gitdir of the main
> > repository (the_repository).
> 
> Right, let me fix that.  Thanks!
> 
> > > 
> > > repo_read_index calls read_index_from with the full path for the index
> > > file, however it doesn't change the cwd, so when split index mode is
> > > turned on, read_index_from can't find the file for the split index.
> > > 
> > > For example t3007-ls-files-recurse-submodules.sh was broken with
> > > GIT_TEST_SPLIT_INDEX set in 188dce131f ("ls-files: use repository
> > > object", 2017-06-22), and t7814-grep-recurse-submodules.sh was also
> > > broken in a similar manner, probably by introducing struct repository
> > > there, although I didn't track down the exact commit for that.
> > > 
> > > Fix this by introducing a new read_index_for_repo function, which knows
> > > about the correct paths for the submodules.
> > > 
> > > The alternative would have been to make the callers pass in the base
> > > path for the split index, however that ended up being more complicated,
> > > and I think we want to converge towards using struct repository for
> > > things like these anyway.
> > > 
> > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > > ---
> > >  cache.h      |  1 +
> > >  read-cache.c | 16 ++++++++++++++--
> > >  repository.c |  2 +-
> > >  3 files changed, 16 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/cache.h b/cache.h
> > > index cb5db7bf83..d42bea1ef7 100644
> > > --- a/cache.h
> > > +++ b/cache.h
> > > @@ -614,6 +614,7 @@ extern int read_index_preload(struct index_state *, const struct pathspec *paths
> > >  extern int do_read_index(struct index_state *istate, const char *path,
> > >  			 int must_exist); /* for testting only! */
> > >  extern int read_index_from(struct index_state *, const char *path);
> > > +extern int read_index_for_repo(const struct repository *);
> > >  extern int is_index_unborn(struct index_state *);
> > >  extern int read_index_unmerged(struct index_state *);
> > >  
> > > diff --git a/read-cache.c b/read-cache.c
> > > index 2eb81a66b9..70357febdc 100644
> > > --- a/read-cache.c
> > > +++ b/read-cache.c
> > > @@ -20,6 +20,7 @@
> > >  #include "split-index.h"
> > >  #include "utf8.h"
> > >  #include "fsmonitor.h"
> > > +#include "repository.h"
> > >  
> > >  /* Mask for the name length in ce_flags in the on-disk index */
> > >  
> > > @@ -1871,7 +1872,8 @@ static void freshen_shared_index(char *base_sha1_hex, int warn)
> > >  	free(shared_index);
> > >  }
> > >  
> > > -int read_index_from(struct index_state *istate, const char *path)
> > > +static int do_read_index_from(struct index_state *istate, const char *path,
> > > +			      const struct repository *repo)
> > >  {
> > >  	struct split_index *split_index;
> > >  	int ret;
> > > @@ -1896,7 +1898,7 @@ int read_index_from(struct index_state *istate, const char *path)
> > >  		split_index->base = xcalloc(1, sizeof(*split_index->base));
> > >  
> > >  	base_sha1_hex = sha1_to_hex(split_index->base_sha1);
> > > -	base_path = git_path("sharedindex.%s", base_sha1_hex);
> > > +	base_path = repo_git_path(repo, "sharedindex.%s", base_sha1_hex);
> > >  	ret = do_read_index(split_index->base, base_path, 1);
> > >  	if (hashcmp(split_index->base_sha1, split_index->base->sha1))
> > >  		die("broken index, expect %s in %s, got %s",
> > > @@ -1909,6 +1911,16 @@ int read_index_from(struct index_state *istate, const char *path)
> > >  	return ret;
> > >  }
> > >  
> > > +int read_index_for_repo(const struct repository *repo)
> > > +{
> > > +	return do_read_index_from(repo->index, repo->index_file, repo);
> > > +}
> > 
> > > +
> > > +int read_index_from(struct index_state *istate, const char *path)
> > > +{
> > > +	return do_read_index_from(istate, path, the_repository);
> > > +}
> > 
> > This looks fine, though I wonder what the point of passing in the index
> > file even was since we end just ended up reading the 'sharedindex' file based
> > on the git path. I'm just curious about how this function evolved.
> 
> There are some callsites that are using an index different form
> $gitdir/index, or even GIT_INDEX_FILE.  e.g. see builtin/am.c [*1*],
> which uses it's own 'patch-merge-index' in the am state directory for
> its internal operations.
> 
> The split index mode was later bolted on, and the sharedindex.xxxx
> would always go in $gitdir for the repository.  Others probably know
> quite a bit more about this, while I'm always interested in index
> related things as that's how I got started with the git project, I
> couldn't follow all the conversations that were going on there.
> 
> *1*: https://github.com/gitster/git/blob/52015aaf9d19c97b52c47c7046058e6d029ff856/builtin/am.c#L1844

Thanks for the explanation and pointer! :) 

> 
> > > +
> > >  int is_index_unborn(struct index_state *istate)
> > >  {
> > >  	return (!istate->cache_nr && !istate->timestamp.sec);
> > > diff --git a/repository.c b/repository.c
> > > index bb2fae5446..928b1f553d 100644
> > > --- a/repository.c
> > > +++ b/repository.c
> > > @@ -229,5 +229,5 @@ int repo_read_index(struct repository *repo)
> > >  	if (!repo->index)
> > >  		repo->index = xcalloc(1, sizeof(*repo->index));
> > >  
> > > -	return read_index_from(repo->index, repo->index_file);
> > > +	return read_index_for_repo(repo);
> > >  }
> > > -- 
> > > 2.15.1.620.gb9897f4670
> > > 
> > 
> > -- 
> > Brandon Williams

-- 
Brandon Williams