submodule--helper: fix initialization of warn_if_uninitialized

PATCH Superseded Orgad Shaneh via GitGitGadget 4 participants 11 messages Apr 24, 2022 – Apr 27, 2022

lore

Share & tools

lore

Orgad Shaneh via GitGitGadget

author

2022-04-24 06:26 UTC #0 lore raw

PATCH 1/1 submodule--helper: fix initialization of warn_if_uninitialized

From: Orgad Shaneh <[email protected]>

This field is supposed to be off by default, and it is only enabled when running git submodule update <path>, and path is not initialized.

Commit c9911c9358 changed it to enabled by default. This affects for example git checkout, which displays the following warning for each uninitialized submodule:

Submodule path 'sub' not initialized
Maybe you want to use 'update --init'?

Amends c9911c9358e611390e2444f718c73900d17d3d60.

Signed-off-by Orgad Shaneh <[email protected]>

---
submodule--helper: fix initialization of warn_if_uninitialized

This field is supposed to be off by default, and it is only enabled when running git submodule update <path>, and path is not initialized.

Commit c9911c9358 changed it to enabled by default. This affects for example git checkout, which displays the following warning for each uninitialized submodule:

Submodule path 'sub' not initialized
Maybe you want to use 'update --init'?

Amends c9911c9358e611390e2444f718c73900d17d3d60.

Signed-off-by Orgad Shaneh [email protected]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1258%2Forgads%2Fsub-no-warn-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1258/orgads/sub-no-warn-v1
Pull-Request: https://github.com/git/git/pull/1258

 builtin/submodule--helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

builtin/submodule--helper.c +1 -1

