threads / discuss / 49654

git pull defaults for recursesubmodules

Subject: git pull defaults for recursesubmodules

## tl;dr

6 messages between Oct 23, 2018 and Oct 25, 2018.

replies: 5people: 4as markdown or json

Tommi Vainikainen· Oct 23, 2018, 21:04 UTC · lore

I configured my local git to fetch with recurseSubmodules = on-demand, which I found the most convenient setting. However then I noticed that I mostly use git pull actually to fetch from remotes, but git pull does not utilize any recurseSubmoddules setting now, or at least I could not find such.

I would expect that if git-config has fetch.recurseSubmodules set, also git pull should use this setting, or at least similar option such as pull.recurseSubmodules should be available. I'd prefer sharing fetch.recurseSubmodules setting here.

I've attached a minimal patch, which I believe implements this configuration usage, and a test case to show my expected behavior for git pull.

-- Tommi Vainikainen

From e4483ec5b3d9c38a6e30aa0ab9fa521cba582345 Mon Sep 17 00:00:00 2001
From: Tommi Vainikainen <thv@iki.fi>
Date: Tue, 23 Oct 2018 23:47:58 +0300
Subject: [PATCH 1/1] pull: obey fetch.recurseSubmodules when fetching
"git pull" now uses same recurse-submodules config for fetching as "git
fetch" by default if not overridden from command line.1
---
 builtin/pull.c            |  3 +++
 t/t5572-pull-submodule.sh | 11 +++++++++++
 2 files changed, 14 insertions(+)
diff --git a/builtin/pull.c b/builtin/pull.c
index 798ecf7faf..ed39b0e8ed 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -347,6 +347,9 @@ static int git_pull_config(const char *var, const char *value, void *cb)
 		recurse_submodules = git_config_bool(var, value) ?
 			RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
 		return 0;
+	} else if (!strcmp(var, "fetch.recursesubmodules")) {
+		recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);
+		return 0;
 	}
 	return git_default_config(var, value, cb);
 }
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index f916729a12..579ae5c374 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -75,6 +75,17 @@ test_expect_success "submodule.recurse option triggers recursive pull" '
 	test_path_is_file super/sub/merge_strategy_2.t
 '
 
+test_expect_success "fetch.recurseSubmodules=true triggers recursive pull" '
+	test_commit -C child fetch_recurse_submodules &&
+	git -C parent submodule update --remote &&
+	git -C parent add sub &&
+	git -C parent commit -m "update submodule" &&
+
+	git -C super config fetch.recurseSubmodules true &&
+	git -C super pull --no-rebase &&
+	test_path_is_file super/sub/fetch_recurse_submodules.t
+'
+
 test_expect_success " --[no-]recurse-submodule and submodule.recurse" '
 	test_commit -C child merge_strategy_3 &&
 	git -C parent submodule update --remote &&
-- 
2.19.1
Stefan Beller· Oct 23, 2018, 21:57 UTC · re: Tommi Vainikainen · lore

Re: git pull defaults for recursesubmodules

On Tue, Oct 23, 2018 at 2:04 PM Tommi Vainikainen <tvainika@gmail.com> wrote:
Show 15 quoted lines
>
> I configured my local git to fetch with recurseSubmodules = on-demand,
> which I found the most convenient setting. However then I noticed that
> I mostly use git pull actually to fetch from remotes, but git pull
> does not utilize any recurseSubmoddules setting now, or at least I
> could not find such.
>
> I would expect that if git-config has fetch.recurseSubmodules set,
> also git pull should use this setting, or at least similar option such
> as pull.recurseSubmodules should be available. I'd prefer sharing
> fetch.recurseSubmodules setting here.
>
> I've attached a minimal patch, which I believe implements this
> configuration usage, and a test case to show my expected behavior for
> git pull.

This makes sense to me and the patch looks good to me. It is unclear to me if this is a regression or an oversight of of a6d7eb2c7a (pull: optionally rebase submodules (remote submodule changes only), 2017-06-23)

Thanks, Stefan

Tommi Vainikainen· Oct 24, 2018, 05:32 UTC · re: Stefan Beller · lore

Re: git pull defaults for recursesubmodules

ke 24. lokak. 2018 klo 0.57 Stefan Beller (sbeller@google.com) kirjoitti:
Show 8 quoted lines
> On Tue, Oct 23, 2018 at 2:04 PM Tommi Vainikainen <tvainika@gmail.com> wrote:
> > I would expect that if git-config has fetch.recurseSubmodules set,
> > also git pull should use this setting, or at least similar option such
>
> This makes sense to me and the patch looks good to me.
> It is unclear to me if this is a regression or an oversight of
> of a6d7eb2c7a (pull: optionally rebase submodules (remote
> submodule changes only), 2017-06-23)

With my testing, no it was not regression at least from that commit. It did not work as I expected before that commit.

What was unclear to me is why this config needs to be read as pull calls fetch, why fetch does not use this configuration as is? If fetch.prune=1, should git pull command also prune or not during fetch? There does not seem to be test case for that behavior.

-- 
Tommi Vainikainen
brian m. carlson· Oct 23, 2018, 23:03 UTC · re: Tommi Vainikainen · lore

Re: git pull defaults for recursesubmodules

On Wed, Oct 24, 2018 at 12:04:06AM +0300, Tommi Vainikainen wrote:
Show 14 quoted lines
> I configured my local git to fetch with recurseSubmodules = on-demand,
> which I found the most convenient setting. However then I noticed that
> I mostly use git pull actually to fetch from remotes, but git pull
> does not utilize any recurseSubmoddules setting now, or at least I
> could not find such.
> 
> I would expect that if git-config has fetch.recurseSubmodules set,
> also git pull should use this setting, or at least similar option such
> as pull.recurseSubmodules should be available. I'd prefer sharing
> fetch.recurseSubmodules setting here.
> 
> I've attached a minimal patch, which I believe implements this
> configuration usage, and a test case to show my expected behavior for
> git pull.

Typically, we prefer patches to be inline; descriptive content like this goes after the --- line in the patch, or in a cover letter.

Show 7 quoted lines
> From e4483ec5b3d9c38a6e30aa0ab9fa521cba582345 Mon Sep 17 00:00:00 2001
> From: Tommi Vainikainen <thv@iki.fi>
> Date: Tue, 23 Oct 2018 23:47:58 +0300
> Subject: [PATCH 1/1] pull: obey fetch.recurseSubmodules when fetching
> 
> "git pull" now uses same recurse-submodules config for fetching as "git
> fetch" by default if not overridden from command line.1
You have an extra '1" at the end of this line.
Also, missing sign-off.  See Documentation/SubmittingPatches.
Show 32 quoted lines
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 798ecf7faf..ed39b0e8ed 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -347,6 +347,9 @@ static int git_pull_config(const char *var, const char *value, void *cb)
>  		recurse_submodules = git_config_bool(var, value) ?
>  			RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
>  		return 0;
> +	} else if (!strcmp(var, "fetch.recursesubmodules")) {
> +		recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);
> +		return 0;
>  	}
>  	return git_default_config(var, value, cb);
>  }
> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
> index f916729a12..579ae5c374 100755
> --- a/t/t5572-pull-submodule.sh
> +++ b/t/t5572-pull-submodule.sh
> @@ -75,6 +75,17 @@ test_expect_success "submodule.recurse option triggers recursive pull" '
>  	test_path_is_file super/sub/merge_strategy_2.t
>  '
>  
> +test_expect_success "fetch.recurseSubmodules=true triggers recursive pull" '
> +	test_commit -C child fetch_recurse_submodules &&
> +	git -C parent submodule update --remote &&
> +	git -C parent add sub &&
> +	git -C parent commit -m "update submodule" &&
> +
> +	git -C super config fetch.recurseSubmodules true &&
> +	git -C super pull --no-rebase &&
> +	test_path_is_file super/sub/fetch_recurse_submodules.t
> +'

Can we have a test that --no-recurse-submodules overrides fetch.recurseSubmodules?

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Tommi Vainikainen· Oct 24, 2018, 06:57 UTC · re: brian m. carlson · lore

