> -----Original Message----- > From: Jeff King [mailto:peff@xxxxxxxx] > Sent: Friday, February 10, 2017 4:15 PM > To: David Turner <David.Turner@xxxxxxxxxxxx> > Cc: git@xxxxxxxxxxxxxxx; pclouds@xxxxxxxxx; Junio C Hamano > <gitster@xxxxxxxxx> > 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. Process_log_file_at_exit calls fflush. Will fix the other.