Re: [PATCH 4/4] cmd_psuh: Prefer repo_config for config lookup

From: Emily Shaffer <nasamuffin@google.com>
Date: May 16, 2025, 18:26 UTC
Message-ID: <aCeDZgaNWPbDV0Ra@google.com>
In-Reply-To: <20250416061450.25695-5-jayatheerthkulkarni2005@gmail.com>

On Wed, Apr 16, 2025 at 11:44:50AM +0530, K Jayatheerth wrote:

Show 19 quoted lines

> 
> This commit updates cmd_psuh to use repo_config and
> repo_config_get_string_tmp for retrieving the user.name config
> variable. This is a more robust and correct approach than using the
> global git_config functions because:
> 
> git_config uses the global configuration, ignoring any
> repository-specific settings (e.g., in .git/config). repo_config
> loads the configuration specific to the repository,
> ensuring that the correct settings are used.
> 
> repo_config_get_string_tmp retrieves configuration values
> relative to the repository, respecting any local overrides.
> 
> This change ensures that cmd_psuh correctly reads the
> user.name setting that applies to the current repository,
> rather than relying on globalsettings that might be
> incorrect or misleading. It also demonstrates the correct way
> to access repository-specific configuration within Git commands.

This commit message is a really good start! I like that you're pointing out there's a real bug in using global config and ignoring repo-local config. Although I'm not sure that it's actually accurate... but it is true that git_config() is now an outdated way to access config, since it assumes the_repository instead of passing the `repo` variable that was provided to cmd_psuh.

Show 58 quoted lines

> 
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---
>  Documentation/MyFirstContribution.adoc | 52 ++++++++++++++++++--------
>  1 file changed, 37 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/MyFirstContribution.adoc b/Documentation/MyFirstContribution.adoc
> index ed6dcc1fc6..688240ce8b 100644
> --- a/Documentation/MyFirstContribution.adoc
> +++ b/Documentation/MyFirstContribution.adoc
> @@ -325,26 +325,48 @@ on the command line, including the name of our command. (If `prefix` is empty
>  for you, try `cd Documentation/ && ../bin-wrappers/git psuh`). That's not so
>  helpful. So what other context can we get?
>  
> -Add a line to `#include "config.h"`. Then, add the following bits to the
> -function body:
> +Add a line to `#include "config.h"` and `#include "repository.h"`. 
> +Then, add the following bits to the function body:
>  
>  ----
> -	const char *cfg_name;
> +#include "builtin.h"
> +#include "gettext.h"
> +#include "config.h"
> +#include "repository.h"  
>  
> -...
> +int cmd_psuh(int argc, const char **argv, 
> +			const char *prefix, struct repository *repo)
> +{
> +    const char *cfg_name;
> +
> +    printf(Q_("Your args (there is %d):\n",
> +              "Your args (there are %d):\n",
> +              argc),
> +           argc);
>  
> -	git_config(git_default_config, NULL);
> -	if (git_config_get_string_tmp("user.name", &cfg_name) > 0)
> -		printf(_("No name is found in config\n"));
> -	else
> -		printf(_("Your name: %s\n"), cfg_name);
> +    for (int i = 0; i < argc; i++) {
> +        printf("%d: %s\n", i, argv[i]);
> +    }
> +
> +    printf(_("Your current working directory:\n<top-level>%s%s\n"),
> +           prefix ? "/" : "", prefix ? prefix : "");
> +
> +    repo_config(repo, git_default_config, NULL);
> +
> +    if (repo_config_get_string_tmp(repo, "user.name", &cfg_name))
> +        printf(_("No name is found in config\n"));
> +    else
> +        printf(_("Your name: %s\n"), cfg_name);
> +
> +    return 0;
> +}

I'd prefer to see this stick to the prior formula of including only small chunks of the function, rather than a full function you can copy and paste. Because this is a tutorial, and the goal is for learners to understand each section of code as they add it, not just for them to paste it into their editor and hit run.

So, I don't think it's necessary for you to add the rest of the function here in the process of switching to repo_config from git_config.

Generally, I find the changes to update the code snippets unobjectionable and don't have a problem with the added prose beyond a couple nits. But as I assume you sent this series as a way to learn more about the codebase, definitely please revisit your commit messages to align their style with the rest of the codebase.

But I think with the stuff I called out taken into account in v2, this series is good. Thanks for the effort to update it. I'd also like to update github.com/nasamuffin/git/tree/psuh once this series lands, if you can point me to a branch of yours with the sample code I can pull from :)

(Or, as we discussed when I sent this doc in the first place, does it make sense for a branch with the sample code to be maintained only-best-effort on git/git itself?)

 - Emily

Show 38 quoted lines

>
>  ----
>  
> -`git_config()` will grab the configuration from config files known to Git and
> -apply standard precedence rules. `git_config_get_string_tmp()` will look up
> +`repo_config()` will grab the configuration from config files known to Git and
> +apply standard precedence rules. `repo_config_get_string_tmp()` will look up
>  a specific key ("user.name") and give you the value. There are a number of
>  single-key lookup functions like this one; you can see them all (and more info
> -about how to use `git_config()`) in `Documentation/technical/api-config.adoc`.
> +about how to use `repo_config()`) in `Documentation/technical/api-config.adoc`.
>  
>  You should see that the name printed matches the one you see when you run:
>  
> @@ -377,7 +399,7 @@ status_init_config(&s, git_status_config);
>  ----
>  
>  But as we drill down, we can find that `status_init_config()` wraps a call
> -to `git_config()`. Let's modify the code we wrote in the previous commit.
> +to `repo_config()`. Let's modify the code we wrote in the previous commit.
>  
>  Be sure to include the header to allow you to use `struct wt_status`:
>  
> @@ -393,8 +415,8 @@ prepare it, and print its contents:
>  
>  ...
>  
> -	wt_status_prepare(the_repository, &status);
> -	git_config(git_default_config, &status);
> +	wt_status_prepare(repo, &status);
> +	repo_config(repo, git_default_config, &status);
>  
>  ...
>  
> -- 
> 2.49.GIT
> 
>