From: David Turner Date: Fri, 10 Feb 2017 21:23:15 GMT Subject: RE: [PATCH v5] gc: ignore old gc.log files Message-ID: <069b229470204235b7155b33da52421c@exmbdft7.ad.twosigma.com> In-Reply-To: <20170210211510.zbustcl5ickfiqdy@sigill.intra.peff.net> > -----Original Message----- > From: Jeff King [mailto:peff@peff.net] > Sent: Friday, February 10, 2017 4:15 PM > To: David Turner > Cc: git@vger.kernel.org; pclouds@gmail.com; Junio C Hamano > > 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.