On Wed, Apr 16, 2025 at 11:44:50AM +0530, K Jayatheerth wrote:
>
> 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.
>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@xxxxxxxxx>
> ---
> 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
>
> ----
>
> -`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
>
>
