git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org, me@ttaylorr.com, newren@gmail.com,
	emilyshaffer@google.com
Subject: Re: [PATCH v2 1/4] promisor-remote: read partialClone config here
Date: Tue, 08 Jun 2021 12:18:12 +0900	[thread overview]
Message-ID: <xmqq35ttrqmj.fsf@gitster.g> (raw)
In-Reply-To: <07290cba86fda73ee329a47db8e524b32dba25af.1623111879.git.jonathantanmy@google.com> (Jonathan Tan's message of "Mon, 7 Jun 2021 17:25:56 -0700")

Jonathan Tan <jonathantanmy@google.com> writes:

> Currently, the reading of config related to promisor remotes is done in
> two places: once in setup.c (which sets the global variable
> repository_format_partial_clone, to be read by the code in
> promisor-remote.c), and once in promisor-remote.c. This means that care
> must be taken to ensure that repository_format_partial_clone is set
> before any code in promisor-remote.c accesses it.

The above is very true, but I am puzzled by the chosen direction of
the code movement.

Given that the value in the field repository_format.partial_clone
comes from an extension, and an extension that is not understood by
the version of Git that is running MUST abort the execution of Git,
wouldn't it be guaranteed that, in a correctly written program, the
.partial_clone field must already be set up correctly before
anything else, including those in promissor-remote.c, accesses it?

> To simplify the code, move all such config reading to promisor-remote.c.
> By doing this, it will be easier to see when
> repository_format_partial_clone is written and, thus, to reason about
> the code. This will be especially helpful in a subsequent commit, which
> modifies this code.

So, I am not sure if this simplifies the code the way we want to
read our code.  Doing a thing in one place is indeed simpler than
doing it in two places, but it looks like promisor-remote code
should be using the repository-format data more, not the other way
around, at least to me.

Perhaps I am missing some other motivation, though.

Thanks.

>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  cache.h           |  1 -
>  promisor-remote.c | 14 +++++++++-----
>  promisor-remote.h |  6 ------
>  setup.c           | 10 +++++++---
>  4 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index ba04ff8bd3..dbdcec8601 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1061,7 +1061,6 @@ extern int repository_format_worktree_config;
>  struct repository_format {
>  	int version;
>  	int precious_objects;
> -	char *partial_clone; /* value of extensions.partialclone */
>  	int worktree_config;
>  	int is_bare;
>  	int hash_algo;
> diff --git a/promisor-remote.c b/promisor-remote.c
> index da3f2ca261..c0e5061dfe 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -7,11 +7,6 @@
>  
>  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)
> @@ -99,6 +94,15 @@ static int promisor_remote_config(const char *var, const char *value, void *data
>  	size_t namelen;
>  	const char *subkey;
>  
> +	if (!strcmp(var, "extensions.partialclone")) {
> +		/*
> +		 * NULL value is handled in handle_extension_v0 in setup.c.
> +		 */
> +		if (value)
> +			repository_format_partial_clone = xstrdup(value);
> +		return 0;
> +	}
> +
>  	if (parse_config_key(var, "remote", &name, &namelen, &subkey) < 0)
>  		return 0;
>  
> 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/setup.c b/setup.c
> index 59e2facd9d..d60b6bc554 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -470,7 +470,13 @@ static enum extension_result handle_extension_v0(const char *var,
>  		} else if (!strcmp(ext, "partialclone")) {
>  			if (!value)
>  				return config_error_nonbool(var);
> -			data->partial_clone = xstrdup(value);
> +			/*
> +			 * This config variable will be read together with the
> +			 * other relevant config variables in
> +			 * promisor_remote_config() in promisor_remote.c, so we
> +			 * do not need to read it here. Just report that this
> +			 * extension is known.
> +			 */
>  			return EXTENSION_OK;
>  		} else if (!strcmp(ext, "worktreeconfig")) {
>  			data->worktree_config = git_config_bool(var, value);
> @@ -566,7 +572,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);
> @@ -650,7 +655,6 @@ void clear_repository_format(struct repository_format *format)
>  	string_list_clear(&format->unknown_extensions, 0);
>  	string_list_clear(&format->v1_only_extensions, 0);
>  	free(format->work_tree);
> -	free(format->partial_clone);
>  	init_repository_format(format);
>  }

  reply	other threads:[~2021-06-08  3:18 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 [this message]
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
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=xmqq35ttrqmj.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    --cc=me@ttaylorr.com \
    --cc=newren@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).