[PATCH 0/4] Remove the_repository global for am, annotate, apply, archive builtins

[PATCH 0/4] Remove the_repository global for am, annotate, apply, archive builtins

[John Cai via GitGitGadget]

Remove the_repository global variable for the annotate, apply, and archive
bulitins.

John Cai (4):
  git: pass in repo for RUN_SETUP_GENTLY
  annotate: remove usage of the_repository global
  apply: remove the_repository global variable
  archive: remove the_repository global variable

 builtin/annotate.c | 5 ++---
 builtin/apply.c    | 9 ++++-----
 builtin/archive.c  | 5 ++---
 git.c              | 5 ++++-
 4 files changed, 12 insertions(+), 12 deletions(-)


base-commit: 6258f68c3c1092c901337895c864073dcdea9213
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1788%2Fjohn-cai%2Fjc%2Fremove-global-repo-a-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1788/john-cai/jc/remove-global-repo-a-v1
Pull-Request: https://github.com/git/git/pull/1788
-- 
gitgitgadget

[PATCH 1/4] git: pass in repo for RUN_SETUP_GENTLY

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

commands that have RUN_SETUP_GENTLY potentially need a repository.
Modify the logic in run_builtin() to pass the repository to the builtin
if a builtin has the RUN_SETUP_GENTLY property.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 git.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/git.c b/git.c
index 2fbea24ec92..e31b52dcc50 100644
--- a/git.c
+++ b/git.c
@@ -480,7 +480,10 @@ 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,
+		       ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL);
 	validate_cache_entries(repo->index);
 
 	if (status)
-- 
gitgitgadget

Re: [PATCH 1/4] git: pass in repo for RUN_SETUP_GENTLY

[shejialuo]

On Tue, Sep 24, 2024 at 01:42:41PM +0000, John Cai via GitGitGadget wrote:

[snip]

> diff --git a/git.c b/git.c
> index 2fbea24ec92..e31b52dcc50 100644
> --- a/git.c
> +++ b/git.c
> @@ -480,7 +480,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
>  	trace2_cmd_name(p->cmd);

This line is a little long, we may clean this in this patch.

>  
>  	validate_cache_entries(repo->index);
> -	status = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);
> +	status = p->fn(argc,
> +		       argv,
> +		       prefix,
> +		       ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL);

This reads so strange, could we create a new variable here?

Small problems, don't worth a reroll.

Thanks,
Jialuo

Re: [PATCH 1/4] git: pass in repo for RUN_SETUP_GENTLY

[Junio C Hamano]

shejialuo <shejialuo@gmail.com> writes:

> On Tue, Sep 24, 2024 at 01:42:41PM +0000, John Cai via GitGitGadget wrote:
>
> [snip]
>
>> diff --git a/git.c b/git.c
>> index 2fbea24ec92..e31b52dcc50 100644
>> --- a/git.c
>> +++ b/git.c
>> @@ -480,7 +480,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
>>  	trace2_cmd_name(p->cmd);
>
> This line is a little long, we may clean this in this patch.
>
>>  
>>  	validate_cache_entries(repo->index);
>> -	status = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);
>> +	status = p->fn(argc,
>> +		       argv,
>> +		       prefix,
>> +		       ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL);
>
> This reads so strange, could we create a new variable here?
>
> Small problems, don't worth a reroll.

If it is so annoying to make your reading hiccup, it is not small at
all.  One argument per line is so strange, and one plausible fix
would be

	status = p->fn(argc, argv, prefix,
        	       (p->option & (RUN_SETUP|RUN_SETUP_GENTLY))
		       ? repo : NULL);

Thanks.

Re: [PATCH 1/4] git: pass in repo for RUN_SETUP_GENTLY

[Junio C Hamano]

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

> From: John Cai <johncai86@gmail.com>
>
> commands that have RUN_SETUP_GENTLY potentially need a repository.
> Modify the logic in run_builtin() to pass the repository to the builtin
> if a builtin has the RUN_SETUP_GENTLY property.

Ah, I remember mentioning this as a potiential future direction
while reviewing the other series that added the repository argument
to the cmd_foo() interface.

Nice to see the idea getting followed up.

Let's see how [Patches 2-4/4] can make effective use of this.

> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
>  git.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/git.c b/git.c
> index 2fbea24ec92..e31b52dcc50 100644
> --- a/git.c
> +++ b/git.c
> @@ -480,7 +480,10 @@ 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,
> +		       ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL);
>  	validate_cache_entries(repo->index);
>  
>  	if (status)

Re: [PATCH 1/4] git: pass in repo for RUN_SETUP_GENTLY

[Patrick Steinhardt]

On Tue, Sep 24, 2024 at 01:42:41PM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@gmail.com>
> 
> commands that have RUN_SETUP_GENTLY potentially need a repository.
> Modify the logic in run_builtin() to pass the repository to the builtin
> if a builtin has the RUN_SETUP_GENTLY property.
> 
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
>  git.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/git.c b/git.c
> index 2fbea24ec92..e31b52dcc50 100644
> --- a/git.c
> +++ b/git.c
> @@ -480,7 +480,10 @@ 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,
> +		       ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL);
>  	validate_cache_entries(repo->index);

Should we really pass `repo` unconditionally when `RUN_SETUP_GENTLY` was
requested? I'd think that we should rather pass `NULL` if we didn't find
a repository in that case. So this condition should likely be made
conditional, shouldn't it?

There's also a missing space between the closing brace and the ternary
questionmark.

Patrick

Re: [PATCH 1/4] git: pass in repo for RUN_SETUP_GENTLY

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> -	status = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);
>> +	status = p->fn(argc,
>> +		       argv,
>> +		       prefix,
>> +		       ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL);
>>  	validate_cache_entries(repo->index);
>
> Should we really pass `repo` unconditionally when `RUN_SETUP_GENTLY` was
> requested? I'd think that we should rather pass `NULL` if we didn't find
> a repository in that case. So this condition should likely be made
> conditional, shouldn't it?

Yeah, that is much much more preferrable than my earlier suggestion
to pass yet another Boolean parameter to p->fn().

> There's also a missing space between the closing brace and the ternary
> questionmark.

True, too.

[PATCH 2/4] annotate: remove usage of the_repository global

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

