# [Outreachy][PATCH] builtin/update-server-info: remove the_repository global variable 6 messages from 2025-02-10 to 2025-02-11. Participants: Usman Akinyemi, Junio C Hamano, Patrick Steinhardt. Thread: https://gitlist.dev/t/62930 ## Usman Akinyemi, 2025-02-10 14:28 Subject: [Outreachy][PATCH] builtin/update-server-info: remove the_repository global variable Message-ID: <20250210142820.3588250-1-usmanakinyemi202@gmail.com> URL: https://gitlist.dev/e/20250210142820.3588250-1-usmanakinyemi202%40gmail.com ``` Remove the_repository global variable in favor of the repository argument that gets passed in "builtin/upload-server-info.c". The RUN_SETUP macro is used in "git.c" when the 'update-server-info' command is wired to the 'cmd_update_server_info()' function." This means we can be sure that the `run_builtin()` function inside "git.c" will always pass a valid `repo` variable to `cmd_update_server_info()` when the `update-server-info` command is run inside a Git repository. When the command is run outside a Git repository without the `-h` option, the command will fail (`die`) inside the `run_builtin()` function when the `setup_git_directory()` is called. So, the `cmd_update_server_info()` would not be called at all. When `-h` is passed to the command outside a Git repository, the `run_builtin()` will call the `cmd_update_server_info()` function with `repo` set as NULL. It is certain that the `update_server_info()` function would not be called when the `repo` config is `NULL` since this only happens when the `-h` option is used and the command would exit with code 129 before getting to the `update_server_info()` function inside the `usage_with_options()` function. To prevent accessing a `NULL` value `repo`, it is necessary to check if the `repo` has a valid value before calling the `repo_config` option inside "update-server-info.c" since it comes before the `usage_with_options()` function. So, this change is safe and would not lead to any breakage. Mentored-by: Christian Couder Signed-off-by: Usman Akinyemi --- builtin/update-server-info.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/update-server-info.c b/builtin/update-server-info.c index 47a3f0bdd9..d7467290a8 100644 --- a/builtin/update-server-info.c +++ b/builtin/update-server-info.c @@ -1,4 +1,3 @@ -#define USE_THE_REPOSITORY_VARIABLE #include "builtin.h" #include "config.h" #include "gettext.h" @@ -13,7 +12,7 @@ static const char * const update_server_info_usage[] = { int cmd_update_server_info(int argc, const char **argv, const char *prefix, - struct repository *repo UNUSED) + struct repository *repo) { int force = 0; struct option options[] = { @@ -21,11 +20,12 @@ int cmd_update_server_info(int argc, OPT_END() }; - git_config(git_default_config, NULL); + if (repo) + repo_config(repo, git_default_config, NULL); argc = parse_options(argc, argv, prefix, options, update_server_info_usage, 0); if (argc > 0) usage_with_options(update_server_info_usage, options); - return !!update_server_info(the_repository, force); + return !!update_server_info(repo, force); } -- 2.48.1 ``` ## Junio C Hamano, 2025-02-10 17:12 Subject: Re: [Outreachy][PATCH] builtin/update-server-info: remove the_repository global variable Message-ID: URL: https://gitlist.dev/e/xmqqikphbu6b.fsf%40gitster.g In-Reply-To: <20250210142820.3588250-1-usmanakinyemi202@gmail.com> ``` Usman Akinyemi writes: > Remove the_repository global variable in favor of the repository > argument that gets passed in "builtin/upload-server-info.c". update? upload? I somehow thought that dumb HTTP walker support was on the chopping list for Git 3.0 but apparently it isn't, so updating this remote corner of the system I thought nobody cared about is a good thing. I personally feel that from here ... > The RUN_SETUP macro is used in "git.c" when the 'update-server-info' > command is wired to the 'cmd_update_server_info()' function." > This means we can be sure that the `run_builtin()` function inside > "git.c" will always pass a valid `repo` variable to `cmd_update_server_info()` > when the `update-server-info` command is run inside a Git repository. > > When the command is run outside a Git repository without the `-h` > option, the command will fail (`die`) inside the `run_builtin()` function > when the `setup_git_directory()` is called. So, the `cmd_update_server_info()` > would not be called at all. ... to here are way too verbose and unnecessary. > When `-h` is passed to the command outside a > Git repository, the `run_builtin()` will call the `cmd_update_server_info()` > function with `repo` set as NULL. "set as NULL" -> "set to NULL"? ... and then early in the function, "parse_options()" call will give the options help and exit, without having to consult much of the configuration file. So it is safe to omit reading the config when `repo` argument the caller gave us is NULL. and that would be sufficient. All the rest of the proposed commit log message can also be removed, I think. Thanks. ``` ## Usman Akinyemi, 2025-02-10 18:03 Subject: Re: [Outreachy][PATCH] builtin/update-server-info: remove the_repository global variable Message-ID: URL: https://gitlist.dev/e/CAPSxiM8XOH9ueeYwhdQx6PKUWkRbzZh77ZxAmNjSjXrR0gd9_A%40mail.gmail.com In-Reply-To: ``` On Mon, Feb 10, 2025 at 10:42 PM Junio C Hamano wrote: > > Usman Akinyemi writes: > > > Remove the_repository global variable in favor of the repository > > argument that gets passed in "builtin/upload-server-info.c". > > update? upload? > > I somehow thought that dumb HTTP walker support was on the chopping > list for Git 3.0 but apparently it isn't, so updating this remote > corner of the system I thought nobody cared about is a good thing. Luckily, there are some other files that have similar structure. I will be sending patches soon for those also. > > I personally feel that from here ... > > > The RUN_SETUP macro is used in "git.c" when the 'update-server-info' > > command is wired to the 'cmd_update_server_info()' function." > > This means we can be sure that the `run_builtin()` function inside > > "git.c" will always pass a valid `repo` variable to `cmd_update_server_info()` > > when the `update-server-info` command is run inside a Git repository. > > > > When the command is run outside a Git repository without the `-h` > > option, the command will fail (`die`) inside the `run_builtin()` function > > when the `setup_git_directory()` is called. So, the `cmd_update_server_info()` > > would not be called at all. > > ... to here are way too verbose and unnecessary. > > > When `-h` is passed to the command outside a > > Git repository, the `run_builtin()` will call the `cmd_update_server_info()` > > function with `repo` set as NULL. > > "set as NULL" -> "set to NULL"? > > ... and then early in the function, "parse_options()" call will give > the options help and exit, without having to consult much of the > configuration file. So it is safe to omit reading the config > when `repo` argument the caller gave us is NULL. > > and that would be sufficient. All the rest of the proposed commit > log message can also be removed, I think. Yeah, thanks for the review. I will send the updated version. Thanks. > > Thanks. ``` ## Usman Akinyemi, 2025-02-10 18:10 Subject: [Outreachy][PATCH v2] builtin/update-server-info: remove the_repository global variable Message-ID: <20250210181103.3609495-1-usmanakinyemi202@gmail.com> URL: https://gitlist.dev/e/20250210181103.3609495-1-usmanakinyemi202%40gmail.com In-Reply-To: <20250210142820.3588250-1-usmanakinyemi202@gmail.com> ``` Remove the_repository global variable in favor of the repository argument that gets passed in "builtin/update-server-info.c". When `-h` is passed to the command outside a Git repository, the `run_builtin()` will call the `cmd_update_server_info()` function with `repo` set to NULL and then early in the function, "parse_options()" call will give the options help and exit, without having to consult much of the configuration file. So it is safe to omit reading the config when `repo` argument the caller gave us is NULL. Mentored-by: Christian Couder Signed-off-by: Usman Akinyemi --- builtin/update-server-info.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/update-server-info.c b/builtin/update-server-info.c index 47a3f0bdd9..d7467290a8 100644 --- a/builtin/update-server-info.c +++ b/builtin/update-server-info.c @@ -1,4 +1,3 @@ -#define USE_THE_REPOSITORY_VARIABLE #include "builtin.h" #include "config.h" #include "gettext.h" @@ -13,7 +12,7 @@ static const char * const update_server_info_usage[] = { int cmd_update_server_info(int argc, const char **argv, const char *prefix, - struct repository *repo UNUSED) + struct repository *repo) { int force = 0; struct option options[] = { @@ -21,11 +20,12 @@ int cmd_update_server_info(int argc, OPT_END() }; - git_config(git_default_config, NULL); + if (repo) + repo_config(repo, git_default_config, NULL); argc = parse_options(argc, argv, prefix, options, update_server_info_usage, 0); if (argc > 0) usage_with_options(update_server_info_usage, options); - return !!update_server_info(the_repository, force); + return !!update_server_info(repo, force); } -- 2.48.1 ``` ## Patrick Steinhardt, 2025-02-11 07:43 Subject: Re: [Outreachy][PATCH v2] builtin/update-server-info: remove the_repository global variable Message-ID: URL: https://gitlist.dev/e/Z6r_pqqP5vjJI-R5%40pks.im In-Reply-To: <20250210181103.3609495-1-usmanakinyemi202@gmail.com> ``` On Mon, Feb 10, 2025 at 11:40:30PM +0530, Usman Akinyemi wrote: > Remove the_repository global variable in favor of the repository > argument that gets passed in "builtin/update-server-info.c". > > When `-h` is passed to the command outside a Git repository, the > `run_builtin()` will call the `cmd_update_server_info()` function > with `repo` set to NULL and then early in the function, "parse_options()" > call will give the options help and exit, without having to consult much > of the configuration file. So it is safe to omit reading the config when > `repo` argument the caller gave us is NULL. Thanks, this version looks good to me. Patrick ``` ## Junio C Hamano, 2025-02-11 16:35 Subject: Re: [Outreachy][PATCH v2] builtin/update-server-info: remove the_repository global variable Message-ID: URL: https://gitlist.dev/e/xmqq4j1030cz.fsf%40gitster.g In-Reply-To: ``` Patrick Steinhardt writes: > On Mon, Feb 10, 2025 at 11:40:30PM +0530, Usman Akinyemi wrote: >> Remove the_repository global variable in favor of the repository >> argument that gets passed in "builtin/update-server-info.c". >> >> When `-h` is passed to the command outside a Git repository, the >> `run_builtin()` will call the `cmd_update_server_info()` function >> with `repo` set to NULL and then early in the function, "parse_options()" >> call will give the options help and exit, without having to consult much >> of the configuration file. So it is safe to omit reading the config when >> `repo` argument the caller gave us is NULL. > > Thanks, this version looks good to me. Yup, this looks good. Thanks, both. ```