On Tue, Oct 24, 2023 at 03:23:11PM +0200, Kristoffer Haugsbakk wrote:
> 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
Attachment:
signature.asc
Description: PGP signature