Remove the the_repository with the repository argument that gets passed
down through the builtin function.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 builtin/annotate.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/annotate.c b/builtin/annotate.c
index a99179fe4dd..ce3dfaafb28 100644
--- a/builtin/annotate.c
+++ b/builtin/annotate.c
@@ -4,7 +4,6 @@
  * Copyright (C) 2006 Ryan Anderson
  */
 
-#define USE_THE_REPOSITORY_VARIABLE
 #include "git-compat-util.h"
 #include "builtin.h"
 #include "strvec.h"
@@ -12,7 +11,7 @@
 int cmd_annotate(int argc,
 		 const char **argv,
 		 const char *prefix,
-		 struct repository *repo UNUSED)
+		 struct repository *repo)
 {
 	struct strvec args = STRVEC_INIT;
 	int i;
@@ -23,5 +22,5 @@ int cmd_annotate(int argc,
 		strvec_push(&args, argv[i]);
 	}
 
-	return cmd_blame(args.nr, args.v, prefix, the_repository);
+	return cmd_blame(args.nr, args.v, prefix, repo);
 }
-- 
gitgitgadget

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

[John Cai via GitGitGadget]

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);
 
 	/*
@@ -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,
-- 
gitgitgadget

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

[Junio C Hamano]

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

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> 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)

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

[John Cai]

Hi Junio,

On Tue, Sep 24, 2024 at 2:32 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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.

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.

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

[Junio C Hamano]

John Cai <johncai86@gmail.com> 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 4/4] archive: remove the_repository global variable

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

Replace the_repository with the repository argument that gets passed
down through the builtin function.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 builtin/archive.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index dc926d1a3df..13ea7308c8b 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -2,7 +2,6 @@
  * Copyright (c) 2006 Franck Bui-Huu
  * Copyright (c) 2006 Rene Scharfe
  */
-#define USE_THE_REPOSITORY_VARIABLE
 #include "builtin.h"
 #include "archive.h"
 #include "gettext.h"
@@ -79,7 +78,7 @@ static int run_remote_archiver(int argc, const char **argv,
 int cmd_archive(int argc,
 		const char **argv,
 		const char *prefix,
-		struct repository *repo UNUSED)
+		struct repository *repo)
 {
 	const char *exec = "git-upload-archive";
 	char *output = NULL;
@@ -110,7 +109,7 @@ int cmd_archive(int argc,
 
 	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
 
-	ret = write_archive(argc, argv, prefix, the_repository, output, 0);
+	ret = write_archive(argc, argv, prefix, repo, output, 0);
 
 out:
 	free(output);
-- 
gitgitgadget

Re: [PATCH 4/4] archive: remove the_repository global variable

[Junio C Hamano]

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

>  int cmd_archive(int argc,
>  		const char **argv,
>  		const char *prefix,
> -		struct repository *repo UNUSED)
> +		struct repository *repo)
>  {
>  	const char *exec = "git-upload-archive";
>  	char *output = NULL;
> @@ -110,7 +109,7 @@ int cmd_archive(int argc,
>  
>  	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
>  
> -	ret = write_archive(argc, argv, prefix, the_repository, output, 0);
> +	ret = write_archive(argc, argv, prefix, repo, output, 0);

Exactly the same comment applies here as [3/4]

[PATCH v2 0/4] Remove the_repository global for am, annotate, apply, archive builtins

[John Cai via GitGitGadget]

Remove the_repository global variable for the annotate, apply, and archive
bulitins.

Changes since V1:

 * in patch 1, only pass in repo to the bulitin if the repo exists

John Cai (4):
  git: pass in repo for RUN_SETUP_GENTLY
  annotate: remove usage of the_repository global
  apply: remove the_repository global variable
  archive: remove the_repository global variable

 builtin/annotate.c |  5 ++---
 builtin/apply.c    |  9 ++++-----
 builtin/archive.c  |  5 ++---
 git.c              | 11 +++++++++--
 4 files changed, 17 insertions(+), 13 deletions(-)


base-commit: 3857aae53f3633b7de63ad640737c657387ae0c6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1788%2Fjohn-cai%2Fjc%2Fremove-global-repo-a-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1788/john-cai/jc/remove-global-repo-a-v2
Pull-Request: https://github.com/git/git/pull/1788

Range-diff vs v1:

 1:  eceb2d835be ! 1:  5d72c31c6f3 git: pass in repo for RUN_SETUP_GENTLY
     @@ Commit message
          Signed-off-by: John Cai <johncai86@gmail.com>
      
       ## git.c ##
     +@@ git.c: static int handle_alias(int *argcp, const char ***argv)
     + 
     + static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)
     + {
     +-	int status, help;
     ++	int status, help, repo_exists;
     + 	struct stat st;
     + 	const char *prefix;
     + 	int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));
     +@@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
     + 
     + 	if (run_setup & RUN_SETUP) {
     + 		prefix = setup_git_directory();
     ++		repo_exists = 1;
     + 	} else if (run_setup & RUN_SETUP_GENTLY) {
     + 		int nongit_ok;
     + 		prefix = setup_git_directory_gently(&nongit_ok);
     ++
     ++		if (!nongit_ok)
     ++			repo_exists = 1;
     + 	} else {
     + 		prefix = NULL;
     + 	}
      @@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
       	trace2_cmd_name(p->cmd);
       
     @@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv,
      +	status = p->fn(argc,
      +		       argv,
      +		       prefix,
     -+		       ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL);
     ++		       repo_exists ? repo : NULL);
       	validate_cache_entries(repo->index);
       
       	if (status)
 2:  1bf2b017dd3 = 2:  2a29d113815 annotate: remove usage of the_repository global
 3:  4ce463defa8 = 3:  d64955a2e27 apply: remove the_repository global variable
 4:  f6c32ec609c = 4:  857291d7f7d archive: remove the_repository global variable

-- 
gitgitgadget

[PATCH v2 1/4] git: pass in repo for RUN_SETUP_GENTLY

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

commands that have RUN_SETUP_GENTLY potentially need a repository.
Modify the logic in run_builtin() to pass the repository to the builtin
if a builtin has the RUN_SETUP_GENTLY property.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 git.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/git.c b/git.c
index 2fbea24ec92..f58f169f3c7 100644
--- a/git.c
+++ b/git.c
@@ -443,7 +443,7 @@ static int handle_alias(int *argcp, const char ***argv)
 
 static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)
 {
-	int status, help;
+	int status, help, repo_exists;
 	struct stat st;
 	const char *prefix;
 	int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));
