On 12/11, Brandon Williams wrote:
> 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.
Me neither, I just remember to run the split index tests every once in
a while, which is also why it's taken so long to catch this.
Hopefully after we fixed this we can get travis to run the tests,
which should help :)
> >
> > Signed-off-by: Thomas Gummerer <t.gumme...@gmail.com>
> > ---
> > 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'?
I think that makes sense the only way this function used to work with
split index turned on is if it's called from the main repository, so
just passing through 'the_repository' would have the function behave
the exact same way as before.
> > +
> > 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
