Re: [PATCH v2 3/4] apply: remove the_repository global variable

From: shejialuo <shejialuo@gmail.com>
Date: Oct 1, 2024, 04:58 UTC
Message-ID: <ZvuBduVg9TJeULpl@ArchLinux>
In-Reply-To: <xmqqy13852jk.fsf@gitster.g>

On Mon, Sep 30, 2024 at 01:06:55PM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> >  	/*
> > @@ -28,8 +27,8 @@ int cmd_apply(int argc,
> >  	 * is worth the effort.
> >  	 * cf. https://lore.kernel.org/git/xmqqcypfcmn4.fsf@gitster.g/
> >  	 */
> > -	if (!the_hash_algo)
> > -		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
> > +	if (!repo->hash_algo)
> > +		repo_set_hash_algo(repo, GIT_HASH_SHA1);
> 
> ... is this use of "repo" still valid?  We now pass NULL, not
> the_repository, when a command with SETUP_GENTLY is asked to run
> outside a repository, no?  Shouldn't it detecting the case, and
> passing the pointer to a fallback object (perhaps the_repository)
> instead of repo?
>

This is a bad usage. Although the "t1517: apply a patch outside repository" should check the code, the uninitialized variable "repo_exists" will cause "repo_exists ? repo : NULL" to always be "repo" which hides the wrong usage of the "repo_exists".

By fetching the tree, I initialize the "repo_exists = 0" for the [PATCH v2 1/4]. And there are many tests failed. Many builtins with "RUN_SETUP_GENTLY" property or which could be converted to "RUN_SETUP_GENTLY" property by ONLY "-h" parameter will fail (segmentation fault). It's obvious that we use NULL pointer for "repo".

In my opinion, we should first think about how we handle the situation where we run builtins outside of the repository. The most easiest way is to pass the fallback object (aka "the_repository").

However, this seems a little strange. We are truly outside of the repository but we really rely on the "struct repository *" to do many operations. It's unrealistic to change so many interfaces which use the "struct repository *". So, we should just use the fallback idea at current.

Thanks, Jialuo