@@ -455,9 +455,13 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
 
 	if (run_setup & RUN_SETUP) {
 		prefix = setup_git_directory();
+		repo_exists = 1;
 	} else if (run_setup & RUN_SETUP_GENTLY) {
 		int nongit_ok;
 		prefix = setup_git_directory_gently(&nongit_ok);
+
+		if (!nongit_ok)
+			repo_exists = 1;
 	} else {
 		prefix = NULL;
 	}
@@ -480,7 +484,10 @@ 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,
+		       repo_exists ? repo : NULL);
 	validate_cache_entries(repo->index);
 
 	if (status)
-- 
gitgitgadget

Re: [PATCH v2 1/4] git: pass in repo for RUN_SETUP_GENTLY

[Junio C Hamano]

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

> From: John Cai <johncai86@gmail.com>
>
> commands that have RUN_SETUP_GENTLY potentially need a repository.
> Modify the logic in run_builtin() to pass the repository to the builtin
> if a builtin has the RUN_SETUP_GENTLY property.
>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
>  git.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/git.c b/git.c
> index 2fbea24ec92..f58f169f3c7 100644
> --- a/git.c
> +++ b/git.c
> @@ -443,7 +443,7 @@ static int handle_alias(int *argcp, const char ***argv)
>  
>  static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)
>  {
> -	int status, help;
> +	int status, help, repo_exists;
>  	struct stat st;
>  	const char *prefix;
>  	int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));
> @@ -455,9 +455,13 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
>  
>  	if (run_setup & RUN_SETUP) {
>  		prefix = setup_git_directory();
> +		repo_exists = 1;
>  	} else if (run_setup & RUN_SETUP_GENTLY) {
>  		int nongit_ok;
>  		prefix = setup_git_directory_gently(&nongit_ok);
> +
> +		if (!nongit_ok)
> +			repo_exists = 1;

Why not use the new variable and pass it directly to nongit_ok?  The
polarity of the new variable needs to be swapped, of course, but I
think it makes reading the code to call p->fn() easier to grok

i.e., 

 - rename repo_exists to "no_repo", and initialize it to non-zero.
 - remove "int nongit_ok"; pass &no_repo instead.
 - update the calling of p->fn() to

	p->fn(argc, argv, prefix, no_repo ? NULL : repo);

>  	} else {
>  		prefix = NULL;
>  	}
> @@ -480,7 +484,10 @@ 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,
> +		       repo_exists ? repo : NULL);

Keeping the call to a single line would be much better than spreding
it across four lines.

Thanks

>  	validate_cache_entries(repo->index);
>  
>  	if (status)

Re: [PATCH v2 1/4] git: pass in repo for RUN_SETUP_GENTLY

[shejialuo]

On Mon, Sep 30, 2024 at 05:40:27PM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@gmail.com>
> 
> commands that have RUN_SETUP_GENTLY potentially need a repository.
> Modify the logic in run_builtin() to pass the repository to the builtin
> if a builtin has the RUN_SETUP_GENTLY property.
> 

We will parse the "repo" to the "run_builtin()" for "RUN_SETUP_GENTLY"
property only when we know we run the command in the repository. If we
run the command outside of the repository, we should pass the NULL.

However, the above commit message is not accurate. If a builtin has the
"RUN_SETUP_GENTLY" property. We may pass or not.

> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
>  git.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/git.c b/git.c
> index 2fbea24ec92..f58f169f3c7 100644
> --- a/git.c
> +++ b/git.c
> @@ -443,7 +443,7 @@ static int handle_alias(int *argcp, const char ***argv)
>  
>  static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)
>  {
> -	int status, help;
> +	int status, help, repo_exists;

This is wrong. We should initialize the "repo_exists" variable here.
Because we never set this variable to 0 in the later code path. It will
always be true for the following code:

    repo_exists ? repo : NULL

It will always evaluate to the "repo". This may could answer the
question raised by Junio in [PATCH v2 3/4].

Thanks,
Jialuo

[PATCH v2 2/4] annotate: remove usage of the_repository global

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

Remove the the_repository with the repository argument that gets passed
down through the builtin function.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 builtin/annotate.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/annotate.c b/builtin/annotate.c
index a99179fe4dd..ce3dfaafb28 100644
--- a/builtin/annotate.c
+++ b/builtin/annotate.c
@@ -4,7 +4,6 @@
  * Copyright (C) 2006 Ryan Anderson
  */
 
-#define USE_THE_REPOSITORY_VARIABLE
 #include "git-compat-util.h"
 #include "builtin.h"
 #include "strvec.h"
@@ -12,7 +11,7 @@
 int cmd_annotate(int argc,
 		 const char **argv,
 		 const char *prefix,
-		 struct repository *repo UNUSED)
+		 struct repository *repo)
 {
 	struct strvec args = STRVEC_INIT;
 	int i;
@@ -23,5 +22,5 @@ int cmd_annotate(int argc,
 		strvec_push(&args, argv[i]);
 	}
 
-	return cmd_blame(args.nr, args.v, prefix, the_repository);
+	return cmd_blame(args.nr, args.v, prefix, repo);
 }
-- 
gitgitgadget

Re: [PATCH v2 2/4] annotate: remove usage of the_repository global

[Junio C Hamano]

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

> From: John Cai <johncai86@gmail.com>
>
> Remove the the_repository with the repository argument that gets passed
> down through the builtin function.
>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
>  builtin/annotate.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/annotate.c b/builtin/annotate.c
> index a99179fe4dd..ce3dfaafb28 100644
> --- a/builtin/annotate.c
> +++ b/builtin/annotate.c
> @@ -4,7 +4,6 @@
>   * Copyright (C) 2006 Ryan Anderson
>   */
>  
> -#define USE_THE_REPOSITORY_VARIABLE
>  #include "git-compat-util.h"
>  #include "builtin.h"
>  #include "strvec.h"
> @@ -12,7 +11,7 @@
>  int cmd_annotate(int argc,
>  		 const char **argv,
>  		 const char *prefix,
> -		 struct repository *repo UNUSED)
> +		 struct repository *repo)
>  {
>  	struct strvec args = STRVEC_INIT;
>  	int i;
> @@ -23,5 +22,5 @@ int cmd_annotate(int argc,
>  		strvec_push(&args, argv[i]);
>  	}
>  
> -	return cmd_blame(args.nr, args.v, prefix, the_repository);
> +	return cmd_blame(args.nr, args.v, prefix, repo);
>  }

