Re: [PATCH] submodule--helper: fix initialization of warn_if_uninitialized

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

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