Re: [PATCH] notes-merge: use O_EXCL to avoid overwriting existing files

From: Jeff King <peff@peff.net>
Date: Jul 7, 2016, 20:38 UTC
Message-ID: <20160707203822.GB11804@sigill.intra.peff.net>
In-Reply-To: <577EB6BE.6090504@web.de>

On Thu, Jul 07, 2016 at 10:08:30PM +0200, René Scharfe wrote:

> Use the open(2) flag O_EXCL to ensure the file doesn't already exist
> instead of (racily) calling stat(2) through file_exists().  While at it
> switch to xopen() to reduce code duplication and get more consistent
> error messages.

This is definitely an improvement, as it behaves the same except for the TOCTOU race. But not being very familiar with the notes-merge code, I have to wonder if this is a system of a larger design issue.

Why do we care that the file exists? Should we instead be using the lockfile code to get exclusive access to it? That would also switch us to doing the write-to-tempfile-and-rename dance, but that seems like it would be a good thing. If we hit a write() error in the code now, we leave a partially-written file in the notes worktree.

I dunno. From my cursory reading of the code, it seems like we'd never really expect this file_exists() to trigger in the first place, so perhaps it's not worth thinking too hard about it.

-Peff