| Message ID | 20250214230210.1460111-2-usmanakinyemi202@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | stop using the_repository global variable. | expand |
On Sat, Feb 15, 2025 at 04:27:17AM +0530, Usman Akinyemi wrote: > @@ -35,7 +34,8 @@ int cmd_verify_tag(int argc, > OPT_END() > }; > > - git_config(git_default_config, NULL); > + if (repo) > + repo_config(repo, git_default_config, NULL); > I recently noticed that we have `usage_with_options_if_asked()`. Should we use that function rather than making the call to `git_config()` conditional? Otherwise it's not obvious why we have the conditional in the first place. The same comment also applies to subsequent commits. Patrick
On Mon, Feb 17, 2025 at 12:25 PM Patrick Steinhardt <ps@pks.im> wrote: > > On Sat, Feb 15, 2025 at 04:27:17AM +0530, Usman Akinyemi wrote: > > @@ -35,7 +34,8 @@ int cmd_verify_tag(int argc, > > OPT_END() > > }; > > > > - git_config(git_default_config, NULL); > > + if (repo) > > + repo_config(repo, git_default_config, NULL); > > > > I recently noticed that we have `usage_with_options_if_asked()`. Should > we use that function rather than making the call to `git_config()` > conditional? Otherwise it's not obvious why we have the conditional in > the first place. Hi Patrick, I think the function is `show_usage_with_options_if_asked()`. The function is quite different from `git_config()` or the `repo_config()`. The config function consults the configuration file for setting up config values and it uses the `repo` variable during this. While `show_usage_with_options_if_asked()` is used when the "-h" option is passed to the builtin functions to display the help string. In a case when "-h" is passed to the builtin functions which use the RUN_SETUP macro, the `repo` config will be NULL. There are some builtin commands functions that which has the`git_config()` function comes before `show_usage_with_options_if_asked()` or it's variant and some, `git_config()` comes after. For those that have `git_config()` comes after `show_usage_with_options_if_asked()` , no need for the check, since the `show_usage_with_options_if_asked()`call will exit without reaching `git_config()`. For scenario where the `git_config()` comes earlier, we have to check the `repo` to see if it is NULL, if it is NULL, we are sure this happens when the "-h" is passed to the function and we do not need to setup and configuration since `show_usage_with_options_if_asked()` will exit. So, the condition is necessary else, NULL value will be passed to the `git_config()` which will lead to accessing NULL value. Thank you. > > The same comment also applies to subsequent commits. > > Patrick
On Mon, Feb 17, 2025 at 03:35:05PM +0530, Usman Akinyemi wrote: > On Mon, Feb 17, 2025 at 12:25 PM Patrick Steinhardt <ps@pks.im> wrote: > > > > On Sat, Feb 15, 2025 at 04:27:17AM +0530, Usman Akinyemi wrote: > > > @@ -35,7 +34,8 @@ int cmd_verify_tag(int argc, > > > OPT_END() > > > }; > > > > > > - git_config(git_default_config, NULL); > > > + if (repo) > > > + repo_config(repo, git_default_config, NULL); > > > > > > > I recently noticed that we have `usage_with_options_if_asked()`. Should > > we use that function rather than making the call to `git_config()` > > conditional? Otherwise it's not obvious why we have the conditional in > > the first place. > Hi Patrick, > > I think the function is `show_usage_with_options_if_asked()`. The function > is quite different from `git_config()` or the `repo_config()`. The > config function consults the configuration file for setting up config > values and it uses the `repo` variable during this. While > `show_usage_with_options_if_asked()` is used when the "-h" option is > passed to the builtin functions to display the help string. > > In a case when "-h" is passed to the builtin functions which use the > RUN_SETUP macro, the `repo` config will be NULL. > > There are some builtin commands functions that which has > the`git_config()` function comes before > `show_usage_with_options_if_asked()` or it's variant and some, > `git_config()` comes after. > > For those that have `git_config()` comes after > `show_usage_with_options_if_asked()` , no need for the check, since > the `show_usage_with_options_if_asked()`call will exit without > reaching `git_config()`. For scenario where the `git_config()` comes > earlier, we have to check the `repo` to see if it is NULL, if it is > NULL, we are sure this happens when the "-h" is passed to the function > and we do not need to setup and configuration since > `show_usage_with_options_if_asked()` will exit. Exactly, this is what my suggestion is. If we introduced new calls to `show_usage_with_options_if_asked()` before `git_config()` we wouldn't have to check for a `NULL` repository in the first place because we know that we'd have already exited if there was a "-h" parameter. Patrick
On Mon, Feb 17, 2025 at 3:52 PM Patrick Steinhardt <ps@pks.im> wrote: > > On Mon, Feb 17, 2025 at 03:35:05PM +0530, Usman Akinyemi wrote: > > On Mon, Feb 17, 2025 at 12:25 PM Patrick Steinhardt <ps@pks.im> wrote: > > > > > > On Sat, Feb 15, 2025 at 04:27:17AM +0530, Usman Akinyemi wrote: > > > > @@ -35,7 +34,8 @@ int cmd_verify_tag(int argc, > > > > OPT_END() > > > > }; > > > > > > > > - git_config(git_default_config, NULL); > > > > + if (repo) > > > > + repo_config(repo, git_default_config, NULL); > > > > > > > > > > I recently noticed that we have `usage_with_options_if_asked()`. Should > > > we use that function rather than making the call to `git_config()` > > > conditional? Otherwise it's not obvious why we have the conditional in > > > the first place. > > Hi Patrick, > > > > I think the function is `show_usage_with_options_if_asked()`. The function > > is quite different from `git_config()` or the `repo_config()`. The > > config function consults the configuration file for setting up config > > values and it uses the `repo` variable during this. While > > `show_usage_with_options_if_asked()` is used when the "-h" option is > > passed to the builtin functions to display the help string. > > > > In a case when "-h" is passed to the builtin functions which use the > > RUN_SETUP macro, the `repo` config will be NULL. > > > > There are some builtin commands functions that which has > > the`git_config()` function comes before > > `show_usage_with_options_if_asked()` or it's variant and some, > > `git_config()` comes after. > > > > For those that have `git_config()` comes after > > `show_usage_with_options_if_asked()` , no need for the check, since > > the `show_usage_with_options_if_asked()`call will exit without > > reaching `git_config()`. For scenario where the `git_config()` comes > > earlier, we have to check the `repo` to see if it is NULL, if it is > > NULL, we are sure this happens when the "-h" is passed to the function > > and we do not need to setup and configuration since > > `show_usage_with_options_if_asked()` will exit. > > Exactly, this is what my suggestion is. If we introduced new calls to > `show_usage_with_options_if_asked()` before `git_config()` we wouldn't > have to check for a `NULL` repository in the first place because we know > that we'd have already exited if there was a "-h" parameter. Yeah, that is true. Maybe having this as a preparatory patch could be better. There was a previous similar patch also which has been accepted. Maybe this can be done after this patch series got accepted, so, I could do it together with the already accepted patch. What do you think ? Thank you. > > Patrick
On Mon, Feb 17, 2025 at 04:12:06PM +0530, Usman Akinyemi wrote: > On Mon, Feb 17, 2025 at 3:52 PM Patrick Steinhardt <ps@pks.im> wrote: > > > > On Mon, Feb 17, 2025 at 03:35:05PM +0530, Usman Akinyemi wrote: > > > On Mon, Feb 17, 2025 at 12:25 PM Patrick Steinhardt <ps@pks.im> wrote: > > > > > > > > On Sat, Feb 15, 2025 at 04:27:17AM +0530, Usman Akinyemi wrote: > > > > > @@ -35,7 +34,8 @@ int cmd_verify_tag(int argc, > > > > > OPT_END() > > > > > }; > > > > > > > > > > - git_config(git_default_config, NULL); > > > > > + if (repo) > > > > > + repo_config(repo, git_default_config, NULL); > > > > > > > > > > > > > I recently noticed that we have `usage_with_options_if_asked()`. Should > > > > we use that function rather than making the call to `git_config()` > > > > conditional? Otherwise it's not obvious why we have the conditional in > > > > the first place. > > > Hi Patrick, > > > > > > I think the function is `show_usage_with_options_if_asked()`. The function > > > is quite different from `git_config()` or the `repo_config()`. The > > > config function consults the configuration file for setting up config > > > values and it uses the `repo` variable during this. While > > > `show_usage_with_options_if_asked()` is used when the "-h" option is > > > passed to the builtin functions to display the help string. > > > > > > In a case when "-h" is passed to the builtin functions which use the > > > RUN_SETUP macro, the `repo` config will be NULL. > > > > > > There are some builtin commands functions that which has > > > the`git_config()` function comes before > > > `show_usage_with_options_if_asked()` or it's variant and some, > > > `git_config()` comes after. > > > > > > For those that have `git_config()` comes after > > > `show_usage_with_options_if_asked()` , no need for the check, since > > > the `show_usage_with_options_if_asked()`call will exit without > > > reaching `git_config()`. For scenario where the `git_config()` comes > > > earlier, we have to check the `repo` to see if it is NULL, if it is > > > NULL, we are sure this happens when the "-h" is passed to the function > > > and we do not need to setup and configuration since > > > `show_usage_with_options_if_asked()` will exit. > > > > Exactly, this is what my suggestion is. If we introduced new calls to > > `show_usage_with_options_if_asked()` before `git_config()` we wouldn't > > have to check for a `NULL` repository in the first place because we know > > that we'd have already exited if there was a "-h" parameter. > Yeah, that is true. Maybe having this as a preparatory patch could be better. > > There was a previous similar patch also which has been accepted. Maybe > this can be done after this patch series got accepted, so, I could do > it together > with the already accepted patch. Yup, that'd be great indeed. Thanks! Patrick
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index f6b97048a5..990e967af3 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -5,7 +5,6 @@ * * Based on git-verify-tag.sh */ -#define USE_THE_REPOSITORY_VARIABLE #include "builtin.h" #include "config.h" #include "gettext.h" @@ -23,7 +22,7 @@ static const char * const verify_tag_usage[] = { int cmd_verify_tag(int argc, const char **argv, const char *prefix, - struct repository *repo UNUSED) + struct repository *repo) { int i = 1, verbose = 0, had_error = 0; unsigned flags = 0; @@ -35,7 +34,8 @@ int cmd_verify_tag(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, verify_tag_options, verify_tag_usage, PARSE_OPT_KEEP_ARGV0); @@ -56,7 +56,7 @@ int cmd_verify_tag(int argc, struct object_id oid; const char *name = argv[i++]; - if (repo_get_oid(the_repository, name, &oid)) { + if (repo_get_oid(repo, name, &oid)) { had_error = !!error("tag '%s' not found.", name); continue; }
Remove the_repository global variable in favor of the repository argument that gets passed in "builtin/verify-tag.c". When `-h` is passed to the command outside a Git repository, the `run_builtin()` will call the `cmd_verify_tag()` 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/verify-tag.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)