On 01/08, Thomas Gummerer wrote:
> On 01/08, Duy Nguyen wrote:
> > On Mon, Jan 8, 2018 at 5:30 AM, Thomas Gummerer <t.gummerer@xxxxxxxxx> wrote:
> > > @@ -1896,16 +1895,17 @@ 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 = xstrfmt("%s/sharedindex.%s", gitdir, base_sha1_hex);
> >
> > Personally I prefer the repo_git_path() from v2 (sorry I was away and
> > could not comment anything).
>
> It felt slightly nicer to me as well, but it threw up a bunch of
> questions about how worktrees will fit together with struct
> repository. As I don't feel very strongly about this either way I
> decided to go with what Brandon suggested as an alternative, which
> allows us to defer that decision. I'd be happy to revert this to the
> way I had it in v2, but I don't want to drag the discussion on too
> long either, as this series does fix some real regressions.
>
> > The thing is, git_path() and friends
> > could do some path translation underneath to support multiple
> > worktrees. Think of the given path here as a "virtual path" that may
> > be translated to something else, not exactly <git_dir> + "/" +
> > "sharedindex.%s". But in practice, we're not breaking the relationship
> > between $GIT_DIR/index and $GIT_DIR/sharedindex.* any time soon, doing
> > manual path transformation here is fine.
> >
> > What about the other git_path() in this file? With patch applied I still get
> >
> > > ~/w/git/temp $ git grep git_path read-cache.c
> > read-cache.c: shared_index_path = git_path("%s", de->d_name);
> > read-cache.c: temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
> > read-cache.c: git_path("sharedindex.%s",
> > sha1_to_hex(si->base->sha1)));
> > read-cache.c: const char *shared_index = git_path("sharedindex.%s",
>
> Good point, I hadn't looked at these, I only looked at the current
> test failures. I'm going to be away for the rest of the week, but
> I'll have a look at them when I come back.
>
> > I suppose submodule has not triggered any of these code paths yet. Not
> > sure if we should deal with them now or wait until later.
Having had a look at these now, they are all in the write_locked_index
codepath. We should probably have something like
'repo_write_locked_index()' for this. But that probably requires a
bit more work/discussion to see what this should look like. I'd
rather keep this patch series focused on the current breakages, and
deal with that in a separate patch series.
While looking at this, I did find another breakage in the split index
code, which I'll send as 4/3.
> > Perhaps if we add a "struct repository *" pointer inside index_state,
> > we could retrieve back the_repository (or others) and call
> > repo_git_path() everywhere without changing index api too much. I
> > don't know. I like the 'struct repository' concept but couldn't
> > follow its development so I don't if this is what it should become.
>
> Interesting. I didn't follow the development of struct repository
> too closely either, so I'm not sure. Brandon might have more of an
> opinion on that? :)
>
> > --
> > Duy
