Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 25, 2023, 05:38 UTC
- Message-ID
- <ZTip7JWm-WRWTImU@tanuki>
- In-Reply-To
- <87badbe0-de18-4f8a-9589-314cea46065e@app.fastmail.com>
On Tue, Oct 24, 2023 at 03:23:11PM +0200, Kristoffer Haugsbakk wrote:
Show 36 quoted lines
> Hi Taylor and Patrick
>
> On Mon, Oct 23, 2023, at 19:40, Taylor Blau wrote:
> >> 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
>
> What do you guys think the signature of `git_global_config` should be?Either of the following:
- `int git_global_config(char **out_pat)`
- `char **git_global_config(void)`In the first case you'd signal error via a non-zero return value, whereas in the second case you would signal it via a `NULL` return value.
To decide which one to go with I'd recommend to check whether there is any similar precedent in "config.h" and what style that precedent uses.
Patrick