submodule--helper: fix initialization of warn_if_uninitialized #1258

orgads · 2022-04-24T06:22:56Z

orgads · 2022-04-24T06:25:21Z

/submit

gitgitgadget-git · 2022-04-24T06:26:40Z

Submitted as pull.1258.git.git.1650781575173.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1258/orgads/sub-no-warn-v1

To fetch this version to local tag pr-git-1258/orgads/sub-no-warn-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1258/orgads/sub-no-warn-v1

gitgitgadget-git · 2022-04-25T09:39:12Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Orgad Shaneh via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Orgad Shaneh <orgads@gmail.com>
>
> 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.

> 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.

> 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).

>
> Signed-off-by: Orgad Shaneh <orgads@gmail.com>
> ---
>     submodule--helper: fix initialization of warn_if_uninitialized
> ...
>     Signed-off-by: Orgad Shaneh orgads@gmail.com

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.

> 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(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 2c87ef9364f..b28112e3040 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -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.

The .warn_if_uninitialized member was introduced by 4830868 (git submodule update: have a dedicated helper for cloning, 2016-02-29) to submodule_update_clone struct and initialized to false. When c9911c9 (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 <orgads@gmail.com>

orgads · 2022-04-25T12:44:47Z

/submit

gitgitgadget-git · 2022-04-25T12:45:57Z

Submitted as pull.1258.v2.git.git.1650890741430.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1258/orgads/sub-no-warn-v2

To fetch this version to local tag pr-git-1258/orgads/sub-no-warn-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1258/orgads/sub-no-warn-v2

gitgitgadget-git · 2022-04-25T12:48:04Z

On the Git mailing list, Orgad Shaneh wrote (reply to this):

On Mon, Apr 25, 2022 at 12:34 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Orgad Shaneh via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Orgad Shaneh <orgads@gmail.com>
> >
> > 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.
>
> > 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.

Thank you very much. Updated.

> > 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).

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

> > ---
> >     submodule--helper: fix initialization of warn_if_uninitialized
> > ...
> >     Signed-off-by: Orgad Shaneh orgads@gmail.com
>
> 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.

> > 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(-)
> >
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index 2c87ef9364f..b28112e3040 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -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

phil-blain · 2022-04-25T13:27:51Z

Hi @orgads, you can avoid the duplicated message by using an empty PR description here on GitGitgadget :)

phil-blain · 2022-04-25T13:28:54Z

See gitgitgadget/gitgitgadget#23

phil-blain · 2022-04-25T13:34:24Z

Though for later iterations, v2, v3, etc, it is recommended to use the PR description to explain what has changed since the last iteration, see https://git-scm.com/docs/MyFirstContribution#responding-ggg

orgads · 2022-04-25T13:36:13Z

Thank you.

gitgitgadget-git · 2022-04-25T18:30:12Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Orgad Shaneh via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 2c87ef9364f..1a8e5d06214 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -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.

gitgitgadget-git · 2022-04-25T20:34:45Z

This branch is now known as gc/submodule-update-part2.

gitgitgadget-git · 2022-04-25T20:34:45Z

This patch series was integrated into seen via ebb5e63.

gitgitgadget-git · 2022-04-25T21:00:31Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <gitster@pobox.com> writes:

> 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(+)

diff --git c/t/t7406-submodule-update.sh w/t/t7406-submodule-update.sh
index 000e055811..43f779d751 100755
--- c/t/t7406-submodule-update.sh
+++ w/t/t7406-submodule-update.sh
@@ -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 &&

gitgitgadget-git · 2022-04-25T23:36:31Z

This patch series was integrated into seen via 3ee7beb.

gitgitgadget-git · 2022-04-26T01:26:54Z

There was a status update in the "New Topics" section about the branch gc/submodule-update-part2 on the Git mailing list:

"git submodule update" without pathspec should silently skip an
uninitialized submodule, but it started to become noisy by mistake.

Will merge to 'next' and then to 'master'.
source: <pull.1258.v2.git.git.1650890741430.gitgitgadget@gmail.com>

gitgitgadget-git · 2022-04-26T17:19:28Z

This patch series was integrated into seen via b4526f0.

gitgitgadget-git · 2022-04-27T22:24:44Z

On the Git mailing list, Glen Choo wrote (reply to this):

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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.

Thanks for adding the tests!

>  t/t7406-submodule-update.sh | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git c/t/t7406-submodule-update.sh w/t/t7406-submodule-update.sh
> index 000e055811..43f779d751 100755
> --- c/t/t7406-submodule-update.sh
> +++ w/t/t7406-submodule-update.sh
> @@ -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 &&

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!

gitgitgadget-git · 2022-04-27T22:24:47Z

User Glen Choo <chooglen@google.com> has been added to the cc: list.

gitgitgadget-git · 2022-04-27T22:24:48Z

On the Git mailing list, Glen Choo wrote (reply to this):

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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.

Thanks for adding the tests!

