Re: [PATCH v3 1/3] read-cache: fix reading the shared index for other repos

From: Duy Nguyen <pclouds@gmail.com>
Date: Jan 9, 2018, 01:24 UTC
Message-ID: <CACsJy8AkbMdQZq85N_LSvkM+JN4E2mkEUfaHHGTvrNJ4yCPs2Q@mail.gmail.com>
In-Reply-To: <20180108233840.GB156560@google.com>

On Tue, Jan 9, 2018 at 6:38 AM, Brandon Williams <bmwill@google.com> wrote:

Show 22 quoted lines

> On 01/08, Duy Nguyen wrote:
>> On Mon, Jan 8, 2018 at 5:30 AM, Thomas Gummerer <t.gummerer@gmail.com> 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). 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.
>
> My biggest complaint about v2 is that we still don't quite know the best
> way to integrate worktrees and struct repository yet so I was very
> reluctant to start having them interact in the way v2 was using them
> together.

This will be on my todo list (after I have finished with nd/worktree-move which has been on 'pu' for too long). I'm ok with v3 then.

> I'm very much in favor of this version (v3) as each worktree
> can explicitly provide their gitdir to be used to determine where to
> read the shared index file without having to replicate a struct
> repository for each.

Isn't that the end goal though (I vaguely recall this discussion when 'struct repository' was an idea), that we will pass struct repository around [1] rather than relying on global objects.

[1] or in this case struct worktree-something because the index belongs more to a worktree than the object/refs database.

Show 23 quoted lines

>> 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",
>>
>> 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.
>>
>> 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.
>
> I'm not too keen on having an index_state struct contain a back pointer
> to a repository struct.  I do think that we may want to have worktree
> structs contain a back pointer to the struct repository they correspond
> to.  That way there is only one instance of the repository (and
> object-store once that gets integrated) yet multiple worktrees.

Yeah back pointer from worktree struct makes sense.

-- 
Duy