Re: [PATCH v3 0/3] submodule: port subcommand add from shell to C

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 15, 2020, 21:44 UTC
Message-ID: <xmqqlfdy7niy.fsf@gitster.c.googlers.com>
In-Reply-To: <20201214231939.644175-1-periperidip@gmail.com>

Shourya Shukla <periperidip@gmail.com> writes:

Show 15 quoted lines

>     3. In the following segment:
>         /*
>          * NEEDSWORK: In a multi-working-tree world, this needs to be
>          * set in the per-worktree config.
>          */
>         if (!git_config_get_string("submodule.active", &var) && var) {
>
>         There was a comment: "What if this were a valueless true
>         ("[submodule] active\n" without "= true")?  Wouldn't get_string()
>         fail?"
>
>         I was under the impression that even if the above failed, it
>         will not really affect the big picture since at the we will set
>         'submodule.name.active" as true irrespective of the above value.
>         Is this correct?

Let's see what kind of value the "submodule.active" variable is meant to be set to. Documentation/config/submodule.txt has this:

    submodule.active::
            A repeated field which contains a pathspec used to match against a
            submodule's path to determine if the submodule is of interest to git
            commands. See linkgit:gitsubmodules[7] for details.

It definitely is a string value, and making it a valueless true is an error in the configuration. I wonder if we want to diagnose such an error, or can we just pretend we didn't see it and keep going?

Also the "var" (one of the values set for this multi-valued variable) is never used in the body of the "if" statement. The other user of "submodule.active" in module_init() seems to use config_get_value_multi() on it. The new code may deserve a comment to explain why that is OK to (1) grab just a single value out of the multi-valued variable, and (2) not even look at its value.