>  t/t7406-submodule-update.sh | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git c/t/t7406-submodule-update.sh w/t/t7406-submodule-update.sh
> index 000e055811..43f779d751 100755
> --- c/t/t7406-submodule-update.sh
> +++ w/t/t7406-submodule-update.sh
> @@ -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 &&

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!

gitgitgadget-git · 2022-04-27T22:27:52Z

On the Git mailing list, Glen Choo wrote (reply to this):

"Orgad Shaneh via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Orgad Shaneh <orgads@gmail.com>
>
> 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 <orgads@gmail.com>
> ---
>     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 orgads@gmail.com
>
> 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 <orgads@gmail.com>
>       
>      @@ 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(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 2c87ef9364f..1a8e5d06214 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -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
> -- 
> gitgitgadget

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

gitgitgadget-git · 2022-04-27T22:27:53Z

On the Git mailing list, Glen Choo wrote (reply to this):

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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.

Thanks for adding the tests!

>  t/t7406-submodule-update.sh | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git c/t/t7406-submodule-update.sh w/t/t7406-submodule-update.sh
> index 000e055811..43f779d751 100755
> --- c/t/t7406-submodule-update.sh
> +++ w/t/t7406-submodule-update.sh
> @@ -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 &&

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!

gitgitgadget-git · 2022-04-27T23:24:48Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

Glen Choo <chooglen@google.com> writes:

> "Orgad Shaneh via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 2c87ef9364f..1a8e5d06214 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -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
>> -- 
>> 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.

gitgitgadget-git · 2022-04-28T20:02:03Z

There was a status update in the "Graduated to 'master'" section about the branch gc/submodule-update-part2 on the Git mailing list:

"git submodule update" without pathspec should silently skip an
uninitialized submodule, but it started to become noisy by mistake.

This fixes a regression in 2.36 and is slate to go to 2.36.1
source: <pull.1258.v2.git.git.1650890741430.gitgitgadget@gmail.com>

phil-blain · 2022-05-08T16:55:48Z

@dscho any idea why this PR does not have a master label, even though the branch was merged to master?

dscho · 2022-05-10T19:24:27Z

@dscho any idea why this PR does not have a master label, even though the branch was merged to master?

Honestly, I have no idea... GitGitGadget probably has the wrong idea of the tip commit. When that happens, I kind of hope for the original contributors to notice and close the PR manually.

@orgads can this be closed?

orgads · 2022-05-10T19:47:21Z

Sure, thanks.

orgads force-pushed the sub-no-warn branch from dcf698a to 089a52a Compare April 24, 2022 06:25

orgads force-pushed the sub-no-warn branch from 089a52a to 1e34c9c Compare April 25, 2022 12:41

gitgitgadget-git bot added the seen label Apr 25, 2022

orgads closed this May 10, 2022

submodule--helper: fix initialization of warn_if_uninitialized #1258

submodule--helper: fix initialization of warn_if_uninitialized #1258

Uh oh!

Conversation

orgads commented Apr 24, 2022 • edited by gitgitgadget-git bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orgads commented Apr 24, 2022

Uh oh!

gitgitgadget-git bot commented Apr 24, 2022

Uh oh!

gitgitgadget-git bot commented Apr 25, 2022

Uh oh!

orgads commented Apr 25, 2022

Uh oh!

gitgitgadget-git bot commented Apr 25, 2022

Uh oh!

gitgitgadget-git bot commented Apr 25, 2022

Uh oh!

phil-blain commented Apr 25, 2022

Uh oh!

phil-blain commented Apr 25, 2022

Uh oh!

phil-blain commented Apr 25, 2022

Uh oh!

orgads commented Apr 25, 2022

Uh oh!

gitgitgadget-git bot commented Apr 25, 2022

Uh oh!

gitgitgadget-git bot commented Apr 25, 2022

Uh oh!

gitgitgadget-git bot commented Apr 25, 2022

Uh oh!

gitgitgadget-git bot commented Apr 25, 2022

Uh oh!

gitgitgadget-git bot commented Apr 25, 2022

Uh oh!

gitgitgadget-git bot commented Apr 26, 2022

Uh oh!

gitgitgadget-git bot commented Apr 26, 2022

Uh oh!

gitgitgadget-git bot commented Apr 27, 2022

Uh oh!

gitgitgadget-git bot commented Apr 27, 2022

Uh oh!

gitgitgadget-git bot commented Apr 27, 2022

Uh oh!

gitgitgadget-git bot commented Apr 27, 2022

Uh oh!

gitgitgadget-git bot commented Apr 27, 2022

Uh oh!

gitgitgadget-git bot commented Apr 27, 2022

Uh oh!

gitgitgadget-git bot commented Apr 28, 2022

Uh oh!

phil-blain commented May 8, 2022

Uh oh!

dscho commented May 10, 2022

Uh oh!

orgads commented May 10, 2022

Uh oh!

Uh oh!

orgads commented Apr 24, 2022 •

edited by gitgitgadget-git bot

Loading