On Fri, Feb 10, 2017 at 08:44:27PM +0000, David Turner wrote:
> > But maybe there are other systems emulating fstat() would trigger here.
> > I dunno.
>
> Yeah, I'm also not sure. On the other hand, if we're going to catch fstat
> errors anyway, we might as well do something sensible with this one.
I'd say it's probably not worth worrying about here. We don't think it
can happen, and it would just fall-through to the "woah, fstat failed"
code path if it does.
> > If there was an i/o error, then gc.log.lock still exists. And this attempt to lock will
> > therefore fail, calling die(). Which would trigger our atexit() to call process_log(),
> > which would hit this code again, and so forth. I'm not sure if we'd actually
> > recurse when an atexit handler calls exit(). But it seems questionable.
>
> No, because we call rollback_log_file first.
Ah, right, sorry I missed that.
> > I'm also not sure why we need to re-open the file in the first place. We have an
> > open descriptor (and we even redirected stderr to it already).
> > Why don't we just write to it?
>
> If fstat failed, that probably indicates something bad about the old fd. I'm not
> actually sure why fstat would ever fail, since in all likelihood, the kernel keeps
> information about inodes corresponding to open fds in-memory. Maybe someone
> forcibly unmounted the drive?
It seems like the re-open would fail then, too. And the error message
for that would go to stderr, which goes to...the old file.
I dunno. This seems like a lot of manual scrambling to try to overcome
unlikely errors just to make our stderr heard (and we'd still fail in
some cases). It seems like:
if (fstat(...)) {
/* weird, fstat failed; try our best to mention it */
error_errno("unable to fstat gc.log.lock");
commit_lock_file(&log_lock));
} else if (st.st_size) {
/* we have new errors to report */
commit_lock_file(&log_lock);
} else {
/* no new errors; clean up old ones */
unlink(git_path("gc.log"));
rollback_lock_file(&log_lock);
}
would be sufficient.
-Peff