Re: git pull defaults for recursesubmodules

ke 24. lokak. 2018 klo 2.03 brian m. carlson (sandals@crustytoothpaste.net) kirjoitti:

> You have an extra '1" at the end of this line.
> Also, missing sign-off.  See Documentation/SubmittingPatches.

After reading SubmittingPatches I didn't find if I should now send a fresh patch with changes squashed together or new commits appended after first commit in that patch. Patch is updated accordingly as fresh patch.

> Can we have a test that --no-recurse-submodules overrides
> fetch.recurseSubmodules?

Attached (sorry about that, I've no access now to more convenient mail setup) is refreshed patch which also tests that. I included it in same patch to follow style of other tests in t5572-pull-submodule.

-- 
Tommi Vainikainen


From 5d4b495f7c68a2e99314fef90e538e0d44ec1056 Mon Sep 17 00:00:00 2001
From: Tommi Vainikainen <thv@iki.fi>
Date: Wed, 24 Oct 2018 09:06:54 +0300
Subject: [PATCH] pull: obey fetch.recurseSubmodules when fetching

"git pull" now uses same recurse-submodules config for fetching as "git
fetch" by default if not overridden from command line.

The command line arg --recurse-submodules=no overrides
fetch.recurseSubmodules configuration.

Signed-off-by: Tommi Vainikainen <thv@iki.fi>
---
 builtin/pull.c            |  3 +++
 t/t5572-pull-submodule.sh | 13 +++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/builtin/pull.c b/builtin/pull.c
index 681c127a07..bb51cb531d 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -346,6 +346,9 @@ static int git_pull_config(const char *var, const char *value, void *cb)
 		recurse_submodules = git_config_bool(var, value) ?
 			RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
 		return 0;
+	} else if (!strcmp(var, "fetch.recursesubmodules")) {
+		recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);
+		return 0;
 	}
 	return git_default_config(var, value, cb);
 }
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index f916729a12..66c479a135 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -75,6 +75,19 @@ test_expect_success "submodule.recurse option triggers recursive pull" '
 	test_path_is_file super/sub/merge_strategy_2.t
 '
 
+test_expect_success "fetch.recurseSubmodules=true triggers recursive pull" '
+	test_commit -C child fetch_recurse_submodules &&
+	git -C parent submodule update --remote &&
+	git -C parent add sub &&
+	git -C parent commit -m "update submodule" &&
+
+	git -C super config fetch.recurseSubmodules true &&
+	git -C super pull --recurse-submodules=no --no-rebase &&
+	test_path_is_missing super/sub/fetch_recurse_submodules.t &&
+	git -C super pull --no-rebase &&
+	test_path_is_file super/sub/fetch_recurse_submodules.t
+'
+
 test_expect_success " --[no-]recurse-submodule and submodule.recurse" '
 	test_commit -C child merge_strategy_3 &&
 	git -C parent submodule update --remote &&
-- 
2.19.1
Junio C Hamano· Oct 25, 2018, 10:09 UTC · re: Tommi Vainikainen · lore

Re: git pull defaults for recursesubmodules

Tommi Vainikainen <tvainika@gmail.com> writes:
> After reading SubmittingPatches I didn't find if I should now send a
> fresh patch with
> changes squashed together or new commits appended after first commit in that
> patch. Patch is updated accordingly as fresh patch.
(just on mechanics, not on the contents of your actual patch)

You can and should treat any topic that is not yet in 'next' as if it did not exist. If you refined based on a v1 patch, pretend as if you were a perfect developer and you came up with that refined version without producing any problematic things that were pointed ont in your v1. Pretend mistakes in v1 never happened. Pretend that you are perfect! ;-)

If you can limit the signs that an earlier rounds ever existed to
(1) The In-reply-to: header of the message you send your updated
    version of the patch in, so that people can find the older
    version and its discussion thread, and
(2) The cover letter that describes what you improved in the
    updated version relative to the last round, in addition to the
    overview of the series [note: this only exists for a larger
    patch series, and not usually done for a single patch]
you achieved your goal.

← back to recent threads