Re: [PATCH v2 01/12] builtin/verify-tag: refactor `cmd_verify_tag()`

[Junio C Hamano <gitster@pobox.com>]

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.