This looks obviously correct.  Nicely done.

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

[John Cai via GitGitGadget]

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);
 
 	/*
@@ -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,
-- 
gitgitgadget

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

[Junio C Hamano]

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

> 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);

Is this one, and ...

>  
>  	/*
> @@ -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?

I _think_ state->repo->index is accessed unconditionally only to
figure out whitespace attributes, even outside a repository (thanks
to the_repository standing in), with the expectation that the index
is empty (because we do not read any) and we find no customization,
when "git apply" is used as a better GNU patch to work outside any
repository.  Maybe I am not looking hard enough, but I fail to see
how the code makes sure that repo being NULL outside a repository
does not lead to a dereferencing of a NULL pointer.

Thanks.

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

[shejialuo]

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

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

[Patrick Steinhardt]

On Tue, Oct 01, 2024 at 12:58:30PM +0800, shejialuo wrote:
> On Mon, Sep 30, 2024 at 01:06:55PM -0700, Junio C Hamano wrote:
> 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.

I disagree with this statement. If code isn't prepare to not handle a
`NULL` repository we shouldn't fall back to `the_repository`, but we
should instead prepare the code to handle this case. This of course
requires us to do a ton of refactorings, but that is the idea of this
whole exercise to get rid of `the_repository`.

If a command cannot be converted to stop using `the_repository` right
now we should skip it and revisit once all prerequisites have been
adapted accordingly.

Patrick

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

[shejialuo]

On Tue, Oct 01, 2024 at 02:32:30PM +0200, Patrick Steinhardt wrote:
> On Tue, Oct 01, 2024 at 12:58:30PM +0800, shejialuo wrote:
> > On Mon, Sep 30, 2024 at 01:06:55PM -0700, Junio C Hamano wrote:
> > 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.
> 
> I disagree with this statement. If code isn't prepare to not handle a
> `NULL` repository we shouldn't fall back to `the_repository`, but we
> should instead prepare the code to handle this case. This of course
> requires us to do a ton of refactorings, but that is the idea of this
> whole exercise to get rid of `the_repository`.
> 

Actually, I also insist that we should refactor here. But I worry about
the burden this would bring to John due to we may do a lot of work here.
So, I expressed my meaning in a compromising way.

But we should face the problem directly :).

Thanks,
Jialuo

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

[Patrick Steinhardt]

On Tue, Oct 01, 2024 at 09:40:43PM +0800, shejialuo wrote:
> On Tue, Oct 01, 2024 at 02:32:30PM +0200, Patrick Steinhardt wrote:
> > On Tue, Oct 01, 2024 at 12:58:30PM +0800, shejialuo wrote:
> > > On Mon, Sep 30, 2024 at 01:06:55PM -0700, Junio C Hamano wrote:
> > > 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.
> > 
> > I disagree with this statement. If code isn't prepare to not handle a
> > `NULL` repository we shouldn't fall back to `the_repository`, but we
> > should instead prepare the code to handle this case. This of course
> > requires us to do a ton of refactorings, but that is the idea of this
> > whole exercise to get rid of `the_repository`.
> > 
> 
> Actually, I also insist that we should refactor here. But I worry about
> the burden this would bring to John due to we may do a lot of work here.
> So, I expressed my meaning in a compromising way.
> 
> But we should face the problem directly :).

True, all of this is a long-term effort that is probably going to take
us many months, likely even years. So people working on it should take
things slow and refactor chunks that are mostly ready to be converted to
get rid of `the_repository`.

That will sometimes mean that you have to scrap the conversion you're
currently working on because you discover that it inherently relies on
`the_repository` deep down in the stack, and refactoring it would be a
huge undertaking. That definitely happened to me multiple times while
introducing `USE_THE_REPOSITORY_VARIABLE`. And every time I did discover
that, I went one level deeper to try and fix the underpinnings first.

I mostly don't want us to blur the lines by silently falling back to
`the_repository` in situations where we don't intend to. So I'd rather
go a bit slower overall and design the code such that it doesn't fall
back anymore as a way to prove that something is indeed not relying on
`the_repository` anymore. Otherwise we're going to make everyones life
harder.

Patrick

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> I disagree with this statement. If code isn't prepare to not handle a
> `NULL` repository we shouldn't fall back to `the_repository`, but we
> should instead prepare the code to handle this case. This of course
> requires us to do a ton of refactorings, but that is the idea of this
> whole exercise to get rid of `the_repository`.

I agree.  To me, the patch was screaming that the author was not
prepared to go the whole nine yards, though.  Adding back the
explicit reference to "the_repository" as a fallback is the next
best thing to do, pushing the "problem" closer to where it is.

> If a command cannot be converted to stop using `the_repository` right
> now we should skip it and revisit once all prerequisites have been
> adapted accordingly.

That is also a viable approach.

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

[johncai86]

Hi Junio,

On 1 Oct 2024, at 13:10, Junio C Hamano wrote:

> Patrick Steinhardt <ps@pks.im> writes:
>
>> I disagree with this statement. If code isn't prepare to not handle a
>> `NULL` repository we shouldn't fall back to `the_repository`, but we
>> should instead prepare the code to handle this case. This of course
>> requires us to do a ton of refactorings, but that is the idea of this
>> whole exercise to get rid of `the_repository`.
>
> I agree.  To me, the patch was screaming that the author was not
> prepared to go the whole nine yards, though.  Adding back the
> explicit reference to "the_repository" as a fallback is the next
> best thing to do, pushing the "problem" closer to where it is.
>

Indeed, I did not do my due diligence here instead of assuming all layers
that look at the repo argument do the right thing.

>> If a command cannot be converted to stop using `the_repository` right
>> now we should skip it and revisit once all prerequisites have been
>> adapted accordingly.

Looks like it’d be preferable if I just drop this patch from the series as
it will require a larger refactor.

Thanks
John

>
> That is also a viable approach.

[PATCH v2 4/4] archive: remove the_repository global variable

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

Replace the_repository with the repository argument that gets passed
down through the builtin function.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 builtin/archive.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index dc926d1a3df..13ea7308c8b 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -2,7 +2,6 @@
  * Copyright (c) 2006 Franck Bui-Huu
  * Copyright (c) 2006 Rene Scharfe
  */
