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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 24, 2024, 18:32 UTC
Message-ID: <xmqq7cb0ucm0.fsf@gitster.g>
In-Reply-To: <4ce463defa807fb99eef6ce7abcd758fc2065c13.1727185364.git.gitgitgadget@gmail.com>

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 34 quoted lines

> From: John Cai <johncai86@gmail.com>
>
> Remove the_repository global variable in favor of the repository
> argument that gets passed in through the builtin function.
>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
>  builtin/apply.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 84f1863d3ac..d0bafbec7e4 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -1,4 +1,3 @@
> -#define USE_THE_REPOSITORY_VARIABLE
>  #include "builtin.h"
>  #include "gettext.h"
>  #include "hash.h"
> @@ -12,14 +11,14 @@ static const char * const apply_usage[] = {
>  int cmd_apply(int argc,
>  	      const char **argv,
>  	      const char *prefix,
> -	      struct repository *repo UNUSED)
> +	      struct repository *repo)
>  {
>  	int force_apply = 0;
>  	int options = 0;
>  	int ret;
>  	struct apply_state state;
>  
> -	if (init_apply_state(&state, the_repository, prefix))
> +	if (init_apply_state(&state, repo, prefix))
>  		exit(128);

Hmph, the reason why we do not segfault with this patch is because repo will _always_ be the_repository due to the previous change.

I am not sure if [1/4] is an improvement, though. We used to be able to tell if we were running in a repository, or we were running in "nongit" mode, by looking at the NULL-ness of repo (which was UNUSED because we weren't taking advantage of that).

With [1/4], it no longer is possible. From the point of view of API to call into builtin implementations, it smells like a regression.

A more honest change for this hunk would rather be something like:

        -	if (init_apply_state(&state, the_repository, prefix))
        +	if (!repo)
        +		repo = the_repository;
        +	if (init_apply_state(&state, repo, prefix))

without [1/4]. This change does not address "apply still depends on having access to the_repository even when it is being used as a better GNU patch" issue at all.

So, no, while I earlier said I was happy with [1/4], I no longer am enthused by the change.