Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>

[Taylor Blau <me@ttaylorr.com>]

On Mon, Oct 23, 2023 at 11:58:01AM +0200, Patrick Steinhardt wrote:
> On Wed, Oct 18, 2023 at 10:28:40PM +0200, Kristoffer Haugsbakk wrote:
> >
> > Factor out code that retrieves the global config file so that we can use
> > it in `gc.c` as well.
> >
> > Use the old name from the previous commit since this function acts
> > functionally the same as `git_system_config` but for “global”.
> >
> > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> > ---
> >  builtin/config.c | 25 ++-----------------------
> >  config.c         | 24 ++++++++++++++++++++++++
> >  config.h         |  1 +
> >  3 files changed, 27 insertions(+), 23 deletions(-)
> >
> > diff --git a/builtin/config.c b/builtin/config.c
> > index 6fff2655816..df06b766fad 100644
> > --- a/builtin/config.c
> > +++ b/builtin/config.c
> > @@ -708,30 +708,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> >  	}
> >
> >  	if (use_global_config) {
> > -		char *user_config, *xdg_config;
> > -
> > -		git_global_config_paths(&user_config, &xdg_config);
> > -		if (!user_config)
> > -			/*
> > -			 * It is unknown if HOME/.gitconfig exists, so
> > -			 * we do not know if we should write to XDG
> > -			 * location; error out even if XDG_CONFIG_HOME
> > -			 * is set and points at a sane location.
> > -			 */
> > -			die(_("$HOME not set"));
> > -
> > +		given_config_source.file = git_global_config();
> >  		given_config_source.scope = CONFIG_SCOPE_GLOBAL;
> > -
> > -		if (access_or_warn(user_config, R_OK, 0) &&
> > -		    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
> > -			given_config_source.file = xdg_config;
> > -			free(user_config);
> > -		} else {
> > -			given_config_source.file = user_config;
> > -			free(xdg_config);
> > -		}
> > -	}
> > -	else if (use_system_config) {
> > +	} else if (use_system_config) {
> >  		given_config_source.file = git_system_config();
> >  		given_config_source.scope = CONFIG_SCOPE_SYSTEM;
> >  	} else if (use_local_config) {
> > diff --git a/config.c b/config.c
> > index d2cdda96edd..2ff766c56ff 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -2111,6 +2111,30 @@ char *git_system_config(void)
> >  	return system_config;
> >  }
> >
> > +char *git_global_config(void)
> > +{
> > +	char *user_config, *xdg_config;
> > +
> > +	git_global_config_paths(&user_config, &xdg_config);
> > +	if (!user_config)
> > +		/*
> > +		 * It is unknown if HOME/.gitconfig exists, so
> > +		 * we do not know if we should write to XDG
>
> Nit: we don't know about the intent of the caller, so they may not want
> to write to the file but only read it.

I was going to suggest that we allow the caller to pass in the flags
that they wish for git_global_config() to pass down to access(2), but
was surprised to see that we always use R_OK.

But thinking on it for a moment longer, I realized that we don't care
about write-level permissions for the config, since we want to instead
open $GIT_DIR/config.lock for writing, and then rename() it into place,
meaning we only care about whether or not we have write permissions on
$GIT_DIR itself.

I think in the existing location of this code, the "if we should write"
portion of the comment is premature, since we don't know for sure
whether or not we are writing. So I'd be fine with leaving it as-is, but
changing the comment seems easy enough to do...

> > +		 * location; error out even if XDG_CONFIG_HOME
> > +		 * is set and points at a sane location.
> > +		 */
> > +		die(_("$HOME not set"));
>
> Is it sensible to `die()` here in this new function that behaves more
> like a library function? I imagine it would be more sensible to indicate
> the error to the user and let them handle it accordingly.

Agreed.

Thanks,
Taylor