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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 20, 2025, 15:32 UTC
Message-ID: <xmqqa5ageio7.fsf@gitster.g>
In-Reply-To: <20250219203349.787173-2-usmanakinyemi202@gmail.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).

Show 5 quoted lines

> 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.