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

[shejialuo <shejialuo@gmail.com>]

On Mon, Sep 30, 2024 at 01:06:55PM -0700, Junio C Hamano wrote:
> >  	/*
> > @@ -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