[3/4] apply: remove the_repository global variable

Commit Message

John Cai Sept. 24, 2024, 1:42 p.m. UTC

From: John Cai <[e-mail]>

Remove the_repository global variable in favor of the repository
argument that gets passed in through the builtin function.

Signed-off-by: John Cai <[e-mail]>
---
 builtin/apply.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Junio C Hamano Sept. 24, 2024, 6:32 p.m. UTC | #1

"John Cai via GitGitGadget" <[e-mail]> writes:

> From: John Cai <[e-mail]>
>
> Remove the_repository global variable in favor of the repository
> argument that gets passed in through the builtin function.
>
> Signed-off-by: John Cai <[e-mail]>
> ---
>  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.

Junio C Hamano Sept. 24, 2024, 6:50 p.m. UTC | #2

Junio C Hamano <[e-mail]> writes:

>> -	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.

We can avoid the regression by passing the discovered "nongit" (aka
"are we outside of a repository?") bit separately, perhaps like
this.  With such a change, I do not mind this change too much, but
pretending that we do not depend on the_repository (by removing the
textual mention of the_repository), but still depending on
the_repository (which points at the_repo) may be losing a bigger
picture, the true reason why we want to reduce the dependence on
the_repository.  We still need "if the hash-algo is not initialized
fall back to SHA-1" code here, but that is an overly broad fallback
that we would rather want to tighten to something like "we know we
have no reasonable value to initialize hash-algo in the_repository
if we are outside a repository, so initialize hash-algo if we are
outside any repository" (leaving it an error not have hash-algo in
"repo" if we _are_ in a repository).
   
diff --git c/git.c w/git.c
index 2fbea24ec9..579c6fa36d 100644
--- c/git.c
+++ w/git.c
@@ -447,6 +447,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
 	struct stat st;
 	const char *prefix;
 	int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));
+	int nongit = 0;
 
 	help = argc == 2 && !strcmp(argv[1], "-h");
 	if (help && (run_setup & RUN_SETUP))
@@ -456,8 +457,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
 	if (run_setup & RUN_SETUP) {
 		prefix = setup_git_directory();
 	} else if (run_setup & RUN_SETUP_GENTLY) {
-		int nongit_ok;
-		prefix = setup_git_directory_gently(&nongit_ok);
+		prefix = setup_git_directory_gently(&nongit);
 	} else {
 		prefix = NULL;
 	}
@@ -480,7 +480,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
 	trace2_cmd_name(p->cmd);
 
 	validate_cache_entries(repo->index);
-	status = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);
+	status = p->fn(argc, argv, prefix, run_setup ? repo : NULL, nongit);
 	validate_cache_entries(repo->index);
 
 	if (status)

John Cai Sept. 26, 2024, 6:59 p.m. UTC | #3

Hi Junio,

On Tue, Sep 24, 2024 at 2:32 PM Junio C Hamano <[e-mail]> wrote:
>
> "John Cai via GitGitGadget" <[e-mail]> writes:
>
> > From: John Cai <[e-mail]>
> >
> > Remove the_repository global variable in favor of the repository
> > argument that gets passed in through the builtin function.
> >
> > Signed-off-by: John Cai <[e-mail]>
> > ---
> >  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.

I see your point here. However, I was wondering about this because
we are passing in the_repository through run_builtin() as repo--so wouldn't
this be equivalent to using the_repository and hence the
same API contract can remain that looks at the NULL-ness of repo?

But I could be missing something here.

thanks!
John

>
> 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.

Junio C Hamano Sept. 26, 2024, 7:17 p.m. UTC | #4

John Cai <[e-mail]> writes:

>> > -     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.
>
> I see your point here. However, I was wondering about this because
> we are passing in the_repository through run_builtin() as repo--so wouldn't
> this be equivalent to using the_repository and hence the
> same API contract can remain that looks at the NULL-ness of repo?
>
> But I could be missing something here.

As run_builtin() discards the value of nongit, we will always see
repo == the_repository passed to this function, whether _gently()
found that we are in or not in a repository.  I think Patrick also
noticed it and suggested to pass repo or NULL conditionally in a
separate message, and if that is done, then I am fine.  I do not
think your [1/4] as-is did that.

Thanks.

Patch

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);
 
 	/*
@@ -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);
 
 	argc = apply_parse_options(argc, argv,
 				   &state, &force_apply, &options,