From: Thomas Gummerer <t.gummerer@gmail.com>
To: Brandon Williams <bmwill@google.com>
Cc: git@vger.kernel.org, "Lars Schneider" <larsxschneider@gmail.com>,
"Jeff King" <peff@peff.net>, "Junio C Hamano" <gitster@pobox.com>,
"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
"SZEDER Gábor" <szeder.dev@gmail.com>
Subject: Re: [PATCH v2 2/3] prune: fix pruning with multiple worktrees and split index
Date: Wed, 3 Jan 2018 22:18:49 +0000 [thread overview]
Message-ID: <20180103221849.GC2641@hank> (raw)
In-Reply-To: <20171218181913.GB147973@google.com>
[sorry for the late reply. I was on Christmas holidays until today
and am still catching up on the mailing list. It will probably take
me untill the weekend to send a re-roll]
On 12/18, Brandon Williams wrote:
> On 12/17, Thomas Gummerer wrote:
> > be489d02d2 ("revision.c: --indexed-objects add objects from all
> > worktrees", 2017-08-23) made sure that pruning takes objects from all
> > worktrees into account.
> >
> > It did that by reading the index of every worktree and adding the
> > necessary index objects to the set of pending objects. The index is
> > read by read_index_from. As mentioned in the previous commit,
> > read_index_from depends on the CWD for the location of the split index,
>
> As I mentioned before this doesn't actually depend on the CWD but
> rather the per-worktree gitdir.
Right, will fix.
> > and add_index_objects_to_pending doesn't set that before using
> > read_index_from.
> >
> > Instead of using read_index_from, use repo_read_index, which is aware of
> > the proper paths for the worktree.
> >
> > This fixes t5304-prune when ran with GIT_TEST_SPLIT_INDEX set.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> > repository.c | 11 +++++++++++
> > repository.h | 2 ++
> > revision.c | 14 +++++++++-----
> > 3 files changed, 22 insertions(+), 5 deletions(-)
> >
> > diff --git a/repository.c b/repository.c
> > index 928b1f553d..3c9bfbd1b8 100644
> > --- a/repository.c
> > +++ b/repository.c
> > @@ -2,6 +2,7 @@
> > #include "repository.h"
> > #include "config.h"
> > #include "submodule-config.h"
> > +#include "worktree.h"
> >
> > /* The main repository */
> > static struct repository the_repo = {
> > @@ -146,6 +147,16 @@ int repo_init(struct repository *repo, const char *gitdir, const char *worktree)
> > return -1;
> > }
> >
> > +/*
> > + * Initialize 'repo' based on the provided worktree
> > + * Return 0 upon success and a non-zero value upon failure.
> > + */
> > +int repo_worktree_init(struct repository *repo, struct worktree *worktree)
> > +{
> > + return repo_init(repo, get_worktree_git_dir(worktree),
> > + worktree->path);
>
> I still feel very unsettled about this and don't think its a good idea.
> get_worktree_git_dir depends implicitly on the global the_repository
> object and I would like to avoid relying on it for an initialization
> function like this.
>
> > +}
> > +
> > /*
> > * Initialize 'submodule' as the submodule given by 'path' in parent repository
> > * 'superproject'.
> > diff --git a/repository.h b/repository.h
> > index 7f5e24a0a2..2adeb05bf4 100644
> > --- a/repository.h
> > +++ b/repository.h
> > @@ -4,6 +4,7 @@
> > struct config_set;
> > struct index_state;
> > struct submodule_cache;
> > +struct worktree;
> >
> > struct repository {
> > /* Environment */
> > @@ -87,6 +88,7 @@ extern struct repository *the_repository;
> > extern void repo_set_gitdir(struct repository *repo, const char *path);
> > extern void repo_set_worktree(struct repository *repo, const char *path);
> > extern int repo_init(struct repository *repo, const char *gitdir, const char *worktree);
> > +extern int repo_worktree_init(struct repository *repo, struct worktree *worktree);
> > extern int repo_submodule_init(struct repository *submodule,
> > struct repository *superproject,
> > const char *path);
> > diff --git a/revision.c b/revision.c
> > index e2e691dd5a..34e1e4b799 100644
> > --- a/revision.c
> > +++ b/revision.c
> > @@ -22,6 +22,7 @@
> > #include "packfile.h"
> > #include "worktree.h"
> > #include "argv-array.h"
> > +#include "repository.h"
> >
> > volatile show_early_output_fn_t show_early_output;
> >
> > @@ -1346,15 +1347,18 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
> > worktrees = get_worktrees(0);
> > for (p = worktrees; *p; p++) {
> > struct worktree *wt = *p;
> > - struct index_state istate = { NULL };
> > + struct repository *repo;
> >
> > + repo = xmalloc(sizeof(struct repository));
> > if (wt->is_current)
> > continue; /* current index already taken care of */
> > + if (repo_worktree_init(repo, wt))
> > + BUG("couldn't initialize repository object from worktree");
> >
> > - if (read_index_from(&istate,
> > - worktree_git_path(wt, "index")) > 0)
>
> Ok, after thinking this through a bit more I think a better approach may
> be to restructure the call to read_index_from to take in both an index
> file as well as the explicit gitdir to use when constructing a path to
> the sharedindex file. That way you can fix this for worktrees and
> submodules without having to pass in a repository object to the logic
> which is reading an index file as well as avoiding needing to init a
> repository object for every worktree.
I still think with eventually we probably only want to read the index
through a repository object instead of reading it to a separate struct
index_state.
But we can probably defer that for now, as this series really just
wants to fix the regressions, and we can think a bit more about how
the repository struct interacts with worktrees.
> So you could rework the first patch to do something like:
Thanks for the below, I'll try the below and see how much churn it
causes adjusting all the callsites, and send a re-roll later this
week.
> diff --git a/read-cache.c b/read-cache.c
> index 2eb81a66b..3a04b5567 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1863,20 +1863,19 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
> * This way, shared index can be removed if they have not been used
> * for some time.
> */
> -static void freshen_shared_index(char *base_sha1_hex, int warn)
> +static void freshen_shared_index(const char *shared_index, int warn)
> {
> - char *shared_index = git_pathdup("sharedindex.%s", base_sha1_hex);
> if (!check_and_freshen_file(shared_index, 1) && warn)
> warning("could not freshen shared index '%s'", shared_index);
> - free(shared_index);
> }
>
> -int read_index_from(struct index_state *istate, const char *path)
> +int read_index_from(struct index_state *istate, const char *path,
> + const char *gitdir)
> {
> struct split_index *split_index;
> int ret;
> char *base_sha1_hex;
> - const char *base_path;
> + char *base_path;
>
> /* istate->initialized covers both .git/index and .git/sharedindex.xxx */
> if (istate->initialized)
> @@ -1896,14 +1895,14 @@ 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);
> 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",
> base_sha1_hex, base_path,
> sha1_to_hex(split_index->base->sha1));
>
> - freshen_shared_index(base_sha1_hex, 0);
> + freshen_shared_index(base_path, 0);
> merge_base_index(istate);
> post_read_index_from(istate);
> + free(base_path);
> return ret;
>
>
> --
> Brandon Williams
next prev parent reply other threads:[~2018-01-03 22:16 UTC|newest]
Thread overview: 95+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-10 21:21 [PATCH 0/3] fixes for split index mode Thomas Gummerer
2017-12-10 21:22 ` [PATCH 1/3] repository: fix repo_read_index with submodules Thomas Gummerer
2017-12-11 18:54 ` Brandon Williams
2017-12-11 20:37 ` Thomas Gummerer
2017-12-10 21:22 ` [PATCH 2/3] prune: fix pruning with multiple worktrees and split index Thomas Gummerer
2017-12-11 19:09 ` Brandon Williams
2017-12-11 21:39 ` Thomas Gummerer
2017-12-10 21:22 ` [PATCH 3/3] travis: run tests with GIT_TEST_SPLIT_INDEX Thomas Gummerer
2017-12-10 23:37 ` Eric Sunshine
2017-12-11 21:09 ` SZEDER Gábor
2017-12-11 21:42 ` Thomas Gummerer
2017-12-12 15:54 ` Lars Schneider
2017-12-12 19:15 ` Junio C Hamano
2017-12-12 20:15 ` Thomas Gummerer
2017-12-12 20:51 ` Junio C Hamano
2017-12-13 23:21 ` Thomas Gummerer
2017-12-13 17:21 ` Lars Schneider
2017-12-13 17:38 ` Junio C Hamano
2017-12-13 17:46 ` Lars Schneider
2017-12-13 23:28 ` Thomas Gummerer
2017-12-17 22:51 ` [PATCH v2 0/3] fixes for split index mode Thomas Gummerer
2017-12-17 22:51 ` [PATCH v2 1/3] repository: fix repo_read_index with submodules Thomas Gummerer
2017-12-18 18:01 ` Brandon Williams
2017-12-18 23:05 ` Thomas Gummerer
2017-12-18 23:05 ` Brandon Williams
2017-12-17 22:51 ` [PATCH v2 2/3] prune: fix pruning with multiple worktrees and split index Thomas Gummerer
2017-12-18 18:19 ` Brandon Williams
2018-01-03 22:18 ` Thomas Gummerer [this message]
2018-01-04 19:12 ` Junio C Hamano
2017-12-17 22:51 ` [PATCH v2 3/3] travis: run tests with GIT_TEST_SPLIT_INDEX Thomas Gummerer
2017-12-18 18:16 ` Lars Schneider
2018-01-04 20:13 ` Thomas Gummerer
2018-01-05 11:03 ` Lars Schneider
2018-01-07 20:02 ` Thomas Gummerer
2018-01-07 22:30 ` [PATCH v3 0/3] fixes for split index mode Thomas Gummerer
2018-01-07 22:30 ` [PATCH v3 1/3] read-cache: fix reading the shared index for other repos Thomas Gummerer
2018-01-08 10:41 ` Duy Nguyen
2018-01-08 22:41 ` Thomas Gummerer
2018-01-13 22:33 ` Thomas Gummerer
2018-01-08 23:38 ` Brandon Williams
2018-01-09 1:24 ` Duy Nguyen
2018-01-16 21:42 ` Brandon Williams
2018-01-17 0:16 ` Duy Nguyen
2018-01-17 0:32 ` Brandon Williams
2018-01-17 18:16 ` Jonathan Nieder
2018-01-18 10:19 ` Duy Nguyen
2018-01-19 21:57 ` Junio C Hamano
2018-01-20 11:58 ` Thomas Gummerer
2018-01-22 6:14 ` Junio C Hamano
2018-01-27 12:18 ` Thomas Gummerer
2018-02-07 22:41 ` Junio C Hamano
2018-01-07 22:30 ` [PATCH v3 2/3] split-index: don't write cache tree with null oid entries Thomas Gummerer
2018-01-07 22:30 ` [PATCH v3 3/3] travis: run tests with GIT_TEST_SPLIT_INDEX Thomas Gummerer
2018-01-13 22:37 ` [PATCH v3 4/3] read-cache: don't try to write index if we can't write shared index Thomas Gummerer
2018-01-14 9:36 ` Duy Nguyen
2018-01-14 10:18 ` [PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index() Nguyễn Thái Ngọc Duy
2018-01-14 10:18 ` [PATCH 2/3] read-cache.c: move tempfile creation/cleanup out of write_shared_index Nguyễn Thái Ngọc Duy
2018-01-14 10:18 ` [PATCH 3/3] read-cache: don't write index twice if we can't write shared index Nguyễn Thái Ngọc Duy
2018-01-18 11:36 ` SZEDER Gábor
2018-01-18 12:47 ` Duy Nguyen
2018-01-18 13:29 ` Jeff King
2018-01-18 13:36 ` Duy Nguyen
2018-01-18 15:00 ` Duy Nguyen
2018-01-18 21:37 ` Jeff King
2018-01-18 22:32 ` SZEDER Gábor
2018-01-19 0:30 ` Duy Nguyen
2018-01-22 13:32 ` [PATCH 0/5] Travis CI: don't run the test suite as root in the 32 bit Linux build SZEDER Gábor
2018-01-22 13:32 ` [PATCH 1/5] travis-ci: use 'set -x' for the commands under 'su' " SZEDER Gábor
2018-01-22 13:32 ` [PATCH 2/5] travis-ci: use 'set -e' in the 32 bit Linux build job SZEDER Gábor
2018-01-23 16:26 ` Jeff King
2018-01-23 16:32 ` Jeff King
2018-01-24 12:12 ` SZEDER Gábor
2018-01-24 15:49 ` Jeff King
2018-01-22 13:32 ` [PATCH 3/5] travis-ci: don't repeat the path of the cache directory SZEDER Gábor
2018-01-23 16:30 ` Jeff King
2018-01-24 13:14 ` SZEDER Gábor
2018-01-22 13:32 ` [PATCH 4/5] travis-ci: don't run the test suite as root in the 32 bit Linux build SZEDER Gábor
2018-01-23 16:43 ` Jeff King
2018-01-24 13:45 ` SZEDER Gábor
2018-01-24 15:56 ` Jeff King
2018-01-24 18:01 ` Jeff King
2018-01-24 19:51 ` Jeff King
2018-01-22 13:32 ` [PATCH 5/5] travis-ci: don't fail if user already exists on 32 bit Linux build job SZEDER Gábor
2018-01-23 16:46 ` Jeff King
2018-01-24 0:32 ` Duy Nguyen
2018-01-24 19:39 ` SZEDER Gábor
2018-01-22 18:27 ` [PATCH 3/3] read-cache: don't write index twice if we can't write shared index SZEDER Gábor
2018-01-22 19:46 ` Eric Sunshine
2018-01-22 22:10 ` SZEDER Gábor
2018-01-24 9:11 ` Duy Nguyen
2018-01-26 22:44 ` Lars Schneider
2018-01-14 14:29 ` [PATCH v3 4/3] read-cache: don't try to write index " Thomas Gummerer
2018-01-18 21:53 ` [PATCH v3 0/3] fixes for split index mode Thomas Gummerer
2018-01-19 18:34 ` Junio C Hamano
2018-01-19 21:11 ` Thomas Gummerer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180103221849.GC2641@hank \
--to=t.gummerer@gmail.com \
--cc=bmwill@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=larsxschneider@gmail.com \
--cc=pclouds@gmail.com \
--cc=peff@peff.net \
--cc=szeder.dev@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).