From: Jeff King <peff@peff.net>
Date: Sat, 31 Dec 2016 06:26:07 GMT
Subject: Re: [PATCH v3 11/23] log_ref_setup(): separate code for create vs non-create
Message-ID: <20161231062607.uxftwujophv37ymb@sigill.intra.peff.net>
In-Reply-To: <d9f96df1bb2d5b9a95388da04b770ea9f317c491.1483153436.git.mhagger@alum.mit.edu>

On Sat, Dec 31, 2016 at 04:12:51AM +0100, Michael Haggerty wrote:

> +	} else {
> +		logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
>  		if (logfd < 0) {
> -			strbuf_addf(err, "unable to append to '%s': %s",
> -				    logfile->buf, strerror(errno));
> -			return -1;
> +			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->buf, strerror(errno));
> +				return -1;
> +			}
>  		}
>  	}
>  
> -	adjust_shared_perm(logfile->buf);
> -	close(logfd);
> +	if (logfd >= 0) {
> +		adjust_shared_perm(logfile->buf);
> +		close(logfd);
> +	}
> +

Hmm. I would have thought in the existing-logfile case that we would not
need to adjust_shared_perm(). But maybe we just do it anyway to pick up
potentially-changed config.

I also had to double-take at this close(). Aren't we calling this
function so we can actually write to the log? But I skipped ahead in
your series and see you address that confusion. :)

-Peff