Re: [PATCH v9] builtin/clone.c: add --reject-shallow option

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 31, 2021, 22:24 UTC
Message-ID: <nycvar.QRO.7.76.6.2104010021510.54@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqim576spj.fsf@gitster.g>

Hi Junio,

On Wed, 31 Mar 2021, Junio C Hamano wrote:

Show 18 quoted lines

> > @@ -858,6 +861,9 @@ static int git_clone_config(const char *k, const char *v, void *cb)
> >  		free(remote_name);
> >  		remote_name = xstrdup(v);
> >  	}
> > +	if (!strcmp(k, "clone.rejectshallow") && option_reject_shallow < 0)
> > +		option_reject_shallow = git_config_bool(k, v);
>
> Does this "single variable is enough" really work?
>
> Imagine that the first time around we'd read from $HOME/.gitconfig
> that says true (flips the variable from "unspecified").  Further
> imagine that we are running "git clone -c config.rejectShallow=no"
> to countermand the global config.  We call write_config() to write
> the extra configuration value out, and then call git_config() to
> read from the repository configuration again.
>
> Because of the value taken from $HOME/.gitconfig, however, the
> attempt to override is silently ignored, isn't it?

That's my mistake, and you don't even need a second config to see the problem:

	[clone]
		rejectShallow = false
		rejectShallow = true

Clearly, the second one should override the first one, but the code I suggested (and which was integrated into this iteration) simply does not work correctly.

So yes, I think you're right, and we need to have that second variable, and then do something like this:

	if (option_reject_shallow < 0)
		option_reject_shallow = config_reject_shallow;

Sorry for the trouble! Dscho