Re: [PATCH v2 5/5] RFC: refs: reflog entries aren't written based on reflog existence.

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 6, 2021, 22:50 UTC
Message-ID: <87v93dmh2n.fsf@evledraar.gmail.com>
In-Reply-To: <f6a7c5ad56efceef9c11226beb854b806ef54687.1630947142.git.gitgitgadget@gmail.com>

On Mon, Sep 06 2021, Han-Wen Nienhuys via GitGitGadget wrote:

> In CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com,

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.

Show 8 quoted lines

>  	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...

Show 32 quoted lines

> -			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...

Show 18 quoted lines

> +		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?