Re: [PATCH v5] gc: ignore old gc.log files

[Jeff King <peff@peff.net>]

> @@ -76,10 +78,30 @@ static void git_config_date_string(const char *key, const char **output)
>  static void process_log_file(void)
>  {
>  	struct stat st;
> -	if (!fstat(get_lock_file_fd(&log_lock), &st) && st.st_size)
> +	if (fstat(get_lock_file_fd(&log_lock), &st)) {
> +		/*
> +		 * Perhaps there was an i/o error or another
> +		 * unlikely situation.  Try to make a note of
> +		 * this in gc.log along with any existing
> +		 * messages.
> +		 */
> +		FILE *fp;
> +		int saved_errno = errno;
> +		fp = fdopen(log_lock.tempfile.fd, "a");

We usually use xfdopen() to handle (unlikely) errors rather than
segfaulting.  But I think you'd actually want fdopen_lock_file(), which
attaches the fd to the tempfile for flushing and cleanup purposes.

That said, I'm not sure I understand why you're opening a new stdio
filehandle. We know that stderr already points to our logfile (that's
how content gets there in the first place). If there's a problem with
the file or the descriptor, opening a new filehandle around the same
descriptor won't help.

Speaking of stderr, I wonder if this function should be calling
fflush(stderr) before looking at the fstat result. There could be
contents buffered there that haven't been written out yet (not from
child processes, but perhaps ones written in this process itself).
Probably unlikely in practice, since stderr is typically unbuffered by
default.

-Peff