{"thread":{"id":"62930","subject":"[Outreachy][PATCH] builtin/update-server-info: remove the_repository global variable","startedAt":"2025-02-10T14:28:27Z","lastAt":"2025-02-11T16:35:59Z","messageCount":6,"participants":["Usman Akinyemi","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"512176","messageId":"20250210142820.3588250-1-usmanakinyemi202@gmail.com","threadId":"62930","inReplyTo":null,"subject":"[Outreachy][PATCH] builtin/update-server-info: remove the_repository global variable","fromName":"Usman Akinyemi","fromEmail":"usmanakinyemi202@gmail.com","sentAt":"2025-02-10T14:28:10Z","receivedAt":"2025-02-10T14:28:27Z","isPatch":true,"sender":{"key":"usmanakinyemi202@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86585626?v=4"},"body":"Remove the_repository global variable in favor of the repository\nargument that gets passed in \"builtin/upload-server-info.c\".\n\nThe RUN_SETUP macro is used in \"git.c\" when the 'update-server-info'\ncommand is wired to the 'cmd_update_server_info()' function.\"\nThis means we can be sure that the `run_builtin()` function inside\n\"git.c\" will always pass a valid `repo` variable to `cmd_update_server_info()`\nwhen the `update-server-info` command is run inside a Git repository.\n\nWhen the command is run outside a Git repository without the `-h`\noption, the command will fail (`die`) inside the `run_builtin()` function\nwhen the `setup_git_directory()` is called. So, the `cmd_update_server_info()`\nwould not be called at all. When `-h` is passed to the command outside a\nGit repository, the `run_builtin()` will call the `cmd_update_server_info()`\nfunction with `repo` set as NULL.\n\nIt is certain that the `update_server_info()` function would not be\ncalled when the `repo` config is `NULL` since this only happens when the\n`-h` option is used and the command would exit with code 129 before\ngetting to the `update_server_info()` function inside the\n`usage_with_options()` function.\n\nTo prevent accessing a `NULL` value `repo`, it is necessary to check if\nthe `repo` has a valid value before calling the `repo_config` option\ninside \"update-server-info.c\" since it comes before the\n`usage_with_options()` function.\n\nSo, this change is safe and would not lead to any breakage.\n\nMentored-by: Christian Couder \nSigned-off-by: Usman Akinyemi \n---\n builtin/update-server-info.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/update-server-info.c b/builtin/update-server-info.c\nindex 47a3f0bdd9..d7467290a8 100644\n--- a/builtin/update-server-info.c\n+++ b/builtin/update-server-info.c\n@@ -1,4 +1,3 @@\n-#define USE_THE_REPOSITORY_VARIABLE\n #include \"builtin.h\"\n #include \"config.h\"\n #include \"gettext.h\"\n@@ -13,7 +12,7 @@ static const char * const update_server_info_usage[] = {\n int cmd_update_server_info(int argc,\n \t\t\t const char **argv,\n \t\t\t const char *prefix,\n-\t\t\t struct repository *repo UNUSED)\n+\t\t\t struct repository *repo)\n {\n \tint force = 0;\n \tstruct option options[] = {\n@@ -21,11 +20,12 @@ int cmd_update_server_info(int argc,\n \t\tOPT_END()\n \t};\n \n-\tgit_config(git_default_config, NULL);\n+\tif (repo)\n+\t\trepo_config(repo, git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t update_server_info_usage, 0);\n \tif (argc > 0)\n \t\tusage_with_options(update_server_info_usage, options);\n \n-\treturn !!update_server_info(the_repository, force);\n+\treturn !!update_server_info(repo, force);\n }\n-- \n2.48.1\n\n"},{"id":"512186","messageId":"xmqqikphbu6b.fsf@gitster.g","threadId":"62930","inReplyTo":"20250210142820.3588250-1-usmanakinyemi202@gmail.com","subject":"Re: [Outreachy][PATCH] builtin/update-server-info: remove the_repository global variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-10T17:12:28Z","receivedAt":"2025-02-10T17:12:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Usman Akinyemi writes:\n\n> Remove the_repository global variable in favor of the repository\n> argument that gets passed in \"builtin/upload-server-info.c\".\n\nupdate? upload?\n\nI somehow thought that dumb HTTP walker support was on the chopping\nlist for Git 3.0 but apparently it isn't, so updating this remote\ncorner of the system I thought nobody cared about is a good thing.\n\nI personally feel that from here ...\n\n> The RUN_SETUP macro is used in \"git.c\" when the 'update-server-info'\n> command is wired to the 'cmd_update_server_info()' function.\"\n> This means we can be sure that the `run_builtin()` function inside\n> \"git.c\" will always pass a valid `repo` variable to `cmd_update_server_info()`\n> when the `update-server-info` command is run inside a Git repository.\n>\n> When the command is run outside a Git repository without the `-h`\n> option, the command will fail (`die`) inside the `run_builtin()` function\n> when the `setup_git_directory()` is called. So, the `cmd_update_server_info()`\n> would not be called at all.\n\n... to here are way too verbose and unnecessary.\n\n> When `-h` is passed to the command outside a\n> Git repository, the `run_builtin()` will call the `cmd_update_server_info()`\n> function with `repo` set as NULL.\n\n\"set as NULL\" -> \"set to NULL\"?\n\n ... and then early in the function, \"parse_options()\" call will give\n the options help and exit, without having to consult much of the\n configuration file. So it is safe to omit reading the config\n when `repo` argument the caller gave us is NULL.\n\nand that would be sufficient. All the rest of the proposed commit\nlog message can also be removed, I think.\n\nThanks.\n"},{"id":"512188","messageId":"CAPSxiM8XOH9ueeYwhdQx6PKUWkRbzZh77ZxAmNjSjXrR0gd9_A@mail.gmail.com","threadId":"62930","inReplyTo":"xmqqikphbu6b.fsf@gitster.g","subject":"Re: [Outreachy][PATCH] builtin/update-server-info: remove the_repository global variable","fromName":"Usman Akinyemi","fromEmail":"usmanakinyemi202@gmail.com","sentAt":"2025-02-10T18:03:30Z","receivedAt":"2025-02-10T18:03:41Z","isPatch":true,"sender":{"key":"usmanakinyemi202@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86585626?v=4"},"body":"On Mon, Feb 10, 2025 at 10:42 PM Junio C Hamano wrote:\n>\n> Usman Akinyemi writes:\n>\n> > Remove the_repository global variable in favor of the repository\n> > argument that gets passed in \"builtin/upload-server-info.c\".\n>\n> update? upload?\n>\n> I somehow thought that dumb HTTP walker support was on the chopping\n> list for Git 3.0 but apparently it isn't, so updating this remote\n> corner of the system I thought nobody cared about is a good thing.\nLuckily, there are some other files that have similar structure. I\nwill be sending\npatches soon for those also.\n>\n> I personally feel that from here ...\n>\n> > The RUN_SETUP macro is used in \"git.c\" when the 'update-server-info'\n> > command is wired to the 'cmd_update_server_info()' function.\"\n> > This means we can be sure that the `run_builtin()` function inside\n> > \"git.c\" will always pass a valid `repo` variable to `cmd_update_server_info()`\n> > when the `update-server-info` command is run inside a Git repository.\n> >\n> > When the command is run outside a Git repository without the `-h`\n> > option, the command will fail (`die`) inside the `run_builtin()` function\n> > when the `setup_git_directory()` is called. So, the `cmd_update_server_info()`\n> > would not be called at all.\n>\n> ... to here are way too verbose and unnecessary.\n>\n> > When `-h` is passed to the command outside a\n> > Git repository, the `run_builtin()` will call the `cmd_update_server_info()`\n> > function with `repo` set as NULL.\n>\n> \"set as NULL\" -> \"set to NULL\"?\n>\n> ... and then early in the function, \"parse_options()\" call will give\n> the options help and exit, without having to consult much of the\n> configuration file. So it is safe to omit reading the config\n> when `repo` argument the caller gave us is NULL.\n>\n> and that would be sufficient. All the rest of the proposed commit\n> log message can also be removed, I think.\nYeah, thanks for the review. I will send the updated version.\nThanks.\n>\n> Thanks.\n"},{"id":"512189","messageId":"20250210181103.3609495-1-usmanakinyemi202@gmail.com","threadId":"62930","inReplyTo":"20250210142820.3588250-1-usmanakinyemi202@gmail.com","subject":"[Outreachy][PATCH v2] builtin/update-server-info: remove the_repository global variable","fromName":"Usman Akinyemi","fromEmail":"usmanakinyemi202@gmail.com","sentAt":"2025-02-10T18:10:30Z","receivedAt":"2025-02-10T18:11:09Z","isPatch":true,"sender":{"key":"usmanakinyemi202@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86585626?v=4"},"body":"Remove the_repository global variable in favor of the repository\nargument that gets passed in \"builtin/update-server-info.c\".\n\nWhen `-h` is passed to the command outside a Git repository, the\n`run_builtin()` will call the `cmd_update_server_info()` function\nwith `repo` set to NULL and then early in the function, \"parse_options()\"\ncall will give the options help and exit, without having to consult much\nof the configuration file. So it is safe to omit reading the config when\n`repo` argument the caller gave us is NULL.\n\nMentored-by: Christian Couder \nSigned-off-by: Usman Akinyemi \n---\n builtin/update-server-info.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/update-server-info.c b/builtin/update-server-info.c\nindex 47a3f0bdd9..d7467290a8 100644\n--- a/builtin/update-server-info.c\n+++ b/builtin/update-server-info.c\n@@ -1,4 +1,3 @@\n-#define USE_THE_REPOSITORY_VARIABLE\n #include \"builtin.h\"\n #include \"config.h\"\n #include \"gettext.h\"\n@@ -13,7 +12,7 @@ static const char * const update_server_info_usage[] = {\n int cmd_update_server_info(int argc,\n \t\t\t const char **argv,\n \t\t\t const char *prefix,\n-\t\t\t struct repository *repo UNUSED)\n+\t\t\t struct repository *repo)\n {\n \tint force = 0;\n \tstruct option options[] = {\n@@ -21,11 +20,12 @@ int cmd_update_server_info(int argc,\n \t\tOPT_END()\n \t};\n \n-\tgit_config(git_default_config, NULL);\n+\tif (repo)\n+\t\trepo_config(repo, git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t update_server_info_usage, 0);\n \tif (argc > 0)\n \t\tusage_with_options(update_server_info_usage, options);\n \n-\treturn !!update_server_info(the_repository, force);\n+\treturn !!update_server_info(repo, force);\n }\n-- \n2.48.1\n\n"},{"id":"512212","messageId":"Z6r_pqqP5vjJI-R5@pks.im","threadId":"62930","inReplyTo":"20250210181103.3609495-1-usmanakinyemi202@gmail.com","subject":"Re: [Outreachy][PATCH v2] builtin/update-server-info: remove the_repository global variable","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-11T07:43:34Z","receivedAt":"2025-02-11T07:43:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 10, 2025 at 11:40:30PM +0530, Usman Akinyemi wrote:\n> Remove the_repository global variable in favor of the repository\n> argument that gets passed in \"builtin/update-server-info.c\".\n> \n> When `-h` is passed to the command outside a Git repository, the\n> `run_builtin()` will call the `cmd_update_server_info()` function\n> with `repo` set to NULL and then early in the function, \"parse_options()\"\n> call will give the options help and exit, without having to consult much\n> of the configuration file. So it is safe to omit reading the config when\n> `repo` argument the caller gave us is NULL.\n\nThanks, this version looks good to me.\n\nPatrick\n"},{"id":"512237","messageId":"xmqq4j1030cz.fsf@gitster.g","threadId":"62930","inReplyTo":"Z6r_pqqP5vjJI-R5@pks.im","subject":"Re: [Outreachy][PATCH v2] builtin/update-server-info: remove the_repository global variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-11T16:35:56Z","receivedAt":"2025-02-11T16:35:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Mon, Feb 10, 2025 at 11:40:30PM +0530, Usman Akinyemi wrote:\n>> Remove the_repository global variable in favor of the repository\n>> argument that gets passed in \"builtin/update-server-info.c\".\n>> \n>> When `-h` is passed to the command outside a Git repository, the\n>> `run_builtin()` will call the `cmd_update_server_info()` function\n>> with `repo` set to NULL and then early in the function, \"parse_options()\"\n>> call will give the options help and exit, without having to consult much\n>> of the configuration file. So it is safe to omit reading the config when\n>> `repo` argument the caller gave us is NULL.\n>\n> Thanks, this version looks good to me.\n\nYup, this looks good.\n\nThanks, both.\n"}]}