Show 37 quoted lines
> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> Sent: Friday, February 10, 2017 4:15 PM
> To: David Turner <David.Turner@twosigma.com>
> Cc: git@vger.kernel.org; pclouds@gmail.com; Junio C Hamano
> <gitster@pobox.com>
> Subject: Re: [PATCH v5] gc: ignore old gc.log files
>
> > @@ -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.