git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Glen Choo via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
	Philippe Blain <levraiphilippeblain@gmail.com>,
	Huang Zou <huang.zou@schrodinger.com>,
	Josh Steadmon <steadmon@google.com>,
	Glen Choo <chooglen@google.com>
Subject: Re: [PATCH] pull: only pass '--recurse-submodules' to subcommands
Date: Mon, 09 May 2022 17:09:09 -0700	[thread overview]
Message-ID: <xmqqpmkm9rkq.fsf@gitster.g> (raw)
In-Reply-To: <pull.1262.git.git.1652138854255.gitgitgadget@gmail.com> (Glen Choo via GitGitGadget's message of "Mon, 09 May 2022 23:27:34 +0000")

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

>     This patch fixes the original bug, but not in the 'obvious' way of
>     teaching "git pull" to parse fetch.recurseSubmodules. Instead, "git
>     pull" now propagates its value of "--recurse-submodules" to "git fetch"
>     (ignoring any config values), and leaves the config parsing to "git
>     fetch".

OK.  So the sub-git that is run in submodules will always see a
command line option, and what its configuration file says does not
matter?

>     I think this works better because we get a nice separation of "config
>     that git pull cares about" and "config that its subprocess care about",
>     and as a result:
>     
>      * We don't maintain two identical config-parsing implementations in
>        "git pull" and "git fetch".
>      * It works better with other commands invoked by "git pull" e.g. "git
>        merge" won't accidentally respect fetch.recurseSubmodules.
>     
>     PS I'm having a hard time writing today, let me know how the commit
>     message/cover letter can be improved :)

OK.

> diff --git a/builtin/pull.c b/builtin/pull.c
> index 4d667abc19d..01155ba67b2 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -72,6 +72,7 @@ static const char * const pull_usage[] = {
>  static int opt_verbosity;
>  static char *opt_progress;
>  static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
> +static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;

This ...

>  /* Options passed to git-merge or git-rebase */
>  static enum rebase_type opt_rebase = -1;
> @@ -120,7 +121,7 @@ static struct option pull_options[] = {
>  		N_("force progress reporting"),
>  		PARSE_OPT_NOARG),
>  	OPT_CALLBACK_F(0, "recurse-submodules",
> -		   &recurse_submodules, N_("on-demand"),
> +		   &recurse_submodules_cli, N_("on-demand"),
>  		   N_("control for recursive fetching of submodules"),
>  		   PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),

... is where we keep track of what value we got from the command
line.  OK.

> @@ -536,8 +537,8 @@ static int run_fetch(const char *repo, const char **refspecs)
>  		strvec_push(&args, opt_tags);
>  	if (opt_prune)
>  		strvec_push(&args, opt_prune);
> -	if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
> -		switch (recurse_submodules) {
> +	if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)

The fact that the variable is different from _DEFAULT is a sure sign
that we got something from the command line, because there is no way
for the command line option to set the variable to _DEFAULT (in
other words, _DEFAULT is not really a default, it is a sign that it
is not yet set to any value).  OK.

> +		switch (recurse_submodules_cli) {
>  		case RECURSE_SUBMODULES_ON:
>  			strvec_push(&args, "--recurse-submodules=on");
>  			break;

OK, so the net effect is that we only strvec_push() a command line
option to underlying "fetch" when we got a command line option.  It
does not matter what "recurse_submodules" variable is set to.  The
variable can be set via the configuration mechanism.  _cli one is
different.

OK.  And they underying "git fetch" will read its configuration as
needed anyway (if we do not do these strvec_push() here).

Sounds very sensible.  FWIW, despite what you said earlier, I find
this "if we have command line override, pass it down, otherwise they
know how to read and interpret configuration on their own" a very
sensible and intuitive approach.

Very nicely done.

> @@ -1001,6 +1002,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>  
>  	argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
>  
> +	if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
> +		recurse_submodules = recurse_submodules_cli;

This is a small fallout from the separation of the variables.
Again, _DEFAULT is not "the default behaviour whatever it is", but
is a signal "This was not set at all", and that makes this addition
correct.  At some point, we may want to rename the _DEFAULT to
_UNSPECIFIED or something for readability, but it does not have to
be a part of this fix.

> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
> index fa6b4cca65c..65aaa7927fb 100755
> --- a/t/t5572-pull-submodule.sh
> +++ b/t/t5572-pull-submodule.sh
> @@ -107,6 +107,20 @@ test_expect_success " --[no-]recurse-submodule and submodule.recurse" '
>  	test_path_is_file super/sub/merge_strategy_4.t
>  '
>  
> +test_expect_success "fetch.recurseSubmodules option triggers recursive fetch (but not recursive update)" '
> +	test_commit -C child merge_strategy_5 &&
> +	git -C parent submodule update --remote &&
> +	git -C parent add sub &&
> +	git -C parent commit -m "update submodule" &&
> +
> +	git -C super -c fetch.recursesubmodules=true pull --no-rebase &&
> +	# Check that the submodule commit was fetched
> +	sub_oid=$(git -C super rev-parse FETCH_HEAD:sub) &&
> +	git -C super/sub cat-file -e $sub_oid &&
> +	# Check that the submodule worktree did not update
> +	! test_path_is_file super/sub/merge_strategy_5.t
> +'
> +
>  test_expect_success 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '
>  	# This tests the following scenario :
>  	# - local submodule has new commits
>
> base-commit: e8005e4871f130c4e402ddca2032c111252f070a

Thanks, will queue.

  reply	other threads:[~2022-05-10  0:09 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-09 23:27 Glen Choo via GitGitGadget
2022-05-10  0:09 ` Junio C Hamano [this message]
2022-05-10  0:44 ` Junio C Hamano
2022-05-10 13:28 ` Philippe Blain
2022-05-10 18:27   ` Glen Choo
2022-05-10 18:43   ` Glen Choo
2022-05-10 19:25 ` [PATCH v2] " Glen Choo via GitGitGadget
2022-05-11 22:30   ` Philippe Blain
2022-05-11 22:34     ` Junio C Hamano
2022-05-11 22:35       ` Philippe Blain
2022-05-11 23:21         ` Glen Choo
2022-05-12 20:37           ` Junio C Hamano
2022-05-11 23:42   ` [PATCH v3] pull: do not let submodule.recurse override fetch.recurseSubmodules Glen Choo via GitGitGadget
2022-05-12 20:38     ` Junio C Hamano
2022-05-12 23:35       ` Philippe Blain

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=xmqqpmkm9rkq.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=chooglen@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=huang.zou@schrodinger.com \
    --cc=levraiphilippeblain@gmail.com \
    --cc=steadmon@google.com \
    --subject='Re: [PATCH] pull: only pass '\''--recurse-submodules'\'' to subcommands' \
    /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

Code repositories for project(s) associated with this 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).