Re: [PATCH v8 3/3] rev-parse: short-circuit superproject worktree when config unset

From: Emily Shaffer <emilyshaffer@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v8 3/3] rev-parse: short-circuit superproject worktree when config unset
Date: Tue, 8 Mar 2022 16:38:51 -0800	[thread overview]
Message-ID: <Yif3G5zxP9kj/Lkc@google.com> (raw)
In-Reply-To: <xmqq7d9eup7k.fsf@gitster.g>

On Mon, Feb 28, 2022 at 11:06:07PM -0800, Junio C Hamano wrote:
> 
> Emily Shaffer <emilyshaffer@google.com> 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;
> 	}
>