On Mon, Feb 28, 2022 at 11:06:07PM -0800, Junio C Hamano wrote:
>
> Emily Shaffer <emilyshaffer@xxxxxxxxxx> writes:
>
> > diff --git a/submodule.c b/submodule.c
> > index 741104af8a..463e7f0c48 100644
> > --- a/submodule.c
> > +++ b/submodule.c
> > @@ -2237,6 +2237,7 @@ int get_superproject_working_tree(struct strbuf *buf)
> > struct strbuf sb = STRBUF_INIT;
> > struct strbuf one_up = STRBUF_INIT;
> > const char *cwd = xgetcwd();
> > + int has_superproject_cfg = 0;
> > int ret = 0;
> > const char *subpath;
> > int code;
> > @@ -2250,6 +2251,17 @@ int get_superproject_working_tree(struct strbuf *buf)
> > */
> > return 0;
> >
> > + if (git_config_get_bool("submodule.hassuperproject", &has_superproject_cfg)
> > + || !has_superproject_cfg) {
>
> git_config_get_bool() returns 0 when it successfully finds the
> variable, so the above says "If submodule.hasSuperproject is not set
> at all, or if it is set to false, then..."
>
> > + /*
> > + * If we don't have a superproject, then we're probably not a
> > + * submodule. If this is failing and shouldn't be, investigate
> > + * why the config was never set.
> > + */
> > + error(_("Asked to find a superproject, but submodule.hasSuperproject != true"));
> > + return 0;
>
> But given that this thing is new, I am not sure if that is a
> sensible guard to use here. Shouldn't we say
>
> - If submodule.hasSuperproject is EXPLICITLY set to false then ...
Ah, interesting. I think that makes sense. I wrote this patch hoping to
get an additional check for completeness of the previous patch (that is
- if I can rely on that value for this other operation, and all the
tests still pass without me touching them, then I seem to have wired
the new config through everywhere) and I think it's served that purpose;
for the real world, that's a little more dangerous, so I think relying
on explicit false makes sense. Will do.
>
> instead? I.e.
>
> if (!git_config_get_bool("submodule.hassuperproject", &value) &&
> !value) {
> error(_("asked to ..."));
> return 0;
> }
>
