On Mon, Sep 06 2021, Han-Wen Nienhuys via GitGitGadget wrote:
> In CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@xxxxxxxxxxxxxx,
Nit: use <message-id> for quoting, not message-id.
> we came to the conclusion that this feature is probably a remnant from
> the time that reflogs weren't enabled by default, and it does not need
> to be kept.
Maybe some summary of the flexibily either Jeff King or Junio mentioned
we were losing (i.e. we can't selectively enable per-ref now), but that
we think it's OK because...
For the implementation:
> + *logfd = -1;
Weird, more on this later...
> + if (!force_create && !should_autocreate_reflog(refname))
> + return 0;
OK, so we can early abort.
> files_reflog_path(refs, &logfile_sb, refname);
> logfile = strbuf_detach(&logfile_sb, NULL);
>
> - if (force_create || should_autocreate_reflog(refname)) {
> - if (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {
> - if (errno == ENOENT)
> - strbuf_addf(err, "unable to create directory for '%s': "
> - "%s", logfile, strerror(errno));
Here we use one indent/wrapping style...
> - else if (errno == EISDIR)
> - strbuf_addf(err, "there are still logs under '%s'",
> - logfile);
> - else
> - strbuf_addf(err, "unable to append to '%s': %s",
> - logfile, strerror(errno));
> -
> - goto error;
> - }
> - } else {
> - *logfd = open(logfile, O_APPEND | O_WRONLY, 0666);
> - if (*logfd < 0) {
> - if (errno == ENOENT || errno == EISDIR) {
> - /*
> - * The logfile doesn't already exist,
> - * but that is not an error; it only
> - * means that we won't write log
> - * entries to it.
> - */
> - ;
> - } else {
> - strbuf_addf(err, "unable to append to '%s': %s",
> - logfile, strerror(errno));
> - goto error;
> - }
> - }
> + if (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {
> + if (errno == ENOENT)
> + strbuf_addf(err,
> + "unable to create directory for '%s': "
> + "%s",
> + logfile, strerror(errno));
...but here it's changed while we're at it, this patch would be easier
to follow IMO if we just left the formatting alone (or did it as another
step). I'm aware that it ends us at over 79 columns, but that was the
case before...
> + else if (errno == EISDIR)
> + strbuf_addf(err, "there are still logs under '%s'",
> + logfile);
> + else
> + strbuf_addf(err, "unable to append to '%s': %s",
> + logfile, strerror(errno));
> }
>
> if (*logfd >= 0)
> adjust_shared_perm(logfile);
>
> free(logfile);
> - return 0;
> -
> -error:
> - free(logfile);
> - return -1;
> + return (*logfd < 0) ? -1 : 0;
On "more on this later": Since we just return -1, 0 or a valid fd now,
can't we just return the "fd" here and let the callers sort out -1, 0
and >0?