-#define USE_THE_REPOSITORY_VARIABLE
 #include "builtin.h"
 #include "archive.h"
 #include "gettext.h"
@@ -79,7 +78,7 @@ static int run_remote_archiver(int argc, const char **argv,
 int cmd_archive(int argc,
 		const char **argv,
 		const char *prefix,
-		struct repository *repo UNUSED)
+		struct repository *repo)
 {
 	const char *exec = "git-upload-archive";
 	char *output = NULL;
@@ -110,7 +109,7 @@ int cmd_archive(int argc,
 
 	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
 
-	ret = write_archive(argc, argv, prefix, the_repository, output, 0);
+	ret = write_archive(argc, argv, prefix, repo, output, 0);
 
 out:
 	free(output);
-- 
gitgitgadget

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

[Junio C Hamano]

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

> -#define USE_THE_REPOSITORY_VARIABLE
>  #include "builtin.h"
>  #include "archive.h"
>  #include "gettext.h"
> @@ -79,7 +78,7 @@ static int run_remote_archiver(int argc, const char **argv,
>  int cmd_archive(int argc,
>  		const char **argv,
>  		const char *prefix,
> -		struct repository *repo UNUSED)
> +		struct repository *repo)
>  {
>  	const char *exec = "git-upload-archive";
>  	char *output = NULL;
> @@ -110,7 +109,7 @@ int cmd_archive(int argc,
>  
>  	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
>  
> -	ret = write_archive(argc, argv, prefix, the_repository, output, 0);
> +	ret = write_archive(argc, argv, prefix, repo, output, 0);

This looks OK, but unlike the original, write_archive() now needs to
be prepared to see NULL in the repo parameter.  Is that reasonable?

Perdon me to think aloud for a bit.

The context before this hunk handles "git archive --remote" which
can be run outside a repository (and that is the whole reason why we
ask SETUP_GENTLY), but this part has to happen in a repository,
doesn't it?  Or is there some mode of operation of "git archive" I
am forgetting that can be done without a repository?

	... goes and looks ...

OK, write_archive() has its own setup_git_directory() call when
startup_info->have_repository is false, so this happens to be OK,
until the beginning part of archive.c:write_archive() will not
changed to start dereferencing "repo" pointer.

That sounds brittle, but probalby outside the scope of what this
patch series wants to address.  It also makes git_config() calls
even before it realizes there is no repository and dies, which
smells fishy without actually doing any harm.

So, after all, I think this step is probably OK.

Thanks.

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

[johncai86]

Hi Junio,

On 30 Sep 2024, at 16:01, Junio C Hamano wrote:

> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> -#define USE_THE_REPOSITORY_VARIABLE
>>  #include "builtin.h"
>>  #include "archive.h"
>>  #include "gettext.h"
>> @@ -79,7 +78,7 @@ static int run_remote_archiver(int argc, const char **argv,
>>  int cmd_archive(int argc,
>>  		const char **argv,
>>  		const char *prefix,
>> -		struct repository *repo UNUSED)
>> +		struct repository *repo)
>>  {
>>  	const char *exec = "git-upload-archive";
>>  	char *output = NULL;
>> @@ -110,7 +109,7 @@ int cmd_archive(int argc,
>>
>>  	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
>>
>> -	ret = write_archive(argc, argv, prefix, the_repository, output, 0);
>> +	ret = write_archive(argc, argv, prefix, repo, output, 0);
>
> This looks OK, but unlike the original, write_archive() now needs to
> be prepared to see NULL in the repo parameter.  Is that reasonable?
>
> Perdon me to think aloud for a bit.
>
> The context before this hunk handles "git archive --remote" which
> can be run outside a repository (and that is the whole reason why we
> ask SETUP_GENTLY), but this part has to happen in a repository,
> doesn't it?  Or is there some mode of operation of "git archive" I
> am forgetting that can be done without a repository?
>
>     ... goes and looks ...
>
> OK, write_archive() has its own setup_git_directory() call when
> startup_info->have_repository is false, so this happens to be OK,
> until the beginning part of archive.c:write_archive() will not
> changed to start dereferencing "repo" pointer.
>
> That sounds brittle, but probalby outside the scope of what this
> patch series wants to address.  It also makes git_config() calls
> even before it realizes there is no repository and dies, which
> smells fishy without actually doing any harm.
>
> So, after all, I think this step is probably OK.

Yeah I think these are issues we’ll need to address once removing the
the_repository global from archive code.

Thanks
John

>
> Thanks.

[PATCH v3 0/3] Remove the_repository global for am, annotate, apply, archive builtins

[John Cai via GitGitGadget]

Remove the_repository global variable for the annotate, apply, and archive
bulitins.

Changes since V1:

 * in patch 1, only pass in repo to the bulitin if the repo exists

Changes since V2:

 * drop patch 3, which is a bit more involved to dis-entangle the_repository
 * use a single variable in run_builtin() to keep track of whether or not we
   are operating in a repository

John Cai (3):
  git: pass in repo to builtin based on setup_git_directory_gently
  annotate: remove usage of the_repository global
  archive: remove the_repository global variable

 builtin/add.c      | 3 ++-
 builtin/annotate.c | 5 ++---
 builtin/archive.c  | 5 ++---
 git.c              | 7 ++++---
 4 files changed, 10 insertions(+), 10 deletions(-)


base-commit: 3857aae53f3633b7de63ad640737c657387ae0c6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1788%2Fjohn-cai%2Fjc%2Fremove-global-repo-a-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1788/john-cai/jc/remove-global-repo-a-v3
Pull-Request: https://github.com/git/git/pull/1788

Range-diff vs v2:

 1:  5d72c31c6f3 ! 1:  8009fdb38b0 git: pass in repo for RUN_SETUP_GENTLY
     @@ Metadata
      Author: John Cai <johncai86@gmail.com>
      
       ## Commit message ##
     -    git: pass in repo for RUN_SETUP_GENTLY
     +    git: pass in repo to builtin based on setup_git_directory_gently
      
     -    commands that have RUN_SETUP_GENTLY potentially need a repository.
     -    Modify the logic in run_builtin() to pass the repository to the builtin
     -    if a builtin has the RUN_SETUP_GENTLY property.
     +    The current code in run_builtin() passes in a repository to the builtin
     +    based on whether cmd_struct's option flag has RUN_SETUP.
     +
     +    This is incorrect, however, since some builtins that only have
     +    RUN_SETUP_GENTLY can potentially take a repository.
     +    setup_git_directory_gently() tells us whether or not a command is being
     +    run inside of a repository.
     +
     +    Use the output of setup_git_directory_gently() to help determine whether
     +    or not there is a repository to pass to the builtin. If not, then we
     +    just pass NULL.
     +
     +    As part of this patch, we need to modify add to check for a NULL repo
     +    before calling repo_git_config(), since add -h can be run outside of a
     +    repository.
      
          Signed-off-by: John Cai <johncai86@gmail.com>
      
     + ## builtin/add.c ##
     +@@ builtin/add.c: int cmd_add(int argc,
     + 	char *ps_matched = NULL;
     + 	struct lock_file lock_file = LOCK_INIT;
     + 
     +-	repo_config(repo, add_config, NULL);
     ++	if (repo)
     ++		repo_config(repo, add_config, NULL);
     + 
     + 	argc = parse_options(argc, argv, prefix, builtin_add_options,
     + 			  builtin_add_usage, PARSE_OPT_KEEP_ARGV0);
     +
       ## git.c ##
      @@ git.c: static int handle_alias(int *argcp, const char ***argv)
     - 
       static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)
       {
     --	int status, help;
     -+	int status, help, repo_exists;
     + 	int status, help;
     ++	int no_repo = 1;
       	struct stat st;
       	const char *prefix;
       	int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));
     @@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv,
       
       	if (run_setup & RUN_SETUP) {
       		prefix = setup_git_directory();
     -+		repo_exists = 1;
     ++		no_repo = 0;
       	} else if (run_setup & RUN_SETUP_GENTLY) {
     - 		int nongit_ok;
     - 		prefix = setup_git_directory_gently(&nongit_ok);
     -+
     -+		if (!nongit_ok)
     -+			repo_exists = 1;
     +-		int nongit_ok;
     +-		prefix = setup_git_directory_gently(&nongit_ok);
     ++		prefix = setup_git_directory_gently(&no_repo);
       	} else {
       		prefix = NULL;
       	}
     @@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv,
       
       	validate_cache_entries(repo->index);
      -	status = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);
     -+	status = p->fn(argc,
     -+		       argv,
     -+		       prefix,
     -+		       repo_exists ? repo : NULL);
     ++	status = p->fn(argc, argv, prefix, no_repo ? NULL : repo);
       	validate_cache_entries(repo->index);
       
       	if (status)
 2:  2a29d113815 = 2:  1b82b5dc678 annotate: remove usage of the_repository global
 3:  d64955a2e27 < -:  ----------- apply: remove the_repository global variable
 4:  857291d7f7d = 3:  5d33a375f41 archive: remove the_repository global variable

-- 
gitgitgadget

[PATCH v3 1/3] git: pass in repo to builtin based on setup_git_directory_gently

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

The current code in run_builtin() passes in a repository to the builtin
based on whether cmd_struct's option flag has RUN_SETUP.

This is incorrect, however, since some builtins that only have
RUN_SETUP_GENTLY can potentially take a repository.
setup_git_directory_gently() tells us whether or not a command is being
run inside of a repository.

Use the output of setup_git_directory_gently() to help determine whether
or not there is a repository to pass to the builtin. If not, then we
just pass NULL.

As part of this patch, we need to modify add to check for a NULL repo
before calling repo_git_config(), since add -h can be run outside of a
repository.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 builtin/add.c | 3 ++-
 git.c         | 7 ++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 773b7224a49..7d353077921 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -385,7 +385,8 @@ int cmd_add(int argc,
 	char *ps_matched = NULL;
 	struct lock_file lock_file = LOCK_INIT;
 
-	repo_config(repo, add_config, NULL);
+	if (repo)
+		repo_config(repo, add_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, builtin_add_options,
 			  builtin_add_usage, PARSE_OPT_KEEP_ARGV0);
diff --git a/git.c b/git.c
index 2fbea24ec92..47741be3e4c 100644
--- a/git.c
+++ b/git.c
@@ -444,6 +444,7 @@ static int handle_alias(int *argcp, const char ***argv)
 static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)
 {
 	int status, help;
+	int no_repo = 1;
 	struct stat st;
 	const char *prefix;
 	int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));
@@ -455,9 +456,9 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
 
 	if (run_setup & RUN_SETUP) {
 		prefix = setup_git_directory();
+		no_repo = 0;
 	} else if (run_setup & RUN_SETUP_GENTLY) {
-		int nongit_ok;
-		prefix = setup_git_directory_gently(&nongit_ok);
+		prefix = setup_git_directory_gently(&no_repo);
 	} else {
 		prefix = NULL;
 	}
@@ -480,7 +481,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, no_repo ? NULL : repo);
 	validate_cache_entries(repo->index);
 
 	if (status)
-- 
gitgitgadget

Re: [PATCH v3 1/3] git: pass in repo to builtin based on setup_git_directory_gently

[shejialuo]

On Sat, Oct 05, 2024 at 03:30:41AM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@gmail.com>
> 
> The current code in run_builtin() passes in a repository to the builtin
> based on whether cmd_struct's option flag has RUN_SETUP.
> 
> This is incorrect, however, since some builtins that only have
> RUN_SETUP_GENTLY can potentially take a repository.
> setup_git_directory_gently() tells us whether or not a command is being
> run inside of a repository.
> 
> Use the output of setup_git_directory_gently() to help determine whether
> or not there is a repository to pass to the builtin. If not, then we
> just pass NULL.
> 
> As part of this patch, we need to modify add to check for a NULL repo
> before calling repo_git_config(), since add -h can be run outside of a
> repository.
> 
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
>  builtin/add.c | 3 ++-
>  git.c         | 7 ++++---
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/add.c b/builtin/add.c
> index 773b7224a49..7d353077921 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -385,7 +385,8 @@ int cmd_add(int argc,
>  	char *ps_matched = NULL;
>  	struct lock_file lock_file = LOCK_INIT;
>  
> -	repo_config(repo, add_config, NULL);
> +	if (repo)
> +		repo_config(repo, add_config, NULL);

The reason why we need to check whether the `repo` is NULL is that when
using "git add -h", the RUN_SETUP flag would be converted to
RUN_SETUP_GENTLY.

I think this change is OK. But I wonder whether we should encapsulate
the logic into the "repo_config" function. It's a little cumbersome to
check whether the "repo" exists. I also feel it's a bad idea to check in
the "repo_config" function because in the most time, we run commands
inside the repository. So, in my view, at current, this is enough.

Thanks,
Jialuo

[PATCH v3 2/3] annotate: remove usage of the_repository global

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

Remove the the_repository with the repository argument that gets passed
down through the builtin function.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 builtin/annotate.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/annotate.c b/builtin/annotate.c
index a99179fe4dd..ce3dfaafb28 100644
--- a/builtin/annotate.c
+++ b/builtin/annotate.c
@@ -4,7 +4,6 @@
  * Copyright (C) 2006 Ryan Anderson
  */
 
-#define USE_THE_REPOSITORY_VARIABLE
 #include "git-compat-util.h"
 #include "builtin.h"
 #include "strvec.h"
@@ -12,7 +11,7 @@
 int cmd_annotate(int argc,
 		 const char **argv,
 		 const char *prefix,
-		 struct repository *repo UNUSED)
+		 struct repository *repo)
 {
 	struct strvec args = STRVEC_INIT;
 	int i;
@@ -23,5 +22,5 @@ int cmd_annotate(int argc,
 		strvec_push(&args, argv[i]);
 	}
 
-	return cmd_blame(args.nr, args.v, prefix, the_repository);
+	return cmd_blame(args.nr, args.v, prefix, repo);
 }
-- 
gitgitgadget

[PATCH v3 3/3] archive: remove the_repository global variable

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

Replace the_repository with the repository argument that gets passed
down through the builtin function.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 builtin/archive.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index dc926d1a3df..13ea7308c8b 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -2,7 +2,6 @@
  * Copyright (c) 2006 Franck Bui-Huu
  * Copyright (c) 2006 Rene Scharfe
  */
-#define USE_THE_REPOSITORY_VARIABLE
 #include "builtin.h"
 #include "archive.h"
 #include "gettext.h"
@@ -79,7 +78,7 @@ static int run_remote_archiver(int argc, const char **argv,
 int cmd_archive(int argc,
 		const char **argv,
 		const char *prefix,
-		struct repository *repo UNUSED)
+		struct repository *repo)
 {
 	const char *exec = "git-upload-archive";
 	char *output = NULL;
@@ -110,7 +109,7 @@ int cmd_archive(int argc,
 
 	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
 
-	ret = write_archive(argc, argv, prefix, the_repository, output, 0);
+	ret = write_archive(argc, argv, prefix, repo, output, 0);
 
 out:
 	free(output);
-- 
gitgitgadget

Re: [PATCH v3 3/3] archive: remove the_repository global variable

[shejialuo]

On Sat, Oct 05, 2024 at 03:30:43AM +0000, John Cai via GitGitGadget wrote:
> diff --git a/builtin/archive.c b/builtin/archive.c
> index dc926d1a3df..13ea7308c8b 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c

[snip]

>  
> -	ret = write_archive(argc, argv, prefix, the_repository, output, 0);
> +	ret = write_archive(argc, argv, prefix, repo, output, 0);
>  

When I read this new series, I feel quite strange for why we only change
"the_repository" to "repo". After reading the comments from [PATCH v2 4/4],
I have understood the context.

I think we should improve the commit message to take about we decide to
remove the "the_repository" from "archive.c" code unless it will bring a
lot of confusion for the reader.

Thanks,
Jialuo

Re: [PATCH v3 3/3] archive: remove the_repository global variable

[johncai86]

Hi Jialuo,

On 5 Oct 2024, at 3:13, shejialuo wrote:

> On Sat, Oct 05, 2024 at 03:30:43AM +0000, John Cai via GitGitGadget wrote:
>> diff --git a/builtin/archive.c b/builtin/archive.c
>> index dc926d1a3df..13ea7308c8b 100644
>> --- a/builtin/archive.c
>> +++ b/builtin/archive.c
>
> [snip]
>
>>
>> -	ret = write_archive(argc, argv, prefix, the_repository, output, 0);
>> +	ret = write_archive(argc, argv, prefix, repo, output, 0);
>>
>
> When I read this new series, I feel quite strange for why we only change
> "the_repository" to "repo". After reading the comments from [PATCH v2 4/4],
> I have understood the context.
>
> I think we should improve the commit message to take about we decide to
> remove the "the_repository" from "archive.c" code unless it will bring a
> lot of confusion for the reader.

Sounds good. I will add more explanation for why we are making the change.

>
> Thanks,
> Jialuo

Thanks
John

[PATCH v4 0/3] Remove the_repository global for am, annotate, apply, archive builtins

[John Cai via GitGitGadget]

Remove the_repository global variable for the annotate, apply, and archive
bulitins.

Changes since V3:

 * Improve commit message in patch 2

Changes since V1:

 * in patch 1, only pass in repo to the bulitin if the repo exists

Changes since V2:

 * drop patch 3, which is a bit more involved to dis-entangle the_repository
 * use a single variable in run_builtin() to keep track of whether or not we
   are operating in a repository

John Cai (3):
  git: pass in repo to builtin based on setup_git_directory_gently
  annotate: remove usage of the_repository global
  archive: remove the_repository global variable

 builtin/add.c      | 3 ++-
 builtin/annotate.c | 5 ++---
 builtin/archive.c  | 5 ++---
 git.c              | 7 ++++---
 4 files changed, 10 insertions(+), 10 deletions(-)


base-commit: 3857aae53f3633b7de63ad640737c657387ae0c6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1788%2Fjohn-cai%2Fjc%2Fremove-global-repo-a-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1788/john-cai/jc/remove-global-repo-a-v4
Pull-Request: https://github.com/git/git/pull/1788

Range-diff vs v3:

 1:  8009fdb38b0 = 1:  d59b85b5298 git: pass in repo to builtin based on setup_git_directory_gently
 2:  1b82b5dc678 ! 2:  f26d09215c3 annotate: remove usage of the_repository global
     @@ Metadata
       ## Commit message ##
          annotate: remove usage of the_repository global
      
     -    Remove the the_repository with the repository argument that gets passed
     -    down through the builtin function.
     +    As part of the effort to get rid of global state due to the_repository
     +    variable, remove the the_repository with the repository argument that
     +    gets passed down through the builtin function.
      
          Signed-off-by: John Cai <johncai86@gmail.com>
      
 3:  5d33a375f41 ! 3:  736212f34b5 archive: remove the_repository global variable
     @@ Metadata
       ## Commit message ##
          archive: remove the_repository global variable
      
     -    Replace the_repository with the repository argument that gets passed
     -    down through the builtin function.
     +    As part of the effort to get rid of global state due to the global
     +    the_repository variable, replace the_repository with the repository
     +    argument that gets passed down through the builtin function.
     +
     +    The repo might be NULL, but we should be safe in write_archive() because
     +    it detects if we are outside of a repository and calls
     +    setup_git_directory() which will error.
      
          Signed-off-by: John Cai <johncai86@gmail.com>
      

-- 
gitgitgadget

[PATCH v4 1/3] git: pass in repo to builtin based on setup_git_directory_gently

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

The current code in run_builtin() passes in a repository to the builtin
based on whether cmd_struct's option flag has RUN_SETUP.

This is incorrect, however, since some builtins that only have
RUN_SETUP_GENTLY can potentially take a repository.
setup_git_directory_gently() tells us whether or not a command is being
run inside of a repository.

Use the output of setup_git_directory_gently() to help determine whether
or not there is a repository to pass to the builtin. If not, then we
just pass NULL.

As part of this patch, we need to modify add to check for a NULL repo
before calling repo_git_config(), since add -h can be run outside of a
repository.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 builtin/add.c | 3 ++-
 git.c         | 7 ++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 773b7224a49..7d353077921 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -385,7 +385,8 @@ int cmd_add(int argc,
 	char *ps_matched = NULL;
 	struct lock_file lock_file = LOCK_INIT;
 
-	repo_config(repo, add_config, NULL);
+	if (repo)
+		repo_config(repo, add_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, builtin_add_options,
 			  builtin_add_usage, PARSE_OPT_KEEP_ARGV0);
diff --git a/git.c b/git.c
index 2fbea24ec92..47741be3e4c 100644
--- a/git.c
+++ b/git.c
@@ -444,6 +444,7 @@ static int handle_alias(int *argcp, const char ***argv)
 static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)
 {
 	int status, help;
+	int no_repo = 1;
 	struct stat st;
 	const char *prefix;
 	int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));
@@ -455,9 +456,9 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
 
 	if (run_setup & RUN_SETUP) {
 		prefix = setup_git_directory();
+		no_repo = 0;
 	} else if (run_setup & RUN_SETUP_GENTLY) {
-		int nongit_ok;
-		prefix = setup_git_directory_gently(&nongit_ok);
+		prefix = setup_git_directory_gently(&no_repo);
 	} else {
 		prefix = NULL;
 	}
@@ -480,7 +481,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, no_repo ? NULL : repo);
 	validate_cache_entries(repo->index);
 
 	if (status)
-- 
gitgitgadget

[PATCH v4 2/3] annotate: remove usage of the_repository global

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

As part of the effort to get rid of global state due to the_repository
variable, remove the the_repository with the repository argument that
gets passed down through the builtin function.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 builtin/annotate.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/annotate.c b/builtin/annotate.c
index a99179fe4dd..ce3dfaafb28 100644
--- a/builtin/annotate.c
+++ b/builtin/annotate.c
@@ -4,7 +4,6 @@
  * Copyright (C) 2006 Ryan Anderson
  */
 
-#define USE_THE_REPOSITORY_VARIABLE
 #include "git-compat-util.h"
 #include "builtin.h"
 #include "strvec.h"
@@ -12,7 +11,7 @@
 int cmd_annotate(int argc,
 		 const char **argv,
 		 const char *prefix,
-		 struct repository *repo UNUSED)
+		 struct repository *repo)
 {
 	struct strvec args = STRVEC_INIT;
 	int i;
@@ -23,5 +22,5 @@ int cmd_annotate(int argc,
 		strvec_push(&args, argv[i]);
 	}
 
-	return cmd_blame(args.nr, args.v, prefix, the_repository);
+	return cmd_blame(args.nr, args.v, prefix, repo);
 }
-- 
gitgitgadget

[PATCH v4 3/3] archive: remove the_repository global variable

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

As part of the effort to get rid of global state due to the global
the_repository variable, replace the_repository with the repository
argument that gets passed down through the builtin function.

The repo might be NULL, but we should be safe in write_archive() because
it detects if we are outside of a repository and calls
setup_git_directory() which will error.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 builtin/archive.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index dc926d1a3df..13ea7308c8b 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -2,7 +2,6 @@
  * Copyright (c) 2006 Franck Bui-Huu
  * Copyright (c) 2006 Rene Scharfe
  */
-#define USE_THE_REPOSITORY_VARIABLE
 #include "builtin.h"
 #include "archive.h"
 #include "gettext.h"
@@ -79,7 +78,7 @@ static int run_remote_archiver(int argc, const char **argv,
 int cmd_archive(int argc,
 		const char **argv,
 		const char *prefix,
-		struct repository *repo UNUSED)
+		struct repository *repo)
 {
 	const char *exec = "git-upload-archive";
 	char *output = NULL;
@@ -110,7 +109,7 @@ int cmd_archive(int argc,
 
 	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
 
-	ret = write_archive(argc, argv, prefix, the_repository, output, 0);
+	ret = write_archive(argc, argv, prefix, repo, output, 0);
 
 out:
 	free(output);
-- 
gitgitgadget

Re: [PATCH v4 0/3] Remove the_repository global for am, annotate, apply, archive builtins

[Junio C Hamano]

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

> Remove the_repository global variable for the annotate, apply, and archive
> bulitins.
>
> Changes since V3:
>
>  * Improve commit message in patch 2
>
> Changes since V1:
>
>  * in patch 1, only pass in repo to the bulitin if the repo exists
>
> Changes since V2:
>
>  * drop patch 3, which is a bit more involved to dis-entangle the_repository
>  * use a single variable in run_builtin() to keep track of whether or not we
>    are operating in a repository
>
> John Cai (3):
>   git: pass in repo to builtin based on setup_git_directory_gently
>   annotate: remove usage of the_repository global
>   archive: remove the_repository global variable

Will queue.  Thanks, all.