index 2c87ef9364f..b28112e3040 100644
@@ -2026,7 +2026,7 @@ struct update_data {
 	.references = STRING_LIST_INIT_DUP, \
 	.single_branch = -1, \
 	.max_jobs = 1, \
-	.warn_if_uninitialized = 1, \
+	.warn_if_uninitialized = 0, \
 }
 
 static void next_submodule_warn_missing(struct submodule_update_clone *suc,

base-commit: 6cd33dceed60949e2dbc32e3f0f5e67c4c882e1e

Signature · 1 line

gitgitgadget

Junio C Hamano

2022-04-25 09:36 UTC #1 lore raw

Quoted 4 lines · Orgad Shaneh via GitGitGadget

From: Orgad Shaneh <[email protected]>
This field is supposed to be off by default, and it is only enabled when running git submodule update <path>, and path is not initialized.

"is supposed to be": can you substantiate it with evidence and logic?

One easy way to explain the above is to examine what the default value was before the problematic commit, and go back from that commit to see how the default was decided, and examine how the member is used.

Quoted 6 lines

Commit c9911c9358 changed it to enabled by default. This affects for example git checkout, which displays the following warning for each uninitialized submodule:
Submodule path 'sub' not initialized
Maybe you want to use 'update --init'?

We refer to an existing commit like this:

c9911c93 (submodule--helper: teach update_data more options, 2022-03-15) changed it to be enabled by default.

So, taking the above together:

The .warn_if_uninitialized member was introduced by 48308681 (git submodule update: have a dedicated helper for cloning, 2016-02-29) to submodule_update_clone struct and initialized to false. When c9911c93 (submodule--helper: teach update_data more options, 2022-03-15) moved it to update_data struct, it started to initialize it to true but this change was not explained in its log message.

The member is set to true only when pathspec was given, and is used when a submodule that matched the pathspec is found uninitialized to give diagnostic message. "submodule update" without pathspec is supposed to iterate over all submodules (i.e. without pathspec limitation) and update only the initialized submodules, and finding uninitialized submodules during the iteration is a totally expected and normal thing that should not be warned.

Fix this regression by initializing the member to 0.

Quoted 1 line

Amends c9911c9358e611390e2444f718c73900d17d3d60.

In the context of "git", the verb "amend" has a specific meaning (i.e. "commit --amend"), and we should refrain from using it when we are doing something else to avoid confusing readers.

We could say

Fix this problem that was introduced by c9911c93 (submodule--helper: teach update_data more options, 2022-03-15)

but it is not necessary, as long as we explained how that commit broke the code to justify this change (which we should do anyway).

Quoted 6 lines

Signed-off-by Orgad Shaneh <[email protected]>
---
submodule--helper: fix initialization of warn_if_uninitialized
...
Signed-off-by Orgad Shaneh [email protected]

Here under three-dash line is where you would write comment meant for those who read the message on the list that are not necessarily meant to be part of resulting commit. Repeating the same message as the log message is not desired here.

Quoted 12 lines

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1258%2Forgads%2Fsub-no-warn-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1258/orgads/sub-no-warn-v1
Pull-Request: https://github.com/git/git/pull/1258
 builtin/submodule--helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/submodule--helper.c +0 -0
index 2c87ef9364f..b28112e3040 100644

builtin/submodule--helper.c struct update_data { · line 2030

Quoted 6 lines

builtin/submodule--helper.c +1 -1

@@ -2026,7 +2026,7 @@ struct update_data {
 	.references = STRING_LIST_INIT_DUP, \
 	.single_branch = -1, \
 	.max_jobs = 1, \
-	.warn_if_uninitialized = 1, \
+	.warn_if_uninitialized = 0, \
 }

This is not wrong per-se, but omitting the mention of the member altogether and letting the compiler initialize it to 0 would probably be a better fix.

Thanks.

Orgad Shaneh

2022-04-25 12:44 UTC #2 lore raw

Quoted 47 lines · Junio C Hamano

Quoted 4 lines · Orgad Shaneh via GitGitGadget
From: Orgad Shaneh <[email protected]>
This field is supposed to be off by default, and it is only enabled when running git submodule update <path>, and path is not initialized.
"is supposed to be": can you substantiate it with evidence and logic?
One easy way to explain the above is to examine what the default value was before the problematic commit, and go back from that commit to see how the default was decided, and examine how the member is used.
Quoted 6 lines
Commit c9911c9358 changed it to enabled by default. This affects for example git checkout, which displays the following warning for each uninitialized submodule:
Submodule path 'sub' not initialized
Maybe you want to use 'update --init'?
We refer to an existing commit like this:
c9911c93 (submodule--helper: teach update_data more options, 2022-03-15) changed it to be enabled by default.
So, taking the above together:
The .warn_if_uninitialized member was introduced by 48308681 (git submodule update: have a dedicated helper for cloning, 2016-02-29) to submodule_update_clone struct and initialized to false. When c9911c93 (submodule--helper: teach update_data more options, 2022-03-15) moved it to update_data struct, it started to initialize it to true but this change was not explained in its log message.
The member is set to true only when pathspec was given, and is used when a submodule that matched the pathspec is found uninitialized to give diagnostic message. "submodule update" without pathspec is supposed to iterate over all submodules (i.e. without pathspec limitation) and update only the initialized submodules, and finding uninitialized submodules during the iteration is a totally expected and normal thing that should not be warned.

Quoted 1 line

Fix this regression by initializing the member to 0.

Thank you very much. Updated.

Quoted 6 lines

Quoted 1 line
Amends c9911c9358e611390e2444f718c73900d17d3d60.
In the context of "git", the verb "amend" has a specific meaning (i.e. "commit --amend"), and we should refrain from using it when we are doing something else to avoid confusing readers.

Quoted 7 lines

We could say
Fix this problem that was introduced by c9911c93 (submodule--helper: teach update_data more options, 2022-03-15)
but it is not necessary, as long as we explained how that commit broke the code to justify this change (which we should do anyway).

I'm used to this from other projects, but ok.

Quoted 9 lines

Quoted 4 lines
---
submodule--helper: fix initialization of warn_if_uninitialized
...
Signed-off-by Orgad Shaneh [email protected]
Here under three-dash line is where you would write comment meant for those who read the message on the list that are not necessarily meant to be part of resulting commit. Repeating the same message as the log message is not desired here.

This was done by GitGitGadget. I have no idea how to avoid it.

Quoted 7 lines

Quoted 7 lines
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1258%2Forgads%2Fsub-no-warn-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1258/orgads/sub-no-warn-v1
Pull-Request: https://github.com/git/git/pull/1258
 builtin/submodule--helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 15 lines

builtin/submodule--helper.c struct update_data { · line 2030
Quoted 11 lines
builtin/submodule--helper.c +1 -1
index 2c87ef9364f..b28112e3040 100644
@@ -2026,7 +2026,7 @@ struct update_data {
      .references = STRING_LIST_INIT_DUP, \
      .single_branch = -1, \
      .max_jobs = 1, \
-     .warn_if_uninitialized = 1, \
+     .warn_if_uninitialized = 0, \
 }
This is not wrong per-se, but omitting the mention of the member altogether and letting the compiler initialize it to 0 would probably be a better fix.

Done.

Thanks for the review.

- Orgad

Orgad Shaneh via GitGitGadget

author

2022-04-25 12:46 UTC #3 lore raw

PATCH v2 1/1 submodule--helper: fix initialization of warn_if_uninitialized

In reply to Orgad Shaneh via GitGitGadget

From: Orgad Shaneh <[email protected]>

Signed-off-by Orgad Shaneh <[email protected]>

---
submodule--helper: fix initialization of warn_if_uninitialized

Signed-off-by Orgad Shaneh [email protected]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1258%2Forgads%2Fsub-no-warn-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1258/orgads/sub-no-warn-v2
Pull-Request: https://github.com/git/git/pull/1258

Range-diff vs v1:

1: 089a52a50b4 ! 1: 1e34c9cad18 submodule--helper: fix initialization of warn_if_uninitialized
@@ Metadata
## Commit message ##
submodule--helper: fix initialization of warn_if_uninitialized

- This field is supposed to be off by default, and it is only enabled when
- running git submodule update <path>, and path is not initialized.
+ The .warn_if_uninitialized member was introduced by 48308681
+ (git submodule update: have a dedicated helper for cloning,
+ 2016-02-29) to submodule_update_clone struct and initialized to
+ false. When c9911c93 (submodule--helper: teach update_data more
+ options, 2022-03-15) moved it to update_data struct, it started
+ to initialize it to true but this change was not explained in
+ its log message.

- Commit c9911c9358 changed it to enabled by default. This affects for
- example git checkout, which displays the following warning for each
- uninitialized submodule:
-
- Submodule path 'sub' not initialized
- Maybe you want to use 'update --init'?
-
- Amends c9911c9358e611390e2444f718c73900d17d3d60.
+ The member is set to true only when pathspec was given, and is
+ used when a submodule that matched the pathspec is found
+ uninitialized to give diagnostic message. "submodule update"
+ without pathspec is supposed to iterate over all submodules
+ (i.e. without pathspec limitation) and update only the
+ initialized submodules, and finding uninitialized submodules
+ during the iteration is a totally expected and normal thing that
+ should not be warned.

Signed-off-by Orgad Shaneh <[email protected]>

@@ builtin/submodule--helper.c: struct update_data {
.single_branch = -1, \
.max_jobs = 1, \
- .warn_if_uninitialized = 1, \
-+ .warn_if_uninitialized = 0, \
}

static void next_submodule_warn_missing(struct submodule_update_clone *suc,

 builtin/submodule--helper.c | 1 -
 1 file changed, 1 deletion(-)

builtin/submodule--helper.c +0 -1

index 2c87ef9364f..1a8e5d06214 100644
@@ -2026,7 +2026,6 @@ struct update_data {
 	.references = STRING_LIST_INIT_DUP, \
 	.single_branch = -1, \
 	.max_jobs = 1, \
-	.warn_if_uninitialized = 1, \
 }
 
 static void next_submodule_warn_missing(struct submodule_update_clone *suc,

base-commit: 6cd33dceed60949e2dbc32e3f0f5e67c4c882e1e

Signature · 1 line

gitgitgadget

Junio C Hamano

2022-04-25 18:25 UTC #4 lore raw

Quoted 17 lines · Orgad Shaneh via GitGitGadget

The member is set to true only when pathspec was given, and is used when a submodule that matched the pathspec is found uninitialized to give diagnostic message. "submodule update" without pathspec is supposed to iterate over all submodules (i.e. without pathspec limitation) and update only the initialized submodules, and finding uninitialized submodules during the iteration is a totally expected and normal thing that should not be warned.
...
 builtin/submodule--helper.c | 1 -
 1 file changed, 1 deletion(-)
builtin/submodule--helper.c +0 -0
index 2c87ef9364f..1a8e5d06214 100644

builtin/submodule--helper.c struct update_data { · line 2029

Quoted 5 lines

builtin/submodule--helper.c +0 -1

@@ -2026,7 +2026,6 @@ struct update_data {
 	.references = STRING_LIST_INIT_DUP, \
 	.single_branch = -1, \
 	.max_jobs = 1, \
-	.warn_if_uninitialized = 1, \
 }

Is this a fix we can protect from future breakge by adding a test or tweaking an existing test? It is kind of surprising if we did not have any test that runs "git submodule update" in a superproject with initialized and uninitialized submodule(s) and make sure only the initialized ones are updated. It may be the matter of examining the warning output that is currently ignored in such a test, if there is one.

Thanks.

Junio C Hamano

2022-04-25 20:56 UTC #5 lore raw

Quoted 7 lines · Junio C Hamano

Is this a fix we can protect from future breakge by adding a test or tweaking an existing test? It is kind of surprising if we did not have any test that runs "git submodule update" in a superproject with initialized and uninitialized submodule(s) and make sure only the initialized ones are updated. It may be the matter of examining the warning output that is currently ignored in such a test, if there is one.

Here is a quick-and-dirty one I came up with. The superproject "super" has a handful of submodules ("submodule" and "rebasing" being two of them), so the new tests clone the superproject and initializes only one submodule. Then we see how "submodule update" with pathspec works with these two submodules (one initialied and the other not). In another test, we see how "submodule update" without pathspec works.

I'll queue this on top of your fix for now tentatively. If nobody finds flaws in them, I'll just squash it in soonish before merging the whole thing for the maintenance track.

Thanks.

 t/t7406-submodule-update.sh | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

t/t7406-submodule-update.sh +33 -0

index 000e055811..43f779d751 100755
@@ -670,6 +670,39 @@ test_expect_success 'submodule update --init skips submodule with update=none' '
 	)
 '
 
+test_expect_success 'submodule update with pathspec warns against uninitialized ones' '
+	test_when_finished "rm -fr selective" &&
+	git clone super selective &&
+	(
+		cd selective &&
+		git submodule init submodule &&
+
+		git submodule update submodule 2>err &&
+		! grep "Submodule path .* not initialized" err &&
+
+		git submodule update rebasing 2>err &&
+		grep "Submodule path .rebasing. not initialized" err &&
+
+		test_path_exists submodule/.git &&
+		test_path_is_missing rebasing/.git
+	)
+
+'
+
+test_expect_success 'submodule update without pathspec updates only initialized ones' '
+	test_when_finished "rm -fr selective" &&
+	git clone super selective &&
+	(
+		cd selective &&
+		git submodule init submodule &&
+		git submodule update 2>err &&
+		test_path_exists submodule/.git &&
+		test_path_is_missing rebasing/.git &&
+		! grep "Submodule path .* not initialized" err
+	)
+
+'
+
 test_expect_success 'submodule update continues after checkout error' '
 	(cd super &&
 	 git reset --hard HEAD &&

Glen Choo

2022-04-27 22:22 UTC #6 lore raw

Quoted 18 lines · Junio C Hamano

Quoted 7 lines · Junio C Hamano
Is this a fix we can protect from future breakge by adding a test or tweaking an existing test? It is kind of surprising if we did not have any test that runs "git submodule update" in a superproject with initialized and uninitialized submodule(s) and make sure only the initialized ones are updated. It may be the matter of examining the warning output that is currently ignored in such a test, if there is one.
Here is a quick-and-dirty one I came up with. The superproject "super" has a handful of submodules ("submodule" and "rebasing" being two of them), so the new tests clone the superproject and initializes only one submodule. Then we see how "submodule update" with pathspec works with these two submodules (one initialied and the other not). In another test, we see how "submodule update" without pathspec works.

Quoted 5 lines

I'll queue this on top of your fix for now tentatively. If nobody finds flaws in them, I'll just squash it in soonish before merging the whole thing for the maintenance track.
Thanks.

Thanks for adding the tests!

Quoted 37 lines

 t/t7406-submodule-update.sh | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

t/t7406-submodule-update.sh +26 -0

index 000e055811..43f779d751 100755
@@ -670,6 +670,39 @@ test_expect_success 'submodule update --init skips submodule with update=none' '
 	)
 '
 
+test_expect_success 'submodule update with pathspec warns against uninitialized ones' '
+	test_when_finished "rm -fr selective" &&
+	git clone super selective &&
+	(
+		cd selective &&
+		git submodule init submodule &&
+
+		git submodule update submodule 2>err &&
+		! grep "Submodule path .* not initialized" err &&
+
+		git submodule update rebasing 2>err &&
+		grep "Submodule path .rebasing. not initialized" err &&
+
+		test_path_exists submodule/.git &&
+		test_path_is_missing rebasing/.git
+	)
+
+'
+
+test_expect_success 'submodule update without pathspec updates only initialized ones' '
+	test_when_finished "rm -fr selective" &&
+	git clone super selective &&
+	(
+		cd selective &&
+		git submodule init submodule &&
+		git submodule update 2>err &&

t/t7406-submodule-update.sh test_expect_success 'submodule update --init skips submodule with update=none' ' · line 708

Quoted 10 lines

t/t7406-submodule-update.sh +7 -0

+		test_path_exists submodule/.git &&
+		test_path_is_missing rebasing/.git &&
+		! grep "Submodule path .* not initialized" err
+	)
+
+'
+
 test_expect_success 'submodule update continues after checkout error' '
 	(cd super &&
 	 git reset --hard HEAD &&

So we test that we only issue the warning when a pathspec is given, and that we ignore uninitialized submodules when no pathspec is given. I think this covers all of the cases, so this looks good, thanks!

Glen Choo

2022-04-27 22:23 UTC #7 lore raw

In reply to Junio C Hamano

Quoted 18 lines · Junio C Hamano

Quoted 7 lines · Junio C Hamano
Is this a fix we can protect from future breakge by adding a test or tweaking an existing test? It is kind of surprising if we did not have any test that runs "git submodule update" in a superproject with initialized and uninitialized submodule(s) and make sure only the initialized ones are updated. It may be the matter of examining the warning output that is currently ignored in such a test, if there is one.
Here is a quick-and-dirty one I came up with. The superproject "super" has a handful of submodules ("submodule" and "rebasing" being two of them), so the new tests clone the superproject and initializes only one submodule. Then we see how "submodule update" with pathspec works with these two submodules (one initialied and the other not). In another test, we see how "submodule update" without pathspec works.

Quoted 5 lines

I'll queue this on top of your fix for now tentatively. If nobody finds flaws in them, I'll just squash it in soonish before merging the whole thing for the maintenance track.
Thanks.

Thanks for adding the tests!

Quoted 37 lines

 t/t7406-submodule-update.sh | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

t/t7406-submodule-update.sh +26 -0

index 000e055811..43f779d751 100755
@@ -670,6 +670,39 @@ test_expect_success 'submodule update --init skips submodule with update=none' '
 	)
 '
 
+test_expect_success 'submodule update with pathspec warns against uninitialized ones' '
+	test_when_finished "rm -fr selective" &&
+	git clone super selective &&
+	(
+		cd selective &&
+		git submodule init submodule &&
+
+		git submodule update submodule 2>err &&
+		! grep "Submodule path .* not initialized" err &&
+
+		git submodule update rebasing 2>err &&
+		grep "Submodule path .rebasing. not initialized" err &&
+
+		test_path_exists submodule/.git &&
+		test_path_is_missing rebasing/.git
+	)
+
+'
+
+test_expect_success 'submodule update without pathspec updates only initialized ones' '
+	test_when_finished "rm -fr selective" &&
+	git clone super selective &&
+	(
+		cd selective &&
+		git submodule init submodule &&
+		git submodule update 2>err &&

t/t7406-submodule-update.sh test_expect_success 'submodule update --init skips submodule with update=none' ' · line 708

Quoted 10 lines

t/t7406-submodule-update.sh +7 -0

+		test_path_exists submodule/.git &&
+		test_path_is_missing rebasing/.git &&
+		! grep "Submodule path .* not initialized" err
+	)
+
+'
+
 test_expect_success 'submodule update continues after checkout error' '
 	(cd super &&
 	 git reset --hard HEAD &&

Glen Choo

2022-04-27 22:25 UTC #8 lore raw

In reply to Junio C Hamano

Quoted 18 lines · Junio C Hamano

Quoted 7 lines · Junio C Hamano
Is this a fix we can protect from future breakge by adding a test or tweaking an existing test? It is kind of surprising if we did not have any test that runs "git submodule update" in a superproject with initialized and uninitialized submodule(s) and make sure only the initialized ones are updated. It may be the matter of examining the warning output that is currently ignored in such a test, if there is one.
Here is a quick-and-dirty one I came up with. The superproject "super" has a handful of submodules ("submodule" and "rebasing" being two of them), so the new tests clone the superproject and initializes only one submodule. Then we see how "submodule update" with pathspec works with these two submodules (one initialied and the other not). In another test, we see how "submodule update" without pathspec works.

Quoted 5 lines

I'll queue this on top of your fix for now tentatively. If nobody finds flaws in them, I'll just squash it in soonish before merging the whole thing for the maintenance track.
Thanks.

Thanks for adding the tests!

Quoted 37 lines

 t/t7406-submodule-update.sh | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

t/t7406-submodule-update.sh +26 -0

index 000e055811..43f779d751 100755
@@ -670,6 +670,39 @@ test_expect_success 'submodule update --init skips submodule with update=none' '
 	)
 '
 
+test_expect_success 'submodule update with pathspec warns against uninitialized ones' '
+	test_when_finished "rm -fr selective" &&
+	git clone super selective &&
+	(
+		cd selective &&
+		git submodule init submodule &&
+
+		git submodule update submodule 2>err &&
+		! grep "Submodule path .* not initialized" err &&
+
+		git submodule update rebasing 2>err &&
+		grep "Submodule path .rebasing. not initialized" err &&
+
+		test_path_exists submodule/.git &&
+		test_path_is_missing rebasing/.git
+	)
+
+'
+
+test_expect_success 'submodule update without pathspec updates only initialized ones' '
+	test_when_finished "rm -fr selective" &&
+	git clone super selective &&
+	(
+		cd selective &&
+		git submodule init submodule &&
+		git submodule update 2>err &&

t/t7406-submodule-update.sh test_expect_success 'submodule update --init skips submodule with update=none' ' · line 708

Quoted 10 lines

t/t7406-submodule-update.sh +7 -0

+		test_path_exists submodule/.git &&
+		test_path_is_missing rebasing/.git &&
+		! grep "Submodule path .* not initialized" err
+	)
+
+'
+
 test_expect_success 'submodule update continues after checkout error' '
 	(cd super &&
 	 git reset --hard HEAD &&

Glen Choo

2022-04-27 22:25 UTC #9 lore raw

In reply to Orgad Shaneh via GitGitGadget

Quoted 106 lines · Orgad Shaneh via GitGitGadget

From: Orgad Shaneh <[email protected]>
The .warn_if_uninitialized member was introduced by 48308681 (git submodule update: have a dedicated helper for cloning, 2016-02-29) to submodule_update_clone struct and initialized to false. When c9911c93 (submodule--helper: teach update_data more options, 2022-03-15) moved it to update_data struct, it started to initialize it to true but this change was not explained in its log message.
The member is set to true only when pathspec was given, and is used when a submodule that matched the pathspec is found uninitialized to give diagnostic message. "submodule update" without pathspec is supposed to iterate over all submodules (i.e. without pathspec limitation) and update only the initialized submodules, and finding uninitialized submodules during the iteration is a totally expected and normal thing that should not be warned.
Signed-off-by Orgad Shaneh <[email protected]>
---
submodule--helper: fix initialization of warn_if_uninitialized
The .warn_if_uninitialized member was introduced by 48308681 (git submodule update: have a dedicated helper for cloning, 2016-02-29) to submodule_update_clone struct and initialized to false. When c9911c93 (submodule--helper: teach update_data more options, 2022-03-15) moved it to update_data struct, it started to initialize it to true but this change was not explained in its log message.
The member is set to true only when pathspec was given, and is used when a submodule that matched the pathspec is found uninitialized to give diagnostic message. "submodule update" without pathspec is supposed to iterate over all submodules (i.e. without pathspec limitation) and update only the initialized submodules, and finding uninitialized submodules during the iteration is a totally expected and normal thing that should not be warned.
Signed-off-by Orgad Shaneh [email protected]
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1258%2Forgads%2Fsub-no-warn-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1258/orgads/sub-no-warn-v2
Pull-Request: https://github.com/git/git/pull/1258
Range-diff vs v1:
1: 089a52a50b4 ! 1: 1e34c9cad18 submodule--helper: fix initialization of warn_if_uninitialized
@@ Metadata
## Commit message ##
submodule--helper: fix initialization of warn_if_uninitialized
- This field is supposed to be off by default, and it is only enabled when
- running git submodule update <path>, and path is not initialized.
+ The .warn_if_uninitialized member was introduced by 48308681
+ (git submodule update: have a dedicated helper for cloning,
+ 2016-02-29) to submodule_update_clone struct and initialized to
+ false. When c9911c93 (submodule--helper: teach update_data more
+ options, 2022-03-15) moved it to update_data struct, it started
+ to initialize it to true but this change was not explained in
+ its log message.
- Commit c9911c9358 changed it to enabled by default. This affects for
- example git checkout, which displays the following warning for each
- uninitialized submodule:
-
- Submodule path 'sub' not initialized
- Maybe you want to use 'update --init'?
-
- Amends c9911c9358e611390e2444f718c73900d17d3d60.
+ The member is set to true only when pathspec was given, and is
+ used when a submodule that matched the pathspec is found
+ uninitialized to give diagnostic message. "submodule update"
+ without pathspec is supposed to iterate over all submodules
+ (i.e. without pathspec limitation) and update only the
+ initialized submodules, and finding uninitialized submodules
+ during the iteration is a totally expected and normal thing that
+ should not be warned.
Signed-off-by Orgad Shaneh <[email protected]>
@@ builtin/submodule--helper.c: struct update_data {
.single_branch = -1, \
.max_jobs = 1, \
- .warn_if_uninitialized = 1, \
-+ .warn_if_uninitialized = 0, \
}
static void next_submodule_warn_missing(struct submodule_update_clone *suc,
 builtin/submodule--helper.c | 1 -
 1 file changed, 1 deletion(-)
builtin/submodule--helper.c +0 -1
index 2c87ef9364f..1a8e5d06214 100644
@@ -2026,7 +2026,6 @@ struct update_data {
 	.references = STRING_LIST_INIT_DUP, \
 	.single_branch = -1, \
 	.max_jobs = 1, \
-	.warn_if_uninitialized = 1, \
 }
 
 static void next_submodule_warn_missing(struct submodule_update_clone *suc,

Quoted 3 lines

base-commit: 6cd33dceed60949e2dbc32e3f0f5e67c4c882e1e
Signature · 1 line
gitgitgadget

This was clearly a mistake on my part :( The fix looks good to me, thanks!

Junio C Hamano

2022-04-27 23:21 UTC #10 lore raw

Quoted 12 lines · Glen Choo

Quoted 11 lines · Orgad Shaneh via GitGitGadget

builtin/submodule--helper.c +0 -1

index 2c87ef9364f..1a8e5d06214 100644
@@ -2026,7 +2026,6 @@ struct update_data {
 	.references = STRING_LIST_INIT_DUP, \
 	.single_branch = -1, \
 	.max_jobs = 1, \
-	.warn_if_uninitialized = 1, \
 }

Quoted 8 lines

Quoted 5 lines
builtin/submodule--helper.c +0 -0
 static void next_submodule_warn_missing(struct submodule_update_clone *suc,
base-commit: 6cd33dceed60949e2dbc32e3f0f5e67c4c882e1e
Signature · 1 line
gitgitgadget
This was clearly a mistake on my part :( The fix looks good to me, thanks!

Will merge the fix down in preparation for 2.36.1 and later.

Thanks, both.