{"thread":{"id":"62185","subject":"[PATCH 0/4] Remove the_repository global for am, annotate, apply, archive builtins","startedAt":"2024-09-24T13:42:48Z","lastAt":"2024-10-11T17:47:45Z","messageCount":44,"participants":["John Cai via GitGitGadget","shejialuo","Junio C Hamano","Patrick Steinhardt","John Cai","johncai86@gmail.com"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"503352","messageId":"pull.1788.git.git.1727185364.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":null,"subject":"[PATCH 0/4] Remove the_repository global for am, annotate, apply, archive builtins","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-24T13:42:40Z","receivedAt":"2024-09-24T13:42:48Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Remove the_repository global variable for the annotate, apply, and archive\nbulitins.\n\nJohn Cai (4):\n git: pass in repo for RUN_SETUP_GENTLY\n annotate: remove usage of the_repository global\n apply: remove the_repository global variable\n archive: remove the_repository global variable\n\n builtin/annotate.c | 5 ++---\n builtin/apply.c | 9 ++++-----\n builtin/archive.c | 5 ++---\n git.c | 5 ++++-\n 4 files changed, 12 insertions(+), 12 deletions(-)\n\n\nbase-commit: 6258f68c3c1092c901337895c864073dcdea9213\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1788%2Fjohn-cai%2Fjc%2Fremove-global-repo-a-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1788/john-cai/jc/remove-global-repo-a-v1\nPull-Request: https://github.com/git/git/pull/1788\n-- \ngitgitgadget\n"},{"id":"503353","messageId":"eceb2d835be7168081d6eeffbce57bba89b5f423.1727185364.git.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":"pull.1788.git.git.1727185364.gitgitgadget@gmail.com","subject":"[PATCH 1/4] git: pass in repo for RUN_SETUP_GENTLY","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-24T13:42:41Z","receivedAt":"2024-09-24T13:42:48Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\ncommands that have RUN_SETUP_GENTLY potentially need a repository.\nModify the logic in run_builtin() to pass the repository to the builtin\nif a builtin has the RUN_SETUP_GENTLY property.\n\nSigned-off-by: John Cai \n---\n git.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/git.c b/git.c\nindex 2fbea24ec92..e31b52dcc50 100644\n--- a/git.c\n+++ b/git.c\n@@ -480,7 +480,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \ttrace2_cmd_name(p->cmd);\n \n \tvalidate_cache_entries(repo->index);\n-\tstatus = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);\n+\tstatus = p->fn(argc,\n+\t\t argv,\n+\t\t prefix,\n+\t\t ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL);\n \tvalidate_cache_entries(repo->index);\n \n \tif (status)\n-- \ngitgitgadget\n\n"},{"id":"503354","messageId":"1bf2b017dd3f9fd3d91687307fd519b24fc0f965.1727185364.git.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":"pull.1788.git.git.1727185364.gitgitgadget@gmail.com","subject":"[PATCH 2/4] annotate: remove usage of the_repository global","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-24T13:42:42Z","receivedAt":"2024-09-24T13:42:50Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nRemove the the_repository with the repository argument that gets passed\ndown through the builtin function.\n\nSigned-off-by: John Cai \n---\n builtin/annotate.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/annotate.c b/builtin/annotate.c\nindex a99179fe4dd..ce3dfaafb28 100644\n--- a/builtin/annotate.c\n+++ b/builtin/annotate.c\n@@ -4,7 +4,6 @@\n * Copyright (C) 2006 Ryan Anderson\n */\n \n-#define USE_THE_REPOSITORY_VARIABLE\n #include \"git-compat-util.h\"\n #include \"builtin.h\"\n #include \"strvec.h\"\n@@ -12,7 +11,7 @@\n int cmd_annotate(int argc,\n \t\t const char **argv,\n \t\t const char *prefix,\n-\t\t struct repository *repo UNUSED)\n+\t\t struct repository *repo)\n {\n \tstruct strvec args = STRVEC_INIT;\n \tint i;\n@@ -23,5 +22,5 @@ int cmd_annotate(int argc,\n \t\tstrvec_push(&args, argv[i]);\n \t}\n \n-\treturn cmd_blame(args.nr, args.v, prefix, the_repository);\n+\treturn cmd_blame(args.nr, args.v, prefix, repo);\n }\n-- \ngitgitgadget\n\n"},{"id":"503355","messageId":"4ce463defa807fb99eef6ce7abcd758fc2065c13.1727185364.git.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":"pull.1788.git.git.1727185364.gitgitgadget@gmail.com","subject":"[PATCH 3/4] apply: remove the_repository global variable","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-24T13:42:43Z","receivedAt":"2024-09-24T13:42:50Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nRemove the_repository global variable in favor of the repository\nargument that gets passed in through the builtin function.\n\nSigned-off-by: John Cai \n---\n builtin/apply.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 84f1863d3ac..d0bafbec7e4 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1,4 +1,3 @@\n-#define USE_THE_REPOSITORY_VARIABLE\n #include \"builtin.h\"\n #include \"gettext.h\"\n #include \"hash.h\"\n@@ -12,14 +11,14 @@ static const char * const apply_usage[] = {\n int cmd_apply(int argc,\n \t const char **argv,\n \t const char *prefix,\n-\t struct repository *repo UNUSED)\n+\t struct repository *repo)\n {\n \tint force_apply = 0;\n \tint options = 0;\n \tint ret;\n \tstruct apply_state state;\n \n-\tif (init_apply_state(&state, the_repository, prefix))\n+\tif (init_apply_state(&state, repo, prefix))\n \t\texit(128);\n \n \t/*\n@@ -28,8 +27,8 @@ int cmd_apply(int argc,\n \t * is worth the effort.\n \t * cf. https://lore.kernel.org/git/xmqqcypfcmn4.fsf@gitster.g/\n \t */\n-\tif (!the_hash_algo)\n-\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n+\tif (!repo->hash_algo)\n+\t\trepo_set_hash_algo(repo, GIT_HASH_SHA1);\n \n \targc = apply_parse_options(argc, argv,\n \t\t\t\t &state, &force_apply, &options,\n-- \ngitgitgadget\n\n"},{"id":"503356","messageId":"f6c32ec609cca56f1c02a75929dc7cc19d4834cd.1727185364.git.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":"pull.1788.git.git.1727185364.gitgitgadget@gmail.com","subject":"[PATCH 4/4] archive: remove the_repository global variable","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-24T13:42:44Z","receivedAt":"2024-09-24T13:42:51Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nReplace the_repository with the repository argument that gets passed\ndown through the builtin function.\n\nSigned-off-by: John Cai \n---\n builtin/archive.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex dc926d1a3df..13ea7308c8b 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -2,7 +2,6 @@\n * Copyright (c) 2006 Franck Bui-Huu\n * Copyright (c) 2006 Rene Scharfe\n */\n-#define USE_THE_REPOSITORY_VARIABLE\n #include \"builtin.h\"\n #include \"archive.h\"\n #include \"gettext.h\"\n@@ -79,7 +78,7 @@ static int run_remote_archiver(int argc, const char **argv,\n int cmd_archive(int argc,\n \t\tconst char **argv,\n \t\tconst char *prefix,\n-\t\tstruct repository *repo UNUSED)\n+\t\tstruct repository *repo)\n {\n \tconst char *exec = \"git-upload-archive\";\n \tchar *output = NULL;\n@@ -110,7 +109,7 @@ int cmd_archive(int argc,\n \n \tsetvbuf(stderr, NULL, _IOLBF, BUFSIZ);\n \n-\tret = write_archive(argc, argv, prefix, the_repository, output, 0);\n+\tret = write_archive(argc, argv, prefix, repo, output, 0);\n \n out:\n \tfree(output);\n-- \ngitgitgadget\n"},{"id":"503364","messageId":"ZvLZqIe8rpGZTU0C@ArchLinux","threadId":"62185","inReplyTo":"eceb2d835be7168081d6eeffbce57bba89b5f423.1727185364.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] git: pass in repo for RUN_SETUP_GENTLY","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-09-24T15:24:24Z","receivedAt":"2024-09-24T15:23:11Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Tue, Sep 24, 2024 at 01:42:41PM +0000, John Cai via GitGitGadget wrote:\n\n[snip]\n\n> diff --git a/git.c b/git.c\n> index 2fbea24ec92..e31b52dcc50 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -480,7 +480,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n> \ttrace2_cmd_name(p->cmd);\n\nThis line is a little long, we may clean this in this patch.\n\n> \n> \tvalidate_cache_entries(repo->index);\n> -\tstatus = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);\n> +\tstatus = p->fn(argc,\n> +\t\t argv,\n> +\t\t prefix,\n> +\t\t ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL);\n\nThis reads so strange, could we create a new variable here?\n\nSmall problems, don't worth a reroll.\n\nThanks,\nJialuo\n"},{"id":"503384","messageId":"xmqqr099t07t.fsf@gitster.g","threadId":"62185","inReplyTo":"ZvLZqIe8rpGZTU0C@ArchLinux","subject":"Re: [PATCH 1/4] git: pass in repo for RUN_SETUP_GENTLY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-24T17:45:10Z","receivedAt":"2024-09-24T17:45:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo writes:\n\n> On Tue, Sep 24, 2024 at 01:42:41PM +0000, John Cai via GitGitGadget wrote:\n>\n> [snip]\n>\n>> diff --git a/git.c b/git.c\n>> index 2fbea24ec92..e31b52dcc50 100644\n>> --- a/git.c\n>> +++ b/git.c\n>> @@ -480,7 +480,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n>> \ttrace2_cmd_name(p->cmd);\n>\n> This line is a little long, we may clean this in this patch.\n>\n>> \n>> \tvalidate_cache_entries(repo->index);\n>> -\tstatus = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);\n>> +\tstatus = p->fn(argc,\n>> +\t\t argv,\n>> +\t\t prefix,\n>> +\t\t ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL);\n>\n> This reads so strange, could we create a new variable here?\n>\n> Small problems, don't worth a reroll.\n\nIf it is so annoying to make your reading hiccup, it is not small at\nall. One argument per line is so strange, and one plausible fix\nwould be\n\n\tstatus = p->fn(argc, argv, prefix,\n \t (p->option & (RUN_SETUP|RUN_SETUP_GENTLY))\n\t\t ? repo : NULL);\n\nThanks.\n"},{"id":"503385","messageId":"xmqqjzf1szl7.fsf@gitster.g","threadId":"62185","inReplyTo":"eceb2d835be7168081d6eeffbce57bba89b5f423.1727185364.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] git: pass in repo for RUN_SETUP_GENTLY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-24T17:58:44Z","receivedAt":"2024-09-24T17:58:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> commands that have RUN_SETUP_GENTLY potentially need a repository.\n> Modify the logic in run_builtin() to pass the repository to the builtin\n> if a builtin has the RUN_SETUP_GENTLY property.\n\nAh, I remember mentioning this as a potiential future direction\nwhile reviewing the other series that added the repository argument\nto the cmd_foo() interface.\n\nNice to see the idea getting followed up.\n\nLet's see how [Patches 2-4/4] can make effective use of this.\n\n> Signed-off-by: John Cai \n> ---\n> git.c | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/git.c b/git.c\n> index 2fbea24ec92..e31b52dcc50 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -480,7 +480,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n> \ttrace2_cmd_name(p->cmd);\n> \n> \tvalidate_cache_entries(repo->index);\n> -\tstatus = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);\n> +\tstatus = p->fn(argc,\n> +\t\t argv,\n> +\t\t prefix,\n> +\t\t ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL);\n> \tvalidate_cache_entries(repo->index);\n> \n> \tif (status)\n"},{"id":"503387","messageId":"xmqq7cb0ucm0.fsf@gitster.g","threadId":"62185","inReplyTo":"4ce463defa807fb99eef6ce7abcd758fc2065c13.1727185364.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] apply: remove the_repository global variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-24T18:32:07Z","receivedAt":"2024-09-24T18:32:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> Remove the_repository global variable in favor of the repository\n> argument that gets passed in through the builtin function.\n>\n> Signed-off-by: John Cai \n> ---\n> builtin/apply.c | 9 ++++-----\n> 1 file changed, 4 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 84f1863d3ac..d0bafbec7e4 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -1,4 +1,3 @@\n> -#define USE_THE_REPOSITORY_VARIABLE\n> #include \"builtin.h\"\n> #include \"gettext.h\"\n> #include \"hash.h\"\n> @@ -12,14 +11,14 @@ static const char * const apply_usage[] = {\n> int cmd_apply(int argc,\n> \t const char **argv,\n> \t const char *prefix,\n> -\t struct repository *repo UNUSED)\n> +\t struct repository *repo)\n> {\n> \tint force_apply = 0;\n> \tint options = 0;\n> \tint ret;\n> \tstruct apply_state state;\n> \n> -\tif (init_apply_state(&state, the_repository, prefix))\n> +\tif (init_apply_state(&state, repo, prefix))\n> \t\texit(128);\n\nHmph, the reason why we do not segfault with this patch is because\nrepo will _always_ be the_repository due to the previous change.\n\nI am not sure if [1/4] is an improvement, though. We used to be\nable to tell if we were running in a repository, or we were running\nin \"nongit\" mode, by looking at the NULL-ness of repo (which was\nUNUSED because we weren't taking advantage of that). \n\nWith [1/4], it no longer is possible. From the point of view of API\nto call into builtin implementations, it smells like a regression.\n\nA more honest change for this hunk would rather be something like:\n\n -\tif (init_apply_state(&state, the_repository, prefix))\n +\tif (!repo)\n +\t\trepo = the_repository;\n +\tif (init_apply_state(&state, repo, prefix))\n\nwithout [1/4]. This change does not address \"apply still depends on\nhaving access to the_repository even when it is being used as a better\nGNU patch\" issue at all.\n\nSo, no, while I earlier said I was happy with [1/4], I no longer am\nenthused by the change.\n"},{"id":"503388","messageId":"xmqq34loucgi.fsf@gitster.g","threadId":"62185","inReplyTo":"f6c32ec609cca56f1c02a75929dc7cc19d4834cd.1727185364.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] archive: remove the_repository global variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-24T18:35:25Z","receivedAt":"2024-09-24T18:35:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> int cmd_archive(int argc,\n> \t\tconst char **argv,\n> \t\tconst char *prefix,\n> -\t\tstruct repository *repo UNUSED)\n> +\t\tstruct repository *repo)\n> {\n> \tconst char *exec = \"git-upload-archive\";\n> \tchar *output = NULL;\n> @@ -110,7 +109,7 @@ int cmd_archive(int argc,\n> \n> \tsetvbuf(stderr, NULL, _IOLBF, BUFSIZ);\n> \n> -\tret = write_archive(argc, argv, prefix, the_repository, output, 0);\n> +\tret = write_archive(argc, argv, prefix, repo, output, 0);\n\nExactly the same comment applies here as [3/4]\n"},{"id":"503389","messageId":"xmqqtte4sx7p.fsf@gitster.g","threadId":"62185","inReplyTo":"xmqq7cb0ucm0.fsf@gitster.g","subject":"Re: [PATCH 3/4] apply: remove the_repository global variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-24T18:50:02Z","receivedAt":"2024-09-24T18:50:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> -\tif (init_apply_state(&state, the_repository, prefix))\n>> +\tif (init_apply_state(&state, repo, prefix))\n>> \t\texit(128);\n>\n> Hmph, the reason why we do not segfault with this patch is because\n> repo will _always_ be the_repository due to the previous change.\n>\n> I am not sure if [1/4] is an improvement, though. We used to be\n> able to tell if we were running in a repository, or we were running\n> in \"nongit\" mode, by looking at the NULL-ness of repo (which was\n> UNUSED because we weren't taking advantage of that). \n>\n> With [1/4], it no longer is possible. From the point of view of API\n> to call into builtin implementations, it smells like a regression.\n\nWe can avoid the regression by passing the discovered \"nongit\" (aka\n\"are we outside of a repository?\") bit separately, perhaps like\nthis. With such a change, I do not mind this change too much, but\npretending that we do not depend on the_repository (by removing the\ntextual mention of the_repository), but still depending on\nthe_repository (which points at the_repo) may be losing a bigger\npicture, the true reason why we want to reduce the dependence on\nthe_repository. We still need \"if the hash-algo is not initialized\nfall back to SHA-1\" code here, but that is an overly broad fallback\nthat we would rather want to tighten to something like \"we know we\nhave no reasonable value to initialize hash-algo in the_repository\nif we are outside a repository, so initialize hash-algo if we are\noutside any repository\" (leaving it an error not have hash-algo in\n\"repo\" if we _are_ in a repository).\n \ndiff --git c/git.c w/git.c\nindex 2fbea24ec9..579c6fa36d 100644\n--- c/git.c\n+++ w/git.c\n@@ -447,6 +447,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \tstruct stat st;\n \tconst char *prefix;\n \tint run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));\n+\tint nongit = 0;\n \n \thelp = argc == 2 && !strcmp(argv[1], \"-h\");\n \tif (help && (run_setup & RUN_SETUP))\n@@ -456,8 +457,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \tif (run_setup & RUN_SETUP) {\n \t\tprefix = setup_git_directory();\n \t} else if (run_setup & RUN_SETUP_GENTLY) {\n-\t\tint nongit_ok;\n-\t\tprefix = setup_git_directory_gently(&nongit_ok);\n+\t\tprefix = setup_git_directory_gently(&nongit);\n \t} else {\n \t\tprefix = NULL;\n \t}\n@@ -480,7 +480,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \ttrace2_cmd_name(p->cmd);\n \n \tvalidate_cache_entries(repo->index);\n-\tstatus = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);\n+\tstatus = p->fn(argc, argv, prefix, run_setup ? repo : NULL, nongit);\n \tvalidate_cache_entries(repo->index);\n \n \tif (status)\n"},{"id":"503561","messageId":"ZvVumLAFu1LGzXWP@pks.im","threadId":"62185","inReplyTo":"eceb2d835be7168081d6eeffbce57bba89b5f423.1727185364.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] git: pass in repo for RUN_SETUP_GENTLY","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-26T14:24:24Z","receivedAt":"2024-09-26T14:24:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 24, 2024 at 01:42:41PM +0000, John Cai via GitGitGadget wrote:\n> From: John Cai \n> \n> commands that have RUN_SETUP_GENTLY potentially need a repository.\n> Modify the logic in run_builtin() to pass the repository to the builtin\n> if a builtin has the RUN_SETUP_GENTLY property.\n> \n> Signed-off-by: John Cai \n> ---\n> git.c | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git a/git.c b/git.c\n> index 2fbea24ec92..e31b52dcc50 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -480,7 +480,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n> \ttrace2_cmd_name(p->cmd);\n> \n> \tvalidate_cache_entries(repo->index);\n> -\tstatus = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);\n> +\tstatus = p->fn(argc,\n> +\t\t argv,\n> +\t\t prefix,\n> +\t\t ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL);\n> \tvalidate_cache_entries(repo->index);\n\nShould we really pass `repo` unconditionally when `RUN_SETUP_GENTLY` was\nrequested? I'd think that we should rather pass `NULL` if we didn't find\na repository in that case. So this condition should likely be made\nconditional, shouldn't it?\n\nThere's also a missing space between the closing brace and the ternary\nquestionmark.\n\nPatrick\n"},{"id":"503577","messageId":"xmqq1q16l64d.fsf@gitster.g","threadId":"62185","inReplyTo":"ZvVumLAFu1LGzXWP@pks.im","subject":"Re: [PATCH 1/4] git: pass in repo for RUN_SETUP_GENTLY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-26T16:41:38Z","receivedAt":"2024-09-26T16:41:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> -\tstatus = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);\n>> +\tstatus = p->fn(argc,\n>> +\t\t argv,\n>> +\t\t prefix,\n>> +\t\t ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL);\n>> \tvalidate_cache_entries(repo->index);\n>\n> Should we really pass `repo` unconditionally when `RUN_SETUP_GENTLY` was\n> requested? I'd think that we should rather pass `NULL` if we didn't find\n> a repository in that case. So this condition should likely be made\n> conditional, shouldn't it?\n\nYeah, that is much much more preferrable than my earlier suggestion\nto pass yet another Boolean parameter to p->fn().\n\n> There's also a missing space between the closing brace and the ternary\n> questionmark.\n\nTrue, too.\n"},{"id":"503583","messageId":"CAOCgCU+hv07+FCupr2Ok9LJm6HYT6n6t+ZpifAhwrRnMzOnnWA@mail.gmail.com","threadId":"62185","inReplyTo":"xmqq7cb0ucm0.fsf@gitster.g","subject":"Re: [PATCH 3/4] apply: remove the_repository global variable","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2024-09-26T18:59:50Z","receivedAt":"2024-09-26T19:00:04Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn Tue, Sep 24, 2024 at 2:32 PM Junio C Hamano wrote:\n>\n> \"John Cai via GitGitGadget\" writes:\n>\n> > From: John Cai \n> >\n> > Remove the_repository global variable in favor of the repository\n> > argument that gets passed in through the builtin function.\n> >\n> > Signed-off-by: John Cai \n> > ---\n> > builtin/apply.c | 9 ++++-----\n> > 1 file changed, 4 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/builtin/apply.c b/builtin/apply.c\n> > index 84f1863d3ac..d0bafbec7e4 100644\n> > --- a/builtin/apply.c\n> > +++ b/builtin/apply.c\n> > @@ -1,4 +1,3 @@\n> > -#define USE_THE_REPOSITORY_VARIABLE\n> > #include \"builtin.h\"\n> > #include \"gettext.h\"\n> > #include \"hash.h\"\n> > @@ -12,14 +11,14 @@ static const char * const apply_usage[] = {\n> > int cmd_apply(int argc,\n> > const char **argv,\n> > const char *prefix,\n> > - struct repository *repo UNUSED)\n> > + struct repository *repo)\n> > {\n> > int force_apply = 0;\n> > int options = 0;\n> > int ret;\n> > struct apply_state state;\n> >\n> > - if (init_apply_state(&state, the_repository, prefix))\n> > + if (init_apply_state(&state, repo, prefix))\n> > exit(128);\n>\n> Hmph, the reason why we do not segfault with this patch is because\n> repo will _always_ be the_repository due to the previous change.\n>\n> I am not sure if [1/4] is an improvement, though. We used to be\n> able to tell if we were running in a repository, or we were running\n> in \"nongit\" mode, by looking at the NULL-ness of repo (which was\n> UNUSED because we weren't taking advantage of that).\n>\n> With [1/4], it no longer is possible. From the point of view of API\n> to call into builtin implementations, it smells like a regression.\n\nI see your point here. However, I was wondering about this because\nwe are passing in the_repository through run_builtin() as repo--so wouldn't\nthis be equivalent to using the_repository and hence the\nsame API contract can remain that looks at the NULL-ness of repo?\n\nBut I could be missing something here.\n\nthanks!\nJohn\n\n>\n> A more honest change for this hunk would rather be something like:\n>\n> - if (init_apply_state(&state, the_repository, prefix))\n> + if (!repo)\n> + repo = the_repository;\n> + if (init_apply_state(&state, repo, prefix))\n>\n> without [1/4]. This change does not address \"apply still depends on\n> having access to the_repository even when it is being used as a better\n> GNU patch\" issue at all.\n>\n> So, no, while I earlier said I was happy with [1/4], I no longer am\n> enthused by the change.\n"},{"id":"503585","messageId":"xmqqa5fui5ri.fsf@gitster.g","threadId":"62185","inReplyTo":"CAOCgCU+hv07+FCupr2Ok9LJm6HYT6n6t+ZpifAhwrRnMzOnnWA@mail.gmail.com","subject":"Re: [PATCH 3/4] apply: remove the_repository global variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-26T19:17:37Z","receivedAt":"2024-09-26T19:17:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Cai writes:\n\n>> > - if (init_apply_state(&state, the_repository, prefix))\n>> > + if (init_apply_state(&state, repo, prefix))\n>> > exit(128);\n>>\n>> Hmph, the reason why we do not segfault with this patch is because\n>> repo will _always_ be the_repository due to the previous change.\n>>\n>> I am not sure if [1/4] is an improvement, though. We used to be\n>> able to tell if we were running in a repository, or we were running\n>> in \"nongit\" mode, by looking at the NULL-ness of repo (which was\n>> UNUSED because we weren't taking advantage of that).\n>>\n>> With [1/4], it no longer is possible. From the point of view of API\n>> to call into builtin implementations, it smells like a regression.\n>\n> I see your point here. However, I was wondering about this because\n> we are passing in the_repository through run_builtin() as repo--so wouldn't\n> this be equivalent to using the_repository and hence the\n> same API contract can remain that looks at the NULL-ness of repo?\n>\n> But I could be missing something here.\n\nAs run_builtin() discards the value of nongit, we will always see\nrepo == the_repository passed to this function, whether _gently()\nfound that we are in or not in a repository. I think Patrick also\nnoticed it and suggested to pass repo or NULL conditionally in a\nseparate message, and if that is done, then I am fine. I do not\nthink your [1/4] as-is did that.\n\nThanks.\n"},{"id":"503745","messageId":"pull.1788.v2.git.git.1727718030.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":"pull.1788.git.git.1727185364.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] Remove the_repository global for am, annotate, apply, archive builtins","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-30T17:40:26Z","receivedAt":"2024-09-30T17:40:34Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Remove the_repository global variable for the annotate, apply, and archive\nbulitins.\n\nChanges since V1:\n\n * in patch 1, only pass in repo to the bulitin if the repo exists\n\nJohn Cai (4):\n git: pass in repo for RUN_SETUP_GENTLY\n annotate: remove usage of the_repository global\n apply: remove the_repository global variable\n archive: remove the_repository global variable\n\n builtin/annotate.c | 5 ++---\n builtin/apply.c | 9 ++++-----\n builtin/archive.c | 5 ++---\n git.c | 11 +++++++++--\n 4 files changed, 17 insertions(+), 13 deletions(-)\n\n\nbase-commit: 3857aae53f3633b7de63ad640737c657387ae0c6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1788%2Fjohn-cai%2Fjc%2Fremove-global-repo-a-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1788/john-cai/jc/remove-global-repo-a-v2\nPull-Request: https://github.com/git/git/pull/1788\n\nRange-diff vs v1:\n\n 1: eceb2d835be ! 1: 5d72c31c6f3 git: pass in repo for RUN_SETUP_GENTLY\n @@ Commit message\n Signed-off-by: John Cai \n \n ## git.c ##\n +@@ git.c: static int handle_alias(int *argcp, const char ***argv)\n + \n + static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)\n + {\n +-\tint status, help;\n ++\tint status, help, repo_exists;\n + \tstruct stat st;\n + \tconst char *prefix;\n + \tint run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));\n +@@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n + \n + \tif (run_setup & RUN_SETUP) {\n + \t\tprefix = setup_git_directory();\n ++\t\trepo_exists = 1;\n + \t} else if (run_setup & RUN_SETUP_GENTLY) {\n + \t\tint nongit_ok;\n + \t\tprefix = setup_git_directory_gently(&nongit_ok);\n ++\n ++\t\tif (!nongit_ok)\n ++\t\t\trepo_exists = 1;\n + \t} else {\n + \t\tprefix = NULL;\n + \t}\n @@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \ttrace2_cmd_name(p->cmd);\n \n @@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv,\n +\tstatus = p->fn(argc,\n +\t\t argv,\n +\t\t prefix,\n -+\t\t ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL);\n ++\t\t repo_exists ? repo : NULL);\n \tvalidate_cache_entries(repo->index);\n \n \tif (status)\n 2: 1bf2b017dd3 = 2: 2a29d113815 annotate: remove usage of the_repository global\n 3: 4ce463defa8 = 3: d64955a2e27 apply: remove the_repository global variable\n 4: f6c32ec609c = 4: 857291d7f7d archive: remove the_repository global variable\n\n-- \ngitgitgadget\n"},{"id":"503746","messageId":"5d72c31c6f3b97b7f5f7d3b4fa9a8b1587597670.1727718030.git.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":"pull.1788.v2.git.git.1727718030.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] git: pass in repo for RUN_SETUP_GENTLY","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-30T17:40:27Z","receivedAt":"2024-09-30T17:40:34Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\ncommands that have RUN_SETUP_GENTLY potentially need a repository.\nModify the logic in run_builtin() to pass the repository to the builtin\nif a builtin has the RUN_SETUP_GENTLY property.\n\nSigned-off-by: John Cai \n---\n git.c | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 2fbea24ec92..f58f169f3c7 100644\n--- a/git.c\n+++ b/git.c\n@@ -443,7 +443,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \n static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)\n {\n-\tint status, help;\n+\tint status, help, repo_exists;\n \tstruct stat st;\n \tconst char *prefix;\n \tint run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));\n@@ -455,9 +455,13 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \n \tif (run_setup & RUN_SETUP) {\n \t\tprefix = setup_git_directory();\n+\t\trepo_exists = 1;\n \t} else if (run_setup & RUN_SETUP_GENTLY) {\n \t\tint nongit_ok;\n \t\tprefix = setup_git_directory_gently(&nongit_ok);\n+\n+\t\tif (!nongit_ok)\n+\t\t\trepo_exists = 1;\n \t} else {\n \t\tprefix = NULL;\n \t}\n@@ -480,7 +484,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \ttrace2_cmd_name(p->cmd);\n \n \tvalidate_cache_entries(repo->index);\n-\tstatus = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);\n+\tstatus = p->fn(argc,\n+\t\t argv,\n+\t\t prefix,\n+\t\t repo_exists ? repo : NULL);\n \tvalidate_cache_entries(repo->index);\n \n \tif (status)\n-- \ngitgitgadget\n\n"},{"id":"503747","messageId":"2a29d113815015b82d807d94f8d551c3f885cb9c.1727718031.git.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":"pull.1788.v2.git.git.1727718030.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] annotate: remove usage of the_repository global","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-30T17:40:28Z","receivedAt":"2024-09-30T17:40:35Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nRemove the the_repository with the repository argument that gets passed\ndown through the builtin function.\n\nSigned-off-by: John Cai \n---\n builtin/annotate.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/annotate.c b/builtin/annotate.c\nindex a99179fe4dd..ce3dfaafb28 100644\n--- a/builtin/annotate.c\n+++ b/builtin/annotate.c\n@@ -4,7 +4,6 @@\n * Copyright (C) 2006 Ryan Anderson\n */\n \n-#define USE_THE_REPOSITORY_VARIABLE\n #include \"git-compat-util.h\"\n #include \"builtin.h\"\n #include \"strvec.h\"\n@@ -12,7 +11,7 @@\n int cmd_annotate(int argc,\n \t\t const char **argv,\n \t\t const char *prefix,\n-\t\t struct repository *repo UNUSED)\n+\t\t struct repository *repo)\n {\n \tstruct strvec args = STRVEC_INIT;\n \tint i;\n@@ -23,5 +22,5 @@ int cmd_annotate(int argc,\n \t\tstrvec_push(&args, argv[i]);\n \t}\n \n-\treturn cmd_blame(args.nr, args.v, prefix, the_repository);\n+\treturn cmd_blame(args.nr, args.v, prefix, repo);\n }\n-- \ngitgitgadget\n\n"},{"id":"503748","messageId":"857291d7f7dffdd1a63ce9268c8ac91a82f2bdb5.1727718031.git.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":"pull.1788.v2.git.git.1727718030.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] archive: remove the_repository global variable","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-30T17:40:30Z","receivedAt":"2024-09-30T17:40:36Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nReplace the_repository with the repository argument that gets passed\ndown through the builtin function.\n\nSigned-off-by: John Cai \n---\n builtin/archive.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex dc926d1a3df..13ea7308c8b 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -2,7 +2,6 @@\n * Copyright (c) 2006 Franck Bui-Huu\n * Copyright (c) 2006 Rene Scharfe\n */\n-#define USE_THE_REPOSITORY_VARIABLE\n #include \"builtin.h\"\n #include \"archive.h\"\n #include \"gettext.h\"\n@@ -79,7 +78,7 @@ static int run_remote_archiver(int argc, const char **argv,\n int cmd_archive(int argc,\n \t\tconst char **argv,\n \t\tconst char *prefix,\n-\t\tstruct repository *repo UNUSED)\n+\t\tstruct repository *repo)\n {\n \tconst char *exec = \"git-upload-archive\";\n \tchar *output = NULL;\n@@ -110,7 +109,7 @@ int cmd_archive(int argc,\n \n \tsetvbuf(stderr, NULL, _IOLBF, BUFSIZ);\n \n-\tret = write_archive(argc, argv, prefix, the_repository, output, 0);\n+\tret = write_archive(argc, argv, prefix, repo, output, 0);\n \n out:\n \tfree(output);\n-- \ngitgitgadget\n"},{"id":"503749","messageId":"d64955a2e277da138146020f6a0cf96f4636a162.1727718031.git.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":"pull.1788.v2.git.git.1727718030.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] apply: remove the_repository global variable","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-30T17:40:29Z","receivedAt":"2024-09-30T17:40:36Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nRemove the_repository global variable in favor of the repository\nargument that gets passed in through the builtin function.\n\nSigned-off-by: John Cai \n---\n builtin/apply.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 84f1863d3ac..d0bafbec7e4 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1,4 +1,3 @@\n-#define USE_THE_REPOSITORY_VARIABLE\n #include \"builtin.h\"\n #include \"gettext.h\"\n #include \"hash.h\"\n@@ -12,14 +11,14 @@ static const char * const apply_usage[] = {\n int cmd_apply(int argc,\n \t const char **argv,\n \t const char *prefix,\n-\t struct repository *repo UNUSED)\n+\t struct repository *repo)\n {\n \tint force_apply = 0;\n \tint options = 0;\n \tint ret;\n \tstruct apply_state state;\n \n-\tif (init_apply_state(&state, the_repository, prefix))\n+\tif (init_apply_state(&state, repo, prefix))\n \t\texit(128);\n \n \t/*\n@@ -28,8 +27,8 @@ int cmd_apply(int argc,\n \t * is worth the effort.\n \t * cf. https://lore.kernel.org/git/xmqqcypfcmn4.fsf@gitster.g/\n \t */\n-\tif (!the_hash_algo)\n-\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n+\tif (!repo->hash_algo)\n+\t\trepo_set_hash_algo(repo, GIT_HASH_SHA1);\n \n \targc = apply_parse_options(argc, argv,\n \t\t\t\t &state, &force_apply, &options,\n-- \ngitgitgadget\n\n"},{"id":"503763","messageId":"xmqqldz953qt.fsf@gitster.g","threadId":"62185","inReplyTo":"5d72c31c6f3b97b7f5f7d3b4fa9a8b1587597670.1727718030.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/4] git: pass in repo for RUN_SETUP_GENTLY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-30T19:40:58Z","receivedAt":"2024-09-30T19:41:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> commands that have RUN_SETUP_GENTLY potentially need a repository.\n> Modify the logic in run_builtin() to pass the repository to the builtin\n> if a builtin has the RUN_SETUP_GENTLY property.\n>\n> Signed-off-by: John Cai \n> ---\n> git.c | 11 +++++++++--\n> 1 file changed, 9 insertions(+), 2 deletions(-)\n>\n> diff --git a/git.c b/git.c\n> index 2fbea24ec92..f58f169f3c7 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -443,7 +443,7 @@ static int handle_alias(int *argcp, const char ***argv)\n> \n> static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)\n> {\n> -\tint status, help;\n> +\tint status, help, repo_exists;\n> \tstruct stat st;\n> \tconst char *prefix;\n> \tint run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));\n> @@ -455,9 +455,13 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n> \n> \tif (run_setup & RUN_SETUP) {\n> \t\tprefix = setup_git_directory();\n> +\t\trepo_exists = 1;\n> \t} else if (run_setup & RUN_SETUP_GENTLY) {\n> \t\tint nongit_ok;\n> \t\tprefix = setup_git_directory_gently(&nongit_ok);\n> +\n> +\t\tif (!nongit_ok)\n> +\t\t\trepo_exists = 1;\n\nWhy not use the new variable and pass it directly to nongit_ok? The\npolarity of the new variable needs to be swapped, of course, but I\nthink it makes reading the code to call p->fn() easier to grok\n\ni.e., \n\n - rename repo_exists to \"no_repo\", and initialize it to non-zero.\n - remove \"int nongit_ok\"; pass &no_repo instead.\n - update the calling of p->fn() to\n\n\tp->fn(argc, argv, prefix, no_repo ? NULL : repo);\n\n> \t} else {\n> \t\tprefix = NULL;\n> \t}\n> @@ -480,7 +484,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n> \ttrace2_cmd_name(p->cmd);\n> \n> \tvalidate_cache_entries(repo->index);\n> -\tstatus = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);\n> +\tstatus = p->fn(argc,\n> +\t\t argv,\n> +\t\t prefix,\n> +\t\t repo_exists ? repo : NULL);\n\nKeeping the call to a single line would be much better than spreding\nit across four lines.\n\nThanks\n\n> \tvalidate_cache_entries(repo->index);\n> \n> \tif (status)\n"},{"id":"503764","messageId":"xmqqa5fp53m4.fsf@gitster.g","threadId":"62185","inReplyTo":"2a29d113815015b82d807d94f8d551c3f885cb9c.1727718031.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/4] annotate: remove usage of the_repository global","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-30T19:43:47Z","receivedAt":"2024-09-30T19:43:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> Remove the the_repository with the repository argument that gets passed\n> down through the builtin function.\n>\n> Signed-off-by: John Cai \n> ---\n> builtin/annotate.c | 5 ++---\n> 1 file changed, 2 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/annotate.c b/builtin/annotate.c\n> index a99179fe4dd..ce3dfaafb28 100644\n> --- a/builtin/annotate.c\n> +++ b/builtin/annotate.c\n> @@ -4,7 +4,6 @@\n> * Copyright (C) 2006 Ryan Anderson\n> */\n> \n> -#define USE_THE_REPOSITORY_VARIABLE\n> #include \"git-compat-util.h\"\n> #include \"builtin.h\"\n> #include \"strvec.h\"\n> @@ -12,7 +11,7 @@\n> int cmd_annotate(int argc,\n> \t\t const char **argv,\n> \t\t const char *prefix,\n> -\t\t struct repository *repo UNUSED)\n> +\t\t struct repository *repo)\n> {\n> \tstruct strvec args = STRVEC_INIT;\n> \tint i;\n> @@ -23,5 +22,5 @@ int cmd_annotate(int argc,\n> \t\tstrvec_push(&args, argv[i]);\n> \t}\n> \n> -\treturn cmd_blame(args.nr, args.v, prefix, the_repository);\n> +\treturn cmd_blame(args.nr, args.v, prefix, repo);\n> }\n\nThis looks obviously correct. Nicely done.\n"},{"id":"503766","messageId":"xmqq1q106hdd.fsf@gitster.g","threadId":"62185","inReplyTo":"857291d7f7dffdd1a63ce9268c8ac91a82f2bdb5.1727718031.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/4] archive: remove the_repository global variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-30T20:01:18Z","receivedAt":"2024-09-30T20:01:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> -#define USE_THE_REPOSITORY_VARIABLE\n> #include \"builtin.h\"\n> #include \"archive.h\"\n> #include \"gettext.h\"\n> @@ -79,7 +78,7 @@ static int run_remote_archiver(int argc, const char **argv,\n> int cmd_archive(int argc,\n> \t\tconst char **argv,\n> \t\tconst char *prefix,\n> -\t\tstruct repository *repo UNUSED)\n> +\t\tstruct repository *repo)\n> {\n> \tconst char *exec = \"git-upload-archive\";\n> \tchar *output = NULL;\n> @@ -110,7 +109,7 @@ int cmd_archive(int argc,\n> \n> \tsetvbuf(stderr, NULL, _IOLBF, BUFSIZ);\n> \n> -\tret = write_archive(argc, argv, prefix, the_repository, output, 0);\n> +\tret = write_archive(argc, argv, prefix, repo, output, 0);\n\nThis looks OK, but unlike the original, write_archive() now needs to\nbe prepared to see NULL in the repo parameter. Is that reasonable?\n\nPerdon me to think aloud for a bit.\n\nThe context before this hunk handles \"git archive --remote\" which\ncan be run outside a repository (and that is the whole reason why we\nask SETUP_GENTLY), but this part has to happen in a repository,\ndoesn't it? Or is there some mode of operation of \"git archive\" I\nam forgetting that can be done without a repository?\n\n\t... goes and looks ...\n\nOK, write_archive() has its own setup_git_directory() call when\nstartup_info->have_repository is false, so this happens to be OK,\nuntil the beginning part of archive.c:write_archive() will not\nchanged to start dereferencing \"repo\" pointer.\n\nThat sounds brittle, but probalby outside the scope of what this\npatch series wants to address. It also makes git_config() calls\neven before it realizes there is no repository and dies, which\nsmells fishy without actually doing any harm.\n\nSo, after all, I think this step is probably OK.\n\nThanks.\n\n"},{"id":"503767","messageId":"xmqqy13852jk.fsf@gitster.g","threadId":"62185","inReplyTo":"d64955a2e277da138146020f6a0cf96f4636a162.1727718031.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/4] apply: remove the_repository global variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-30T20:06:55Z","receivedAt":"2024-09-30T20:06:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> Remove the_repository global variable in favor of the repository\n> argument that gets passed in through the builtin function.\n>\n> Signed-off-by: John Cai \n> ---\n> builtin/apply.c | 9 ++++-----\n> 1 file changed, 4 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 84f1863d3ac..d0bafbec7e4 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -1,4 +1,3 @@\n> -#define USE_THE_REPOSITORY_VARIABLE\n> #include \"builtin.h\"\n> #include \"gettext.h\"\n> #include \"hash.h\"\n> @@ -12,14 +11,14 @@ static const char * const apply_usage[] = {\n> int cmd_apply(int argc,\n> \t const char **argv,\n> \t const char *prefix,\n> -\t struct repository *repo UNUSED)\n> +\t struct repository *repo)\n> {\n> \tint force_apply = 0;\n> \tint options = 0;\n> \tint ret;\n> \tstruct apply_state state;\n> \n> -\tif (init_apply_state(&state, the_repository, prefix))\n> +\tif (init_apply_state(&state, repo, prefix))\n> \t\texit(128);\n\nIs this one, and ...\n\n> \n> \t/*\n> @@ -28,8 +27,8 @@ int cmd_apply(int argc,\n> \t * is worth the effort.\n> \t * cf. https://lore.kernel.org/git/xmqqcypfcmn4.fsf@gitster.g/\n> \t */\n> -\tif (!the_hash_algo)\n> -\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n> +\tif (!repo->hash_algo)\n> +\t\trepo_set_hash_algo(repo, GIT_HASH_SHA1);\n\n... is this use of \"repo\" still valid? We now pass NULL, not\nthe_repository, when a command with SETUP_GENTLY is asked to run\noutside a repository, no? Shouldn't it detecting the case, and\npassing the pointer to a fallback object (perhaps the_repository)\ninstead of repo?\n\nI _think_ state->repo->index is accessed unconditionally only to\nfigure out whitespace attributes, even outside a repository (thanks\nto the_repository standing in), with the expectation that the index\nis empty (because we do not read any) and we find no customization,\nwhen \"git apply\" is used as a better GNU patch to work outside any\nrepository. Maybe I am not looking hard enough, but I fail to see\nhow the code makes sure that repo being NULL outside a repository\ndoes not lead to a dereferencing of a NULL pointer.\n\nThanks.\n\n\n\n"},{"id":"503796","messageId":"Zvt4zILqF5Ujorn6@ArchLinux","threadId":"62185","inReplyTo":"5d72c31c6f3b97b7f5f7d3b4fa9a8b1587597670.1727718030.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/4] git: pass in repo for RUN_SETUP_GENTLY","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-10-01T04:21:32Z","receivedAt":"2024-10-01T04:21:31Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, Sep 30, 2024 at 05:40:27PM +0000, John Cai via GitGitGadget wrote:\n> From: John Cai \n> \n> commands that have RUN_SETUP_GENTLY potentially need a repository.\n> Modify the logic in run_builtin() to pass the repository to the builtin\n> if a builtin has the RUN_SETUP_GENTLY property.\n> \n\nWe will parse the \"repo\" to the \"run_builtin()\" for \"RUN_SETUP_GENTLY\"\nproperty only when we know we run the command in the repository. If we\nrun the command outside of the repository, we should pass the NULL.\n\nHowever, the above commit message is not accurate. If a builtin has the\n\"RUN_SETUP_GENTLY\" property. We may pass or not.\n\n> Signed-off-by: John Cai \n> ---\n> git.c | 11 +++++++++--\n> 1 file changed, 9 insertions(+), 2 deletions(-)\n> \n> diff --git a/git.c b/git.c\n> index 2fbea24ec92..f58f169f3c7 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -443,7 +443,7 @@ static int handle_alias(int *argcp, const char ***argv)\n> \n> static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)\n> {\n> -\tint status, help;\n> +\tint status, help, repo_exists;\n\nThis is wrong. We should initialize the \"repo_exists\" variable here.\nBecause we never set this variable to 0 in the later code path. It will\nalways be true for the following code:\n\n repo_exists ? repo : NULL\n\nIt will always evaluate to the \"repo\". This may could answer the\nquestion raised by Junio in [PATCH v2 3/4].\n\nThanks,\nJialuo\n"},{"id":"503799","messageId":"ZvuBduVg9TJeULpl@ArchLinux","threadId":"62185","inReplyTo":"xmqqy13852jk.fsf@gitster.g","subject":"Re: [PATCH v2 3/4] apply: remove the_repository global variable","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-10-01T04:58:30Z","receivedAt":"2024-10-01T04:58:29Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Mon, Sep 30, 2024 at 01:06:55PM -0700, Junio C Hamano wrote:\n> > \t/*\n> > @@ -28,8 +27,8 @@ int cmd_apply(int argc,\n> > \t * is worth the effort.\n> > \t * cf. https://lore.kernel.org/git/xmqqcypfcmn4.fsf@gitster.g/\n> > \t */\n> > -\tif (!the_hash_algo)\n> > -\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n> > +\tif (!repo->hash_algo)\n> > +\t\trepo_set_hash_algo(repo, GIT_HASH_SHA1);\n> \n> ... is this use of \"repo\" still valid? We now pass NULL, not\n> the_repository, when a command with SETUP_GENTLY is asked to run\n> outside a repository, no? Shouldn't it detecting the case, and\n> passing the pointer to a fallback object (perhaps the_repository)\n> instead of repo?\n> \n\nThis is a bad usage. Although the \"t1517: apply a patch outside\nrepository\" should check the code, the uninitialized variable\n\"repo_exists\" will cause \"repo_exists ? repo : NULL\" to always be\n\"repo\" which hides the wrong usage of the \"repo_exists\".\n\nBy fetching the tree, I initialize the \"repo_exists = 0\" for the [PATCH\nv2 1/4]. And there are many tests failed. Many builtins with\n\"RUN_SETUP_GENTLY\" property or which could be converted to\n\"RUN_SETUP_GENTLY\" property by ONLY \"-h\" parameter will fail\n(segmentation fault). It's obvious that we use NULL pointer for \"repo\".\n\nIn my opinion, we should first think about how we handle the situation\nwhere we run builtins outside of the repository. The most easiest way is\nto pass the fallback object (aka \"the_repository\").\n\nHowever, this seems a little strange. We are truly outside of the\nrepository but we really rely on the \"struct repository *\" to do many\noperations. It's unrealistic to change so many interfaces which use the\n\"struct repository *\". So, we should just use the fallback idea at\ncurrent.\n\nThanks,\nJialuo\n"},{"id":"503837","messageId":"Zvvr1_9syRh1McVA@pks.im","threadId":"62185","inReplyTo":"ZvuBduVg9TJeULpl@ArchLinux","subject":"Re: [PATCH v2 3/4] apply: remove the_repository global variable","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-01T12:32:30Z","receivedAt":"2024-10-01T12:32:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Oct 01, 2024 at 12:58:30PM +0800, shejialuo wrote:\n> On Mon, Sep 30, 2024 at 01:06:55PM -0700, Junio C Hamano wrote:\n> In my opinion, we should first think about how we handle the situation\n> where we run builtins outside of the repository. The most easiest way is\n> to pass the fallback object (aka \"the_repository\").\n> \n> However, this seems a little strange. We are truly outside of the\n> repository but we really rely on the \"struct repository *\" to do many\n> operations. It's unrealistic to change so many interfaces which use the\n> \"struct repository *\". So, we should just use the fallback idea at\n> current.\n\nI disagree with this statement. If code isn't prepare to not handle a\n`NULL` repository we shouldn't fall back to `the_repository`, but we\nshould instead prepare the code to handle this case. This of course\nrequires us to do a ton of refactorings, but that is the idea of this\nwhole exercise to get rid of `the_repository`.\n\nIf a command cannot be converted to stop using `the_repository` right\nnow we should skip it and revisit once all prerequisites have been\nadapted accordingly.\n\nPatrick\n"},{"id":"503839","messageId":"Zvv723-OwvEr0qMV@ArchLinux","threadId":"62185","inReplyTo":"Zvvr1_9syRh1McVA@pks.im","subject":"Re: [PATCH v2 3/4] apply: remove the_repository global variable","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-10-01T13:40:43Z","receivedAt":"2024-10-01T13:40:42Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Tue, Oct 01, 2024 at 02:32:30PM +0200, Patrick Steinhardt wrote:\n> On Tue, Oct 01, 2024 at 12:58:30PM +0800, shejialuo wrote:\n> > On Mon, Sep 30, 2024 at 01:06:55PM -0700, Junio C Hamano wrote:\n> > In my opinion, we should first think about how we handle the situation\n> > where we run builtins outside of the repository. The most easiest way is\n> > to pass the fallback object (aka \"the_repository\").\n> > \n> > However, this seems a little strange. We are truly outside of the\n> > repository but we really rely on the \"struct repository *\" to do many\n> > operations. It's unrealistic to change so many interfaces which use the\n> > \"struct repository *\". So, we should just use the fallback idea at\n> > current.\n> \n> I disagree with this statement. If code isn't prepare to not handle a\n> `NULL` repository we shouldn't fall back to `the_repository`, but we\n> should instead prepare the code to handle this case. This of course\n> requires us to do a ton of refactorings, but that is the idea of this\n> whole exercise to get rid of `the_repository`.\n> \n\nActually, I also insist that we should refactor here. But I worry about\nthe burden this would bring to John due to we may do a lot of work here.\nSo, I expressed my meaning in a compromising way.\n\nBut we should face the problem directly :).\n\nThanks,\nJialuo\n"},{"id":"503840","messageId":"ZvwCj4J_HRiSF_S0@pks.im","threadId":"62185","inReplyTo":"Zvv723-OwvEr0qMV@ArchLinux","subject":"Re: [PATCH v2 3/4] apply: remove the_repository global variable","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-01T14:09:25Z","receivedAt":"2024-10-01T14:09:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Oct 01, 2024 at 09:40:43PM +0800, shejialuo wrote:\n> On Tue, Oct 01, 2024 at 02:32:30PM +0200, Patrick Steinhardt wrote:\n> > On Tue, Oct 01, 2024 at 12:58:30PM +0800, shejialuo wrote:\n> > > On Mon, Sep 30, 2024 at 01:06:55PM -0700, Junio C Hamano wrote:\n> > > In my opinion, we should first think about how we handle the situation\n> > > where we run builtins outside of the repository. The most easiest way is\n> > > to pass the fallback object (aka \"the_repository\").\n> > > \n> > > However, this seems a little strange. We are truly outside of the\n> > > repository but we really rely on the \"struct repository *\" to do many\n> > > operations. It's unrealistic to change so many interfaces which use the\n> > > \"struct repository *\". So, we should just use the fallback idea at\n> > > current.\n> > \n> > I disagree with this statement. If code isn't prepare to not handle a\n> > `NULL` repository we shouldn't fall back to `the_repository`, but we\n> > should instead prepare the code to handle this case. This of course\n> > requires us to do a ton of refactorings, but that is the idea of this\n> > whole exercise to get rid of `the_repository`.\n> > \n> \n> Actually, I also insist that we should refactor here. But I worry about\n> the burden this would bring to John due to we may do a lot of work here.\n> So, I expressed my meaning in a compromising way.\n> \n> But we should face the problem directly :).\n\nTrue, all of this is a long-term effort that is probably going to take\nus many months, likely even years. So people working on it should take\nthings slow and refactor chunks that are mostly ready to be converted to\nget rid of `the_repository`.\n\nThat will sometimes mean that you have to scrap the conversion you're\ncurrently working on because you discover that it inherently relies on\n`the_repository` deep down in the stack, and refactoring it would be a\nhuge undertaking. That definitely happened to me multiple times while\nintroducing `USE_THE_REPOSITORY_VARIABLE`. And every time I did discover\nthat, I went one level deeper to try and fix the underpinnings first.\n\nI mostly don't want us to blur the lines by silently falling back to\n`the_repository` in situations where we don't intend to. So I'd rather\ngo a bit slower overall and design the code such that it doesn't fall\nback anymore as a way to prove that something is indeed not relying on\n`the_repository` anymore. Otherwise we're going to make everyones life\nharder.\n\nPatrick\n"},{"id":"503847","messageId":"xmqqwmirzr32.fsf@gitster.g","threadId":"62185","inReplyTo":"Zvvr1_9syRh1McVA@pks.im","subject":"Re: [PATCH v2 3/4] apply: remove the_repository global variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-01T17:10:57Z","receivedAt":"2024-10-01T17:11:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> I disagree with this statement. If code isn't prepare to not handle a\n> `NULL` repository we shouldn't fall back to `the_repository`, but we\n> should instead prepare the code to handle this case. This of course\n> requires us to do a ton of refactorings, but that is the idea of this\n> whole exercise to get rid of `the_repository`.\n\nI agree. To me, the patch was screaming that the author was not\nprepared to go the whole nine yards, though. Adding back the\nexplicit reference to \"the_repository\" as a fallback is the next\nbest thing to do, pushing the \"problem\" closer to where it is.\n\n> If a command cannot be converted to stop using `the_repository` right\n> now we should skip it and revisit once all prerequisites have been\n> adapted accordingly.\n\nThat is also a viable approach.\n"},{"id":"504038","messageId":"DCEF5258-BFF7-4667-A538-0BEED2CB70AA@gmail.com","threadId":"62185","inReplyTo":"xmqqwmirzr32.fsf@gitster.g","subject":"Re: [PATCH v2 3/4] apply: remove the_repository global variable","fromName":"","fromEmail":"johncai86@gmail.com","sentAt":"2024-10-03T18:28:13Z","receivedAt":"2024-10-03T18:28:27Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn 1 Oct 2024, at 13:10, Junio C Hamano wrote:\n\n> Patrick Steinhardt writes:\n>\n>> I disagree with this statement. If code isn't prepare to not handle a\n>> `NULL` repository we shouldn't fall back to `the_repository`, but we\n>> should instead prepare the code to handle this case. This of course\n>> requires us to do a ton of refactorings, but that is the idea of this\n>> whole exercise to get rid of `the_repository`.\n>\n> I agree. To me, the patch was screaming that the author was not\n> prepared to go the whole nine yards, though. Adding back the\n> explicit reference to \"the_repository\" as a fallback is the next\n> best thing to do, pushing the \"problem\" closer to where it is.\n>\n\nIndeed, I did not do my due diligence here instead of assuming all layers\nthat look at the repo argument do the right thing.\n\n>> If a command cannot be converted to stop using `the_repository` right\n>> now we should skip it and revisit once all prerequisites have been\n>> adapted accordingly.\n\nLooks like it’d be preferable if I just drop this patch from the series as\nit will require a larger refactor.\n\nThanks\nJohn\n\n>\n> That is also a viable approach.\n"},{"id":"504112","messageId":"40DAFE71-EC0C-4FC4-B7C4-F054562E8B19@gmail.com","threadId":"62185","inReplyTo":"xmqq1q106hdd.fsf@gitster.g","subject":"Re: [PATCH v2 4/4] archive: remove the_repository global variable","fromName":"","fromEmail":"johncai86@gmail.com","sentAt":"2024-10-04T20:05:54Z","receivedAt":"2024-10-04T20:06:08Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn 30 Sep 2024, at 16:01, Junio C Hamano wrote:\n\n> \"John Cai via GitGitGadget\" writes:\n>\n>> -#define USE_THE_REPOSITORY_VARIABLE\n>> #include \"builtin.h\"\n>> #include \"archive.h\"\n>> #include \"gettext.h\"\n>> @@ -79,7 +78,7 @@ static int run_remote_archiver(int argc, const char **argv,\n>> int cmd_archive(int argc,\n>> \t\tconst char **argv,\n>> \t\tconst char *prefix,\n>> -\t\tstruct repository *repo UNUSED)\n>> +\t\tstruct repository *repo)\n>> {\n>> \tconst char *exec = \"git-upload-archive\";\n>> \tchar *output = NULL;\n>> @@ -110,7 +109,7 @@ int cmd_archive(int argc,\n>>\n>> \tsetvbuf(stderr, NULL, _IOLBF, BUFSIZ);\n>>\n>> -\tret = write_archive(argc, argv, prefix, the_repository, output, 0);\n>> +\tret = write_archive(argc, argv, prefix, repo, output, 0);\n>\n> This looks OK, but unlike the original, write_archive() now needs to\n> be prepared to see NULL in the repo parameter. Is that reasonable?\n>\n> Perdon me to think aloud for a bit.\n>\n> The context before this hunk handles \"git archive --remote\" which\n> can be run outside a repository (and that is the whole reason why we\n> ask SETUP_GENTLY), but this part has to happen in a repository,\n> doesn't it? Or is there some mode of operation of \"git archive\" I\n> am forgetting that can be done without a repository?\n>\n> ... goes and looks ...\n>\n> OK, write_archive() has its own setup_git_directory() call when\n> startup_info->have_repository is false, so this happens to be OK,\n> until the beginning part of archive.c:write_archive() will not\n> changed to start dereferencing \"repo\" pointer.\n>\n> That sounds brittle, but probalby outside the scope of what this\n> patch series wants to address. It also makes git_config() calls\n> even before it realizes there is no repository and dies, which\n> smells fishy without actually doing any harm.\n>\n> So, after all, I think this step is probably OK.\n\nYeah I think these are issues we’ll need to address once removing the\nthe_repository global from archive code.\n\nThanks\nJohn\n\n>\n> Thanks.\n"},{"id":"504126","messageId":"pull.1788.v3.git.git.1728099043.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":"pull.1788.v2.git.git.1727718030.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] Remove the_repository global for am, annotate, apply, archive builtins","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-05T03:30:40Z","receivedAt":"2024-10-05T03:30:47Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Remove the_repository global variable for the annotate, apply, and archive\nbulitins.\n\nChanges since V1:\n\n * in patch 1, only pass in repo to the bulitin if the repo exists\n\nChanges since V2:\n\n * drop patch 3, which is a bit more involved to dis-entangle the_repository\n * use a single variable in run_builtin() to keep track of whether or not we\n are operating in a repository\n\nJohn Cai (3):\n git: pass in repo to builtin based on setup_git_directory_gently\n annotate: remove usage of the_repository global\n archive: remove the_repository global variable\n\n builtin/add.c | 3 ++-\n builtin/annotate.c | 5 ++---\n builtin/archive.c | 5 ++---\n git.c | 7 ++++---\n 4 files changed, 10 insertions(+), 10 deletions(-)\n\n\nbase-commit: 3857aae53f3633b7de63ad640737c657387ae0c6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1788%2Fjohn-cai%2Fjc%2Fremove-global-repo-a-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1788/john-cai/jc/remove-global-repo-a-v3\nPull-Request: https://github.com/git/git/pull/1788\n\nRange-diff vs v2:\n\n 1: 5d72c31c6f3 ! 1: 8009fdb38b0 git: pass in repo for RUN_SETUP_GENTLY\n @@ Metadata\n Author: John Cai \n \n ## Commit message ##\n - git: pass in repo for RUN_SETUP_GENTLY\n + git: pass in repo to builtin based on setup_git_directory_gently\n \n - commands that have RUN_SETUP_GENTLY potentially need a repository.\n - Modify the logic in run_builtin() to pass the repository to the builtin\n - if a builtin has the RUN_SETUP_GENTLY property.\n + The current code in run_builtin() passes in a repository to the builtin\n + based on whether cmd_struct's option flag has RUN_SETUP.\n +\n + This is incorrect, however, since some builtins that only have\n + RUN_SETUP_GENTLY can potentially take a repository.\n + setup_git_directory_gently() tells us whether or not a command is being\n + run inside of a repository.\n +\n + Use the output of setup_git_directory_gently() to help determine whether\n + or not there is a repository to pass to the builtin. If not, then we\n + just pass NULL.\n +\n + As part of this patch, we need to modify add to check for a NULL repo\n + before calling repo_git_config(), since add -h can be run outside of a\n + repository.\n \n Signed-off-by: John Cai \n \n + ## builtin/add.c ##\n +@@ builtin/add.c: int cmd_add(int argc,\n + \tchar *ps_matched = NULL;\n + \tstruct lock_file lock_file = LOCK_INIT;\n + \n +-\trepo_config(repo, add_config, NULL);\n ++\tif (repo)\n ++\t\trepo_config(repo, add_config, NULL);\n + \n + \targc = parse_options(argc, argv, prefix, builtin_add_options,\n + \t\t\t builtin_add_usage, PARSE_OPT_KEEP_ARGV0);\n +\n ## git.c ##\n @@ git.c: static int handle_alias(int *argcp, const char ***argv)\n - \n static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)\n {\n --\tint status, help;\n -+\tint status, help, repo_exists;\n + \tint status, help;\n ++\tint no_repo = 1;\n \tstruct stat st;\n \tconst char *prefix;\n \tint run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));\n @@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv,\n \n \tif (run_setup & RUN_SETUP) {\n \t\tprefix = setup_git_directory();\n -+\t\trepo_exists = 1;\n ++\t\tno_repo = 0;\n \t} else if (run_setup & RUN_SETUP_GENTLY) {\n - \t\tint nongit_ok;\n - \t\tprefix = setup_git_directory_gently(&nongit_ok);\n -+\n -+\t\tif (!nongit_ok)\n -+\t\t\trepo_exists = 1;\n +-\t\tint nongit_ok;\n +-\t\tprefix = setup_git_directory_gently(&nongit_ok);\n ++\t\tprefix = setup_git_directory_gently(&no_repo);\n \t} else {\n \t\tprefix = NULL;\n \t}\n @@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv,\n \n \tvalidate_cache_entries(repo->index);\n -\tstatus = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);\n -+\tstatus = p->fn(argc,\n -+\t\t argv,\n -+\t\t prefix,\n -+\t\t repo_exists ? repo : NULL);\n ++\tstatus = p->fn(argc, argv, prefix, no_repo ? NULL : repo);\n \tvalidate_cache_entries(repo->index);\n \n \tif (status)\n 2: 2a29d113815 = 2: 1b82b5dc678 annotate: remove usage of the_repository global\n 3: d64955a2e27 < -: ----------- apply: remove the_repository global variable\n 4: 857291d7f7d = 3: 5d33a375f41 archive: remove the_repository global variable\n\n-- \ngitgitgadget\n"},{"id":"504127","messageId":"8009fdb38b0b4c3880588119b99ac5387d398540.1728099043.git.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":"pull.1788.v3.git.git.1728099043.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] git: pass in repo to builtin based on setup_git_directory_gently","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-05T03:30:41Z","receivedAt":"2024-10-05T03:30:47Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nThe current code in run_builtin() passes in a repository to the builtin\nbased on whether cmd_struct's option flag has RUN_SETUP.\n\nThis is incorrect, however, since some builtins that only have\nRUN_SETUP_GENTLY can potentially take a repository.\nsetup_git_directory_gently() tells us whether or not a command is being\nrun inside of a repository.\n\nUse the output of setup_git_directory_gently() to help determine whether\nor not there is a repository to pass to the builtin. If not, then we\njust pass NULL.\n\nAs part of this patch, we need to modify add to check for a NULL repo\nbefore calling repo_git_config(), since add -h can be run outside of a\nrepository.\n\nSigned-off-by: John Cai \n---\n builtin/add.c | 3 ++-\n git.c | 7 ++++---\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 773b7224a49..7d353077921 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -385,7 +385,8 @@ int cmd_add(int argc,\n \tchar *ps_matched = NULL;\n \tstruct lock_file lock_file = LOCK_INIT;\n \n-\trepo_config(repo, add_config, NULL);\n+\tif (repo)\n+\t\trepo_config(repo, add_config, NULL);\n \n \targc = parse_options(argc, argv, prefix, builtin_add_options,\n \t\t\t builtin_add_usage, PARSE_OPT_KEEP_ARGV0);\ndiff --git a/git.c b/git.c\nindex 2fbea24ec92..47741be3e4c 100644\n--- a/git.c\n+++ b/git.c\n@@ -444,6 +444,7 @@ static int handle_alias(int *argcp, const char ***argv)\n static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)\n {\n \tint status, help;\n+\tint no_repo = 1;\n \tstruct stat st;\n \tconst char *prefix;\n \tint run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));\n@@ -455,9 +456,9 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \n \tif (run_setup & RUN_SETUP) {\n \t\tprefix = setup_git_directory();\n+\t\tno_repo = 0;\n \t} else if (run_setup & RUN_SETUP_GENTLY) {\n-\t\tint nongit_ok;\n-\t\tprefix = setup_git_directory_gently(&nongit_ok);\n+\t\tprefix = setup_git_directory_gently(&no_repo);\n \t} else {\n \t\tprefix = NULL;\n \t}\n@@ -480,7 +481,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \ttrace2_cmd_name(p->cmd);\n \n \tvalidate_cache_entries(repo->index);\n-\tstatus = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);\n+\tstatus = p->fn(argc, argv, prefix, no_repo ? NULL : repo);\n \tvalidate_cache_entries(repo->index);\n \n \tif (status)\n-- \ngitgitgadget\n\n"},{"id":"504128","messageId":"1b82b5dc6782e21eebf019585d2aac704ec9e8f0.1728099043.git.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":"pull.1788.v3.git.git.1728099043.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] annotate: remove usage of the_repository global","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-05T03:30:42Z","receivedAt":"2024-10-05T03:30:48Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nRemove the the_repository with the repository argument that gets passed\ndown through the builtin function.\n\nSigned-off-by: John Cai \n---\n builtin/annotate.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/annotate.c b/builtin/annotate.c\nindex a99179fe4dd..ce3dfaafb28 100644\n--- a/builtin/annotate.c\n+++ b/builtin/annotate.c\n@@ -4,7 +4,6 @@\n * Copyright (C) 2006 Ryan Anderson\n */\n \n-#define USE_THE_REPOSITORY_VARIABLE\n #include \"git-compat-util.h\"\n #include \"builtin.h\"\n #include \"strvec.h\"\n@@ -12,7 +11,7 @@\n int cmd_annotate(int argc,\n \t\t const char **argv,\n \t\t const char *prefix,\n-\t\t struct repository *repo UNUSED)\n+\t\t struct repository *repo)\n {\n \tstruct strvec args = STRVEC_INIT;\n \tint i;\n@@ -23,5 +22,5 @@ int cmd_annotate(int argc,\n \t\tstrvec_push(&args, argv[i]);\n \t}\n \n-\treturn cmd_blame(args.nr, args.v, prefix, the_repository);\n+\treturn cmd_blame(args.nr, args.v, prefix, repo);\n }\n-- \ngitgitgadget\n\n"},{"id":"504129","messageId":"5d33a375f41132b8b378885d00e934b9f20a0854.1728099043.git.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":"pull.1788.v3.git.git.1728099043.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] archive: remove the_repository global variable","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-05T03:30:43Z","receivedAt":"2024-10-05T03:30:49Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nReplace the_repository with the repository argument that gets passed\ndown through the builtin function.\n\nSigned-off-by: John Cai \n---\n builtin/archive.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex dc926d1a3df..13ea7308c8b 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -2,7 +2,6 @@\n * Copyright (c) 2006 Franck Bui-Huu\n * Copyright (c) 2006 Rene Scharfe\n */\n-#define USE_THE_REPOSITORY_VARIABLE\n #include \"builtin.h\"\n #include \"archive.h\"\n #include \"gettext.h\"\n@@ -79,7 +78,7 @@ static int run_remote_archiver(int argc, const char **argv,\n int cmd_archive(int argc,\n \t\tconst char **argv,\n \t\tconst char *prefix,\n-\t\tstruct repository *repo UNUSED)\n+\t\tstruct repository *repo)\n {\n \tconst char *exec = \"git-upload-archive\";\n \tchar *output = NULL;\n@@ -110,7 +109,7 @@ int cmd_archive(int argc,\n \n \tsetvbuf(stderr, NULL, _IOLBF, BUFSIZ);\n \n-\tret = write_archive(argc, argv, prefix, the_repository, output, 0);\n+\tret = write_archive(argc, argv, prefix, repo, output, 0);\n \n out:\n \tfree(output);\n-- \ngitgitgadget\n"},{"id":"504132","messageId":"ZwDh6XAKIhUF_Lu6@ArchLinux","threadId":"62185","inReplyTo":"8009fdb38b0b4c3880588119b99ac5387d398540.1728099043.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/3] git: pass in repo to builtin based on setup_git_directory_gently","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-10-05T06:51:21Z","receivedAt":"2024-10-05T06:51:17Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Sat, Oct 05, 2024 at 03:30:41AM +0000, John Cai via GitGitGadget wrote:\n> From: John Cai \n> \n> The current code in run_builtin() passes in a repository to the builtin\n> based on whether cmd_struct's option flag has RUN_SETUP.\n> \n> This is incorrect, however, since some builtins that only have\n> RUN_SETUP_GENTLY can potentially take a repository.\n> setup_git_directory_gently() tells us whether or not a command is being\n> run inside of a repository.\n> \n> Use the output of setup_git_directory_gently() to help determine whether\n> or not there is a repository to pass to the builtin. If not, then we\n> just pass NULL.\n> \n> As part of this patch, we need to modify add to check for a NULL repo\n> before calling repo_git_config(), since add -h can be run outside of a\n> repository.\n> \n> Signed-off-by: John Cai \n> ---\n> builtin/add.c | 3 ++-\n> git.c | 7 ++++---\n> 2 files changed, 6 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/add.c b/builtin/add.c\n> index 773b7224a49..7d353077921 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -385,7 +385,8 @@ int cmd_add(int argc,\n> \tchar *ps_matched = NULL;\n> \tstruct lock_file lock_file = LOCK_INIT;\n> \n> -\trepo_config(repo, add_config, NULL);\n> +\tif (repo)\n> +\t\trepo_config(repo, add_config, NULL);\n\nThe reason why we need to check whether the `repo` is NULL is that when\nusing \"git add -h\", the RUN_SETUP flag would be converted to\nRUN_SETUP_GENTLY.\n\nI think this change is OK. But I wonder whether we should encapsulate\nthe logic into the \"repo_config\" function. It's a little cumbersome to\ncheck whether the \"repo\" exists. I also feel it's a bad idea to check in\nthe \"repo_config\" function because in the most time, we run commands\ninside the repository. So, in my view, at current, this is enough.\n\nThanks,\nJialuo\n"},{"id":"504133","messageId":"ZwDnHs92pEs0UJbN@ArchLinux","threadId":"62185","inReplyTo":"5d33a375f41132b8b378885d00e934b9f20a0854.1728099043.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/3] archive: remove the_repository global variable","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-10-05T07:13:34Z","receivedAt":"2024-10-05T07:13:29Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Sat, Oct 05, 2024 at 03:30:43AM +0000, John Cai via GitGitGadget wrote:\n> diff --git a/builtin/archive.c b/builtin/archive.c\n> index dc926d1a3df..13ea7308c8b 100644\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n\n[snip]\n\n> \n> -\tret = write_archive(argc, argv, prefix, the_repository, output, 0);\n> +\tret = write_archive(argc, argv, prefix, repo, output, 0);\n> \n\nWhen I read this new series, I feel quite strange for why we only change\n\"the_repository\" to \"repo\". After reading the comments from [PATCH v2 4/4],\nI have understood the context.\n\nI think we should improve the commit message to take about we decide to\nremove the \"the_repository\" from \"archive.c\" code unless it will bring a\nlot of confusion for the reader.\n\nThanks,\nJialuo\n"},{"id":"504729","messageId":"4A5B361F-159C-421C-BC47-BEE3765E6F34@gmail.com","threadId":"62185","inReplyTo":"ZwDnHs92pEs0UJbN@ArchLinux","subject":"Re: [PATCH v3 3/3] archive: remove the_repository global variable","fromName":"","fromEmail":"johncai86@gmail.com","sentAt":"2024-10-10T18:27:58Z","receivedAt":"2024-10-10T18:28:12Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Jialuo,\n\nOn 5 Oct 2024, at 3:13, shejialuo wrote:\n\n> On Sat, Oct 05, 2024 at 03:30:43AM +0000, John Cai via GitGitGadget wrote:\n>> diff --git a/builtin/archive.c b/builtin/archive.c\n>> index dc926d1a3df..13ea7308c8b 100644\n>> --- a/builtin/archive.c\n>> +++ b/builtin/archive.c\n>\n> [snip]\n>\n>>\n>> -\tret = write_archive(argc, argv, prefix, the_repository, output, 0);\n>> +\tret = write_archive(argc, argv, prefix, repo, output, 0);\n>>\n>\n> When I read this new series, I feel quite strange for why we only change\n> \"the_repository\" to \"repo\". After reading the comments from [PATCH v2 4/4],\n> I have understood the context.\n>\n> I think we should improve the commit message to take about we decide to\n> remove the \"the_repository\" from \"archive.c\" code unless it will bring a\n> lot of confusion for the reader.\n\nSounds good. I will add more explanation for why we are making the change.\n\n>\n> Thanks,\n> Jialuo\n\nThanks\nJohn\n"},{"id":"504750","messageId":"pull.1788.v4.git.git.1728594828.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":"pull.1788.v3.git.git.1728099043.gitgitgadget@gmail.com","subject":"[PATCH v4 0/3] Remove the_repository global for am, annotate, apply, archive builtins","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-10T21:13:45Z","receivedAt":"2024-10-10T21:13:51Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Remove the_repository global variable for the annotate, apply, and archive\nbulitins.\n\nChanges since V3:\n\n * Improve commit message in patch 2\n\nChanges since V1:\n\n * in patch 1, only pass in repo to the bulitin if the repo exists\n\nChanges since V2:\n\n * drop patch 3, which is a bit more involved to dis-entangle the_repository\n * use a single variable in run_builtin() to keep track of whether or not we\n are operating in a repository\n\nJohn Cai (3):\n git: pass in repo to builtin based on setup_git_directory_gently\n annotate: remove usage of the_repository global\n archive: remove the_repository global variable\n\n builtin/add.c | 3 ++-\n builtin/annotate.c | 5 ++---\n builtin/archive.c | 5 ++---\n git.c | 7 ++++---\n 4 files changed, 10 insertions(+), 10 deletions(-)\n\n\nbase-commit: 3857aae53f3633b7de63ad640737c657387ae0c6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1788%2Fjohn-cai%2Fjc%2Fremove-global-repo-a-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1788/john-cai/jc/remove-global-repo-a-v4\nPull-Request: https://github.com/git/git/pull/1788\n\nRange-diff vs v3:\n\n 1: 8009fdb38b0 = 1: d59b85b5298 git: pass in repo to builtin based on setup_git_directory_gently\n 2: 1b82b5dc678 ! 2: f26d09215c3 annotate: remove usage of the_repository global\n @@ Metadata\n ## Commit message ##\n annotate: remove usage of the_repository global\n \n - Remove the the_repository with the repository argument that gets passed\n - down through the builtin function.\n + As part of the effort to get rid of global state due to the_repository\n + variable, remove the the_repository with the repository argument that\n + gets passed down through the builtin function.\n \n Signed-off-by: John Cai \n \n 3: 5d33a375f41 ! 3: 736212f34b5 archive: remove the_repository global variable\n @@ Metadata\n ## Commit message ##\n archive: remove the_repository global variable\n \n - Replace the_repository with the repository argument that gets passed\n - down through the builtin function.\n + As part of the effort to get rid of global state due to the global\n + the_repository variable, replace the_repository with the repository\n + argument that gets passed down through the builtin function.\n +\n + The repo might be NULL, but we should be safe in write_archive() because\n + it detects if we are outside of a repository and calls\n + setup_git_directory() which will error.\n \n Signed-off-by: John Cai \n \n\n-- \ngitgitgadget\n"},{"id":"504751","messageId":"d59b85b529865793c652d983d71a9fbb7e16b3e3.1728594828.git.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":"pull.1788.v4.git.git.1728594828.gitgitgadget@gmail.com","subject":"[PATCH v4 1/3] git: pass in repo to builtin based on setup_git_directory_gently","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-10T21:13:46Z","receivedAt":"2024-10-10T21:13:52Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nThe current code in run_builtin() passes in a repository to the builtin\nbased on whether cmd_struct's option flag has RUN_SETUP.\n\nThis is incorrect, however, since some builtins that only have\nRUN_SETUP_GENTLY can potentially take a repository.\nsetup_git_directory_gently() tells us whether or not a command is being\nrun inside of a repository.\n\nUse the output of setup_git_directory_gently() to help determine whether\nor not there is a repository to pass to the builtin. If not, then we\njust pass NULL.\n\nAs part of this patch, we need to modify add to check for a NULL repo\nbefore calling repo_git_config(), since add -h can be run outside of a\nrepository.\n\nSigned-off-by: John Cai \n---\n builtin/add.c | 3 ++-\n git.c | 7 ++++---\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 773b7224a49..7d353077921 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -385,7 +385,8 @@ int cmd_add(int argc,\n \tchar *ps_matched = NULL;\n \tstruct lock_file lock_file = LOCK_INIT;\n \n-\trepo_config(repo, add_config, NULL);\n+\tif (repo)\n+\t\trepo_config(repo, add_config, NULL);\n \n \targc = parse_options(argc, argv, prefix, builtin_add_options,\n \t\t\t builtin_add_usage, PARSE_OPT_KEEP_ARGV0);\ndiff --git a/git.c b/git.c\nindex 2fbea24ec92..47741be3e4c 100644\n--- a/git.c\n+++ b/git.c\n@@ -444,6 +444,7 @@ static int handle_alias(int *argcp, const char ***argv)\n static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)\n {\n \tint status, help;\n+\tint no_repo = 1;\n \tstruct stat st;\n \tconst char *prefix;\n \tint run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));\n@@ -455,9 +456,9 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \n \tif (run_setup & RUN_SETUP) {\n \t\tprefix = setup_git_directory();\n+\t\tno_repo = 0;\n \t} else if (run_setup & RUN_SETUP_GENTLY) {\n-\t\tint nongit_ok;\n-\t\tprefix = setup_git_directory_gently(&nongit_ok);\n+\t\tprefix = setup_git_directory_gently(&no_repo);\n \t} else {\n \t\tprefix = NULL;\n \t}\n@@ -480,7 +481,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \ttrace2_cmd_name(p->cmd);\n \n \tvalidate_cache_entries(repo->index);\n-\tstatus = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL);\n+\tstatus = p->fn(argc, argv, prefix, no_repo ? NULL : repo);\n \tvalidate_cache_entries(repo->index);\n \n \tif (status)\n-- \ngitgitgadget\n\n"},{"id":"504752","messageId":"f26d09215c3f40b07fa53ca638a058053dfdbffe.1728594828.git.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":"pull.1788.v4.git.git.1728594828.gitgitgadget@gmail.com","subject":"[PATCH v4 2/3] annotate: remove usage of the_repository global","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-10T21:13:47Z","receivedAt":"2024-10-10T21:13:52Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nAs part of the effort to get rid of global state due to the_repository\nvariable, remove the the_repository with the repository argument that\ngets passed down through the builtin function.\n\nSigned-off-by: John Cai \n---\n builtin/annotate.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/annotate.c b/builtin/annotate.c\nindex a99179fe4dd..ce3dfaafb28 100644\n--- a/builtin/annotate.c\n+++ b/builtin/annotate.c\n@@ -4,7 +4,6 @@\n * Copyright (C) 2006 Ryan Anderson\n */\n \n-#define USE_THE_REPOSITORY_VARIABLE\n #include \"git-compat-util.h\"\n #include \"builtin.h\"\n #include \"strvec.h\"\n@@ -12,7 +11,7 @@\n int cmd_annotate(int argc,\n \t\t const char **argv,\n \t\t const char *prefix,\n-\t\t struct repository *repo UNUSED)\n+\t\t struct repository *repo)\n {\n \tstruct strvec args = STRVEC_INIT;\n \tint i;\n@@ -23,5 +22,5 @@ int cmd_annotate(int argc,\n \t\tstrvec_push(&args, argv[i]);\n \t}\n \n-\treturn cmd_blame(args.nr, args.v, prefix, the_repository);\n+\treturn cmd_blame(args.nr, args.v, prefix, repo);\n }\n-- \ngitgitgadget\n\n"},{"id":"504753","messageId":"736212f34b5806de2fc12f5ebc5030bc71884cc7.1728594828.git.gitgitgadget@gmail.com","threadId":"62185","inReplyTo":"pull.1788.v4.git.git.1728594828.gitgitgadget@gmail.com","subject":"[PATCH v4 3/3] archive: remove the_repository global variable","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-10T21:13:48Z","receivedAt":"2024-10-10T21:13:54Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nAs part of the effort to get rid of global state due to the global\nthe_repository variable, replace the_repository with the repository\nargument that gets passed down through the builtin function.\n\nThe repo might be NULL, but we should be safe in write_archive() because\nit detects if we are outside of a repository and calls\nsetup_git_directory() which will error.\n\nSigned-off-by: John Cai \n---\n builtin/archive.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex dc926d1a3df..13ea7308c8b 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -2,7 +2,6 @@\n * Copyright (c) 2006 Franck Bui-Huu\n * Copyright (c) 2006 Rene Scharfe\n */\n-#define USE_THE_REPOSITORY_VARIABLE\n #include \"builtin.h\"\n #include \"archive.h\"\n #include \"gettext.h\"\n@@ -79,7 +78,7 @@ static int run_remote_archiver(int argc, const char **argv,\n int cmd_archive(int argc,\n \t\tconst char **argv,\n \t\tconst char *prefix,\n-\t\tstruct repository *repo UNUSED)\n+\t\tstruct repository *repo)\n {\n \tconst char *exec = \"git-upload-archive\";\n \tchar *output = NULL;\n@@ -110,7 +109,7 @@ int cmd_archive(int argc,\n \n \tsetvbuf(stderr, NULL, _IOLBF, BUFSIZ);\n \n-\tret = write_archive(argc, argv, prefix, the_repository, output, 0);\n+\tret = write_archive(argc, argv, prefix, repo, output, 0);\n \n out:\n \tfree(output);\n-- \ngitgitgadget\n"},{"id":"504855","messageId":"xmqq8quufs5c.fsf@gitster.g","threadId":"62185","inReplyTo":"pull.1788.v4.git.git.1728594828.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 0/3] Remove the_repository global for am, annotate, apply, archive builtins","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-11T17:47:43Z","receivedAt":"2024-10-11T17:47:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> Remove the_repository global variable for the annotate, apply, and archive\n> bulitins.\n>\n> Changes since V3:\n>\n> * Improve commit message in patch 2\n>\n> Changes since V1:\n>\n> * in patch 1, only pass in repo to the bulitin if the repo exists\n>\n> Changes since V2:\n>\n> * drop patch 3, which is a bit more involved to dis-entangle the_repository\n> * use a single variable in run_builtin() to keep track of whether or not we\n> are operating in a repository\n>\n> John Cai (3):\n> git: pass in repo to builtin based on setup_git_directory_gently\n> annotate: remove usage of the_repository global\n> archive: remove the_repository global variable\n\nWill queue. Thanks, all.\n\n\n"}]}