Remove the_repository global for am, annotate, apply, archive builtins

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

From: John Cai <[email protected]>

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.

---
 git.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index 2fbea24ec92..e31b52dcc50 100644
@@ -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

From: John Cai <[email protected]>

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

---
 builtin/annotate.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index a99179fe4dd..ce3dfaafb28 100644
@@ -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

From: John Cai <[email protected]>

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

---
 builtin/apply.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

index 84f1863d3ac..d0bafbec7e4 100644
@@ -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/[email protected]/
 	 */
-	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

From: John Cai <[email protected]>

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

---
 builtin/archive.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index dc926d1a3df..13ea7308c8b 100644
@@ -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

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

[snip]

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.

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.

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

index 2fbea24ec9..579c6fa36d 100644
@@ -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)

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

True, too.

Hi Junio,

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

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.

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

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

From: John Cai <[email protected]>

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.

---
 git.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

index 2fbea24ec92..f58f169f3c7 100644
@@ -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

From: John Cai <[email protected]>

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

---
 builtin/annotate.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index a99179fe4dd..ce3dfaafb28 100644
@@ -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

From: John Cai <[email protected]>

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

---
 builtin/apply.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

index 84f1863d3ac..d0bafbec7e4 100644
@@ -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/[email protected]/
 	 */
-	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

From: John Cai <[email protected]>

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

---
 builtin/archive.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index dc926d1a3df..13ea7308c8b 100644
@@ -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

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.

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

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

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

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

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.

That is also a viable approach.

Hi Junio,

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

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

Thanks
John

Hi Junio,

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

Thanks
John

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 <[email protected]>
      
       ## 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 <[email protected]>
      
     + ## 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

From: John Cai <[email protected]>

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.

---
 builtin/add.c | 3 ++-
 git.c         | 7 ++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

index 773b7224a49..7d353077921 100644
@@ -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);

index 2fbea24ec92..47741be3e4c 100644
@@ -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

From: John Cai <[email protected]>

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

---
 builtin/annotate.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index a99179fe4dd..ce3dfaafb28 100644
@@ -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

From: John Cai <[email protected]>

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

---
 builtin/archive.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index dc926d1a3df..13ea7308c8b 100644
@@ -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

[snip]

Hi Jialuo,

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

Thanks
John

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.

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.

gitgitgadget

From: John Cai <[email protected]>

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.

---
 builtin/add.c | 3 ++-
 git.c         | 7 ++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

index 773b7224a49..7d353077921 100644
@@ -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);

index 2fbea24ec92..47741be3e4c 100644
@@ -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

From: John Cai <[email protected]>

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.

---
 builtin/annotate.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index a99179fe4dd..ce3dfaafb28 100644
@@ -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

From: John Cai <[email protected]>

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.

---
 builtin/archive.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index dc926d1a3df..13ea7308c8b 100644
@@ -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

Will queue. Thanks, all.