On Tue, Nov 15 2022, Johannes Schindelin wrote: > Hi Ævar, > > On Tue, 15 Nov 2022, Johannes Schindelin wrote: > >> On Tue, 15 Nov 2022, Ævar Arnfjörð Bjarmason wrote: >> >> > In 1f80129d61b (maintenance: add option to register in a specific >> > config, 2022-11-09) code was added which triggers a >> > "-Wdeclaration-after-statement" warning, which is on by default with >> > DEVELOPER=1. >> > >> > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> >> > --- >> > >> > This landed in the latest "next" push-out, causing e.g this CI >> > failure: >> > https://github.com/git/git/actions/runs/3467538041/jobs/5792504315 >> >> I looked at the same thing all morning, and found that >> https://lore.kernel.org/git/20221111231910.26769-1-ronan@xxxxxx/ >> _probably_ was designed to fix the same issue (and I think you agree that >> that patch should be used instead of yours because it is more complete in >> fixing left-over issues). >> >> However, try as I might, I did not find out yet why it does not apply >> cleanly over here (I got side-tracked into range-diff'ing patches >> downloaded from the archive, which is quite the challenge and not even fun >> because `range-diff` requires commits, not emails, so I side-tracked >> myself into teaching `range-diff` to accept mbox arguments). >> >> Maybe you can adjust that patch so it applies cleanly? > > Seems that the v2 of "maintenance: improve error reporting for > unregister" [*1*] was _partially_ folded into v3 of "maintenance: add > option to register in a specific config": At least the config change that > talks about `--config-file` inste of `--config` _is_ part of v3, and one > reason that the former patch does not apply cleanly on top of the latter > patch. Which is a bit funny because the former patch was sent out two days > _after_ the latter patch. > > And it looks as if there is one more thing that needs to be forward-ported > from that no-longer-applying patch: the initialization of the configset, > otherwise we're potentially trying to clear an uninitialized data > structure. > > This is the patch I am currently testing (on top of Git for Windows' > `shears/seen` branch: > https://github.com/git-for-windows/git/commit/cd7b86d19f): > > -- snip -- > diff --git a/builtin/gc.c b/builtin/gc.c > index 635f12499d68..a3f63880dfba 100644 > --- a/builtin/gc.c > +++ b/builtin/gc.c > @@ -1569,6 +1569,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi > int found = 0; > struct string_list_item *item; > const struct string_list *list; > + struct config_set cs; > > argc = parse_options(argc, argv, prefix, options, > builtin_maintenance_unregister_usage, 0); > @@ -1576,9 +1577,8 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi > usage_with_options(builtin_maintenance_unregister_usage, > options); > > - struct config_set cs; > + git_configset_init(&cs); > if (config_file) { > - git_configset_init(&cs); > git_configset_add_file(&cs, config_file); > list = git_configset_get_value_multi(&cs, key); > } else { > -- snap -- > > Ævar, can you please have a thorough look and see whether there is > anything else we were missing before advancing this to `next`? Thanks, I submitted a v2 just now that addresses the unit'd issue as well, and as we're fixing that we can just reduce the scope of the variable, which makes it lifetime more obvious: https://lore.kernel.org/git/patch-v2-1.1-f37e99c9d59-20221115T160240Z-avarab@xxxxxxxxx/
