From: Elijah Newren <newren@gmail.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: Git Mailing List <git@vger.kernel.org>,
Taylor Blau <me@ttaylorr.com>,
Emily Shaffer <emilyshaffer@google.com>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v3 1/5] repository: move global r_f_p_c to repo struct
Date: Thu, 10 Jun 2021 13:47:04 -0700 [thread overview]
Message-ID: <CABPp-BGaet8LAJMj_nGDUShPrAw0tq0U=J-Cw1nxRn5v53pgjg@mail.gmail.com> (raw)
In-Reply-To: <255d1122569dabc4654f7521698744bcc0f13a42.1623345496.git.jonathantanmy@google.com>
On Thu, Jun 10, 2021 at 10:35 AM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> Move repository_format_partial_clone, which is currently a global
> variable, into struct repository. (Full support for per-repository
> partial clone config will be done in a subsequent commit - this is split
> into its own commit because of the extent of the changes needed.)
>
> The new repo-specific variable cannot be set in
> check_repository_format_gently() (as is currently), because that
> function does not know which repo it is operating on (or even whether
> the value is important); therefore this responsibility is delegated to
> the outermost caller that knows. Of all the outermost callers that know
> (found by looking at all functions that call clear_repository_format()),
> I looked at those that either read from the main Git directory or write
> into a struct repository. These callers have been modified accordingly
> (write to the_repository in the former case and write to the given
> struct repository in the latter case).
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> promisor-remote.c | 13 +++----------
> promisor-remote.h | 6 ------
> repository.c | 3 +++
> repository.h | 3 +++
> setup.c | 16 +++++++++++-----
> 5 files changed, 20 insertions(+), 21 deletions(-)
>
> diff --git a/promisor-remote.c b/promisor-remote.c
> index da3f2ca261..d24081dc21 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -5,13 +5,6 @@
> #include "transport.h"
> #include "strvec.h"
>
> -static char *repository_format_partial_clone;
> -
> -void set_repository_format_partial_clone(char *partial_clone)
> -{
> - repository_format_partial_clone = xstrdup_or_null(partial_clone);
> -}
> -
> static int fetch_objects(const char *remote_name,
> const struct object_id *oids,
> int oid_nr)
> @@ -145,15 +138,15 @@ static void promisor_remote_init(void)
>
> git_config(promisor_remote_config, NULL);
>
> - if (repository_format_partial_clone) {
> + if (the_repository->repository_format_partial_clone) {
> struct promisor_remote *o, *previous;
>
> - o = promisor_remote_lookup(repository_format_partial_clone,
> + o = promisor_remote_lookup(the_repository->repository_format_partial_clone,
> &previous);
> if (o)
> promisor_remote_move_to_tail(o, previous);
> else
> - promisor_remote_new(repository_format_partial_clone);
> + promisor_remote_new(the_repository->repository_format_partial_clone);
> }
> }
>
> diff --git a/promisor-remote.h b/promisor-remote.h
> index c7a14063c5..687210ab87 100644
> --- a/promisor-remote.h
> +++ b/promisor-remote.h
> @@ -32,10 +32,4 @@ int promisor_remote_get_direct(struct repository *repo,
> const struct object_id *oids,
> int oid_nr);
>
> -/*
> - * This should be used only once from setup.c to set the value we got
> - * from the extensions.partialclone config option.
> - */
> -void set_repository_format_partial_clone(char *partial_clone);
> -
> #endif /* PROMISOR_REMOTE_H */
> diff --git a/repository.c b/repository.c
> index 448cd557d4..4878c297d9 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -172,6 +172,9 @@ int repo_init(struct repository *repo,
>
> repo_set_hash_algo(repo, format.hash_algo);
>
> + repo->repository_format_partial_clone = format.partial_clone;
> + format.partial_clone = NULL;
This was surprising to me, and I tried to dig around to find out why
you set it to NULL. AFAICT, you're trying to avoid the need to do a
xstrdup(), so you take over ownership in the first line and set to
NULL in the second to avoid a double-free. So, it makes sense, but
given how surprising it was and it took me a while to figure it out,
perhaps it's worth adding a comment that this is what you're doing
here? The same comment would also apply in a few other places in this
patch...
> +
> if (worktree)
> repo_set_worktree(repo, worktree);
>
> diff --git a/repository.h b/repository.h
> index a45f7520fd..6fb16ed336 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -139,6 +139,9 @@ struct repository {
> /* True if commit-graph has been disabled within this process. */
> int commit_graph_disabled;
>
> + /* Configurations related to promisor remotes. */
> + char *repository_format_partial_clone;
> +
> /* Configurations */
>
> /* Indicate if a repository has a different 'commondir' from 'gitdir' */
> diff --git a/setup.c b/setup.c
> index 59e2facd9d..fbedfe8e03 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -468,8 +468,6 @@ static enum extension_result handle_extension_v0(const char *var,
> data->precious_objects = git_config_bool(var, value);
> return EXTENSION_OK;
> } else if (!strcmp(ext, "partialclone")) {
> - if (!value)
> - return config_error_nonbool(var);
> data->partial_clone = xstrdup(value);
> return EXTENSION_OK;
> } else if (!strcmp(ext, "worktreeconfig")) {
> @@ -566,7 +564,6 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
> }
>
> repository_format_precious_objects = candidate->precious_objects;
> - set_repository_format_partial_clone(candidate->partial_clone);
> repository_format_worktree_config = candidate->worktree_config;
> string_list_clear(&candidate->unknown_extensions, 0);
> string_list_clear(&candidate->v1_only_extensions, 0);
> @@ -1193,6 +1190,10 @@ int discover_git_directory(struct strbuf *commondir,
> return -1;
> }
>
> + the_repository->repository_format_partial_clone =
> + candidate.partial_clone;
> + candidate.partial_clone = NULL;
> +
comment would also be nice here
> clear_repository_format(&candidate);
> return 0;
> }
> @@ -1300,8 +1301,12 @@ const char *setup_git_directory_gently(int *nongit_ok)
> gitdir = DEFAULT_GIT_DIR_ENVIRONMENT;
> setup_git_env(gitdir);
> }
> - if (startup_info->have_repository)
> + if (startup_info->have_repository) {
> repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
> + the_repository->repository_format_partial_clone =
> + repo_fmt.partial_clone;
> + repo_fmt.partial_clone = NULL;
and here
> + }
> }
> /*
> * Since precompose_string_if_needed() needs to look at
> @@ -1319,7 +1324,6 @@ const char *setup_git_directory_gently(int *nongit_ok)
> setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
> }
>
> -
why the stray whitespace change?
> strbuf_release(&dir);
> strbuf_release(&gitdir);
> clear_repository_format(&repo_fmt);
> @@ -1386,6 +1390,8 @@ void check_repository_format(struct repository_format *fmt)
> check_repository_format_gently(get_git_dir(), fmt, NULL);
> startup_info->have_repository = 1;
> repo_set_hash_algo(the_repository, fmt->hash_algo);
> + the_repository->repository_format_partial_clone =
> + xstrdup_or_null(fmt->partial_clone);
> clear_repository_format(&repo_fmt);
> }
>
> --
> 2.32.0.rc1.229.g3e70b5a671-goog
Other than the minor nits above, this looks good.
next prev parent reply other threads:[~2021-06-10 20:47 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-01 21:34 [PATCH 0/4] First steps towards partial clone submodules Jonathan Tan
2021-06-01 21:34 ` [PATCH 1/4] promisor-remote: read partialClone config here Jonathan Tan
2021-06-04 19:56 ` Taylor Blau
2021-06-05 1:38 ` Jonathan Tan
2021-06-07 22:41 ` Emily Shaffer
2021-06-01 21:34 ` [PATCH 2/4] promisor-remote: support per-repository config Jonathan Tan
2021-06-04 20:09 ` Taylor Blau
2021-06-05 1:43 ` Jonathan Tan
2021-06-04 21:21 ` Elijah Newren
2021-06-05 1:54 ` Jonathan Tan
2021-06-08 0:48 ` Emily Shaffer
2021-06-01 21:34 ` [PATCH 3/4] run-command: move envvar-resetting function Jonathan Tan
2021-06-04 20:19 ` Taylor Blau
2021-06-05 1:57 ` Jonathan Tan
2021-06-08 0:54 ` Emily Shaffer
2021-06-01 21:34 ` [PATCH 4/4] promisor-remote: teach lazy-fetch in any repo Jonathan Tan
2021-06-04 21:25 ` Taylor Blau
2021-06-05 2:11 ` Jonathan Tan
2021-06-04 21:35 ` Elijah Newren
2021-06-05 2:16 ` Jonathan Tan
2021-06-05 3:48 ` Elijah Newren
2021-06-05 0:22 ` Elijah Newren
2021-06-05 2:16 ` Jonathan Tan
2021-06-08 1:41 ` Emily Shaffer
2021-06-09 4:52 ` Jonathan Tan
2021-06-08 0:25 ` [PATCH v2 0/4] First steps towards partial clone submodules Jonathan Tan
2021-06-08 0:25 ` [PATCH v2 1/4] promisor-remote: read partialClone config here Jonathan Tan
2021-06-08 3:18 ` Junio C Hamano
2021-06-09 4:26 ` Jonathan Tan
2021-06-09 9:30 ` Junio C Hamano
2021-06-09 17:16 ` Jonathan Tan
2021-06-08 17:28 ` Elijah Newren
2021-06-09 4:44 ` Jonathan Tan
2021-06-09 5:34 ` Elijah Newren
2021-06-10 17:25 ` Jonathan Tan
2021-06-08 0:25 ` [PATCH v2 2/4] promisor-remote: support per-repository config Jonathan Tan
2021-06-08 3:30 ` Junio C Hamano
2021-06-09 4:29 ` Jonathan Tan
2021-06-08 0:25 ` [PATCH v2 3/4] run-command: move envvar-resetting function Jonathan Tan
2021-06-08 4:14 ` Junio C Hamano
2021-06-09 4:32 ` Jonathan Tan
2021-06-09 5:28 ` Junio C Hamano
2021-06-09 18:15 ` Jonathan Tan
2021-06-08 0:25 ` [PATCH v2 4/4] promisor-remote: teach lazy-fetch in any repo Jonathan Tan
2021-06-08 4:33 ` Junio C Hamano
2021-06-09 4:39 ` Jonathan Tan
2021-06-09 5:33 ` Junio C Hamano
2021-06-09 18:20 ` Jonathan Tan
2021-06-10 1:26 ` Junio C Hamano
2021-06-08 17:42 ` Elijah Newren
2021-06-09 4:46 ` Jonathan Tan
2021-06-08 17:50 ` [PATCH v2 0/4] First steps towards partial clone submodules Elijah Newren
2021-06-08 23:42 ` Junio C Hamano
2021-06-09 0:07 ` Elijah Newren
2021-06-09 0:18 ` Junio C Hamano
2021-06-09 4:58 ` Jonathan Tan
2021-06-08 1:44 ` [PATCH " Emily Shaffer
2021-06-10 17:35 ` [PATCH v3 0/5] " Jonathan Tan
2021-06-10 17:35 ` [PATCH v3 1/5] repository: move global r_f_p_c to repo struct Jonathan Tan
2021-06-10 20:47 ` Elijah Newren [this message]
2021-06-10 17:35 ` [PATCH v3 2/5] promisor-remote: support per-repository config Jonathan Tan
2021-06-10 17:35 ` [PATCH v3 3/5] submodule: refrain from filtering GIT_CONFIG_COUNT Jonathan Tan
2021-06-10 21:13 ` Elijah Newren
2021-06-10 21:51 ` Jeff King
2021-06-11 17:02 ` Jonathan Tan
2021-06-10 17:35 ` [PATCH v3 4/5] run-command: refactor subprocess env preparation Jonathan Tan
2021-06-10 21:21 ` Elijah Newren
2021-06-10 17:35 ` [PATCH v3 5/5] promisor-remote: teach lazy-fetch in any repo Jonathan Tan
2021-06-10 21:29 ` [PATCH v3 0/5] First steps towards partial clone submodules Elijah Newren
2021-06-15 21:22 ` Elijah Newren
2021-06-17 17:13 ` [PATCH v4 " Jonathan Tan
2021-06-17 17:13 ` [PATCH v4 1/5] repository: move global r_f_p_c to repo struct Jonathan Tan
2021-06-17 17:13 ` [PATCH v4 2/5] promisor-remote: support per-repository config Jonathan Tan
2021-06-17 17:13 ` [PATCH v4 3/5] submodule: refrain from filtering GIT_CONFIG_COUNT Jonathan Tan
2021-06-17 17:13 ` [PATCH v4 4/5] run-command: refactor subprocess env preparation Jonathan Tan
2021-06-17 17:13 ` [PATCH v4 5/5] promisor-remote: teach lazy-fetch in any repo Jonathan Tan
2021-06-19 20:01 ` [PATCH v4 0/5] First steps towards partial clone submodules Elijah Newren
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='CABPp-BGaet8LAJMj_nGDUShPrAw0tq0U=J-Cw1nxRn5v53pgjg@mail.gmail.com' \
--to=newren@gmail.com \
--cc=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.com \
--cc=me@ttaylorr.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).