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

From: Shourya Shukla <periperidip@gmail.com>
Date: Dec 17, 2020, 14:16 UTC
Message-ID: <20201217141625.GA7638@konoha>
In-Reply-To: <xmqqlfdy7niy.fsf@gitster.c.googlers.com>

On 15/12 01:44, Junio C Hamano wrote:

Show 28 quoted lines

> Shourya Shukla <periperidip@gmail.com> writes:
> 
> >     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 think that we did not _make_ it a valueless true. It was already there and we somehow managed to check it. If you mean that we should ensure that we set it to "true" so that any such errors don't happen later on, then that is a different thing.

> I wonder if we want to diagnose such
> an error, or can we just pretend we didn't see it and keep going?

I guess we could pretend we did not see it since it isn't affecting the run of the sub-command. If you think otherwise, please suggest.

Show 6 quoted lines

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

Understood. So a comment along the lines of:

	/*
	 * Since we are fetching information only about one submodule,
	 * we need not fetch a  list of submodules to check the activity
	 * status of a single submodule.
	 *
	 * In case of a valueless true, i.e, '[submodule] active\n'
	 * without '= true', we need not worry about any errors since
	 * irrespective of the above value, we will set
	 * 'submodule.<name>.active' as true.
	 */

will work? Also, could you please comment on the other two issues I mentioned in the cover letter so I might as well start work on v4 of this patch?

Regards, Shourya Shukla