| Message ID | 20250219203349.787173-2-usmanakinyemi202@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | stop using the_repository global variable. | expand |
Usman Akinyemi <usmanakinyemi202@gmail.com> writes: > Move `git_config()` call after `usage_with_options()` to avoid NULL `repo` > check. Two things and a half. - This move of a single call is not something I see usually called "refactor". - The new call to git_config() should be chosen a bit more carefully to be future-proof. It is harder to see with the reduced context, but verbose and format.format has already been referenced before the new place where you read the configuration file, which would mean that you are making it impossible for future developers to add configuration variables to give default values to these two settings. The same comment applies to potential new configuration variables that may control how parse_options() would work. With this line of conversion, we are closing the door for such configuration variables (e.g., OPTION_FILENAME may want to understand "~/path" and if we had user.homedirectory configuration variable, the value of the variable should affect what file the string "~/path" given to the option refers to). > When "-h" is passed to builtins using the RUN_SETUP macro, `repo` > passed by `run_builtin()` will be NULL. If we use the `repo` > instead of the global `the_repository` variable. We will have to > switch from `git_config()` to `repo_config()` which takes in > `repo`. I do not quite agree with this line of reasoning behind "will have to switch". You need to realize that this is cmd_verify_tag() that is designed to be ONLY called as the top-level command implementation and not called from random places as a library function. It is perfectly fine for it to work with the_repository with git_config(). The helper functions that this functions call may be different matter, though.
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index f6b97048a5..f0e7c2a2b5 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -35,8 +35,6 @@ int cmd_verify_tag(int argc, OPT_END() }; - git_config(git_default_config, NULL); - argc = parse_options(argc, argv, prefix, verify_tag_options, verify_tag_usage, PARSE_OPT_KEEP_ARGV0); if (argc <= i) @@ -52,6 +50,8 @@ int cmd_verify_tag(int argc, flags |= GPG_VERIFY_OMIT_STATUS; } + git_config(git_default_config, NULL); + while (i < argc) { struct object_id oid; const char *name = argv[i++];
Move `git_config()` call after `usage_with_options()` to avoid NULL `repo` check. When "-h" is passed to builtins using the RUN_SETUP macro, `repo` passed by `run_builtin()` will be NULL. If we use the `repo` instead of the global `the_repository` variable. We will have to switch from `git_config()` to `repo_config()` which takes in `repo`. We must check for NULL `repo` if `repo_config()` comes before `usage_with_options()`. Moving `git_config()` after `usage_with_options()` eliminates this need, as `usage_with_options()` exit before calling `repo_config()`. This will be useful in the following patch which remove `the_repository` global variable in favor of the `repo` passed by `run_builtin()`. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com> --- builtin/verify-tag.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)