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

From: Brandon Williams <[email-masked]>
Date: Mon, 11 Dec 2017 10:54:06 -0800

On 12/10, 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.
> 
> 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.

Thanks for catching this, I'm not a user of split index myself which is
why I unfortunately overlooked this.  Definitely a good change.  I
really only have one nit below.

> 
> Signed-off-by: Thomas Gummerer <[email-masked]>
> ---
>  cache.h      |  1 +
>  read-cache.c | 19 +++++++++++++++++--
>  repository.c |  2 +-
>  3 files changed, 19 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..4d5c4ad79b 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,10 @@ 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);
> +	if (repo)
> +		base_path = repo_git_path(repo, "sharedindex.%s", base_sha1_hex);
> +	else
> +		base_path = git_path("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 +1914,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, NULL);
> +}

Instead of passing NULL and having to special case it in
'do_read_index_from()', how about we pass in 'the_repository'?

> +
>  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.504.g5279b80103
> 

-- 
Brandon Williams