On Wed, Mar 04, 2020 at 08:43:24AM -0700, Tracey Emery wrote:
> On Wed, Mar 04, 2020 at 04:12:40PM +0100, Stefan Sperling wrote:
> > 
> > Indeed, this make it hard to reuse remove_ondisk_file() as it is.
> > We could move the unlinkat() into remove_ondisk_file() and pass dirfd
> > in as a new parameter, and check dirfd for -1 inside remove_ondisk_file().
> > 
> > The point of unlinkat is that the correct files get reported by 'got status'
> > and removed by 'got rm', if the directory we're operating within happens to
> > be deleted and recreated concurrently to the status crawl.
> > 
> > With unlink() via a path computed from parentdir + filename, we cannot be 100%
> > sure that the file is really a child of the directory we opened at parentdir's
> > path. Instead of a path, unlinkat() uses a file descriptor which represents
> > the parent directory. This descriptor remains valid even if the directory's
> > path gets deleted and replaced in the meantime.
> 
> How does this look, then. Did I approach this correctly? There is also
> some added error checking in remove_ondisk_file.

Hmm, looking at it now I am not entirely happy with this diff either.
Sorry, I did not think my suggestion through properly.

The problem now is that even though we pass a directory fd and have
remove_ondisk_file() unlink the file itself in the non-racy way, the
function then proceeds to delete the parent directories in the racy
way which is based on parent paths.

And currently we have no better way to do it.
What we would need to do is pass a chain of file descriptors around during
the status walk, one fd for each parent dir that which has been opened.
This way, remove_ondisk_file could loop over parent directory file
descriptors and remove empty dirs with unlinkat + AT_REMOVEDIR.
Maybe better: We could create a mechanism to have directories be flagged
for removal when the status walk is done traversing them, and remove
each affected dir at the scope which opened it.

But don't worry about any of that now. This race can be dealt with later.
Your original worktree.c patch doesn't make anything worse, and it was a
better starting position for further changes in this direction, I think.