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