* [Outreachy][PATCH] builtin/update-server-info: remove the_repository global variable
@ 2025-02-10 14:28 Usman Akinyemi
2025-02-10 17:12 ` Junio C Hamano
2025-02-10 18:10 ` [Outreachy][PATCH v2] " Usman Akinyemi
0 siblings, 2 replies; 6+ messages in thread
From: Usman Akinyemi @ 2025-02-10 14:28 UTC (permalink / raw)
To: git, christian.couder; +Cc: ps, shejialuo, johncai86, Christian Couder
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 <chriscool@tuxfamily•org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail•com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Outreachy][PATCH] builtin/update-server-info: remove the_repository global variable
2025-02-10 14:28 [Outreachy][PATCH] builtin/update-server-info: remove the_repository global variable Usman Akinyemi
@ 2025-02-10 17:12 ` Junio C Hamano
2025-02-10 18:03 ` Usman Akinyemi
2025-02-10 18:10 ` [Outreachy][PATCH v2] " Usman Akinyemi
1 sibling, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2025-02-10 17:12 UTC (permalink / raw)
To: Usman Akinyemi
Cc: git, christian.couder, ps, shejialuo, johncai86, Christian Couder
Usman Akinyemi <usmanakinyemi202@gmail•com> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy][PATCH] builtin/update-server-info: remove the_repository global variable
2025-02-10 17:12 ` Junio C Hamano
@ 2025-02-10 18:03 ` Usman Akinyemi
0 siblings, 0 replies; 6+ messages in thread
From: Usman Akinyemi @ 2025-02-10 18:03 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, christian.couder, ps, shejialuo, johncai86, Christian Couder
On Mon, Feb 10, 2025 at 10:42 PM Junio C Hamano <gitster@pobox•com> wrote:
>
> Usman Akinyemi <usmanakinyemi202@gmail•com> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Outreachy][PATCH v2] builtin/update-server-info: remove the_repository global variable
2025-02-10 14:28 [Outreachy][PATCH] builtin/update-server-info: remove the_repository global variable Usman Akinyemi
2025-02-10 17:12 ` Junio C Hamano
@ 2025-02-10 18:10 ` Usman Akinyemi
2025-02-11 7:43 ` Patrick Steinhardt
1 sibling, 1 reply; 6+ messages in thread
From: Usman Akinyemi @ 2025-02-10 18:10 UTC (permalink / raw)
To: git, christian.couder; +Cc: ps, shejialuo, johncai86, Christian Couder
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 <chriscool@tuxfamily•org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail•com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Outreachy][PATCH v2] builtin/update-server-info: remove the_repository global variable
2025-02-10 18:10 ` [Outreachy][PATCH v2] " Usman Akinyemi
@ 2025-02-11 7:43 ` Patrick Steinhardt
2025-02-11 16:35 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Patrick Steinhardt @ 2025-02-11 7:43 UTC (permalink / raw)
To: Usman Akinyemi
Cc: git, christian.couder, shejialuo, johncai86, Christian Couder
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy][PATCH v2] builtin/update-server-info: remove the_repository global variable
2025-02-11 7:43 ` Patrick Steinhardt
@ 2025-02-11 16:35 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2025-02-11 16:35 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Usman Akinyemi, git, christian.couder, shejialuo, johncai86,
Christian Couder
Patrick Steinhardt <ps@pks•im> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-02-11 16:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-10 14:28 [Outreachy][PATCH] builtin/update-server-info: remove the_repository global variable Usman Akinyemi
2025-02-10 17:12 ` Junio C Hamano
2025-02-10 18:03 ` Usman Akinyemi
2025-02-10 18:10 ` [Outreachy][PATCH v2] " Usman Akinyemi
2025-02-11 7:43 ` Patrick Steinhardt
2025-02-11 16:35 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox