On Wed, Mar 04, 2020 at 06:38:39AM -0700, Tracey Emery wrote:
> On Wed, Mar 04, 2020 at 09:59:08AM +0100, Stefan Sperling wrote:
> > > +		while (parent && strcmp(parent, a->worktree->root_path) != 0) {
> > > +			if (rmdir(parent) == -1) {
> > > +				if (errno != ENOTEMPTY)
> > > +					err = got_error_from_errno2("rmdir",
> > > +					    parent);
> > > +				break;
> > > +			}
> > > +			parent = dirname(parent);
> > 
> > Couldn't we simply reuse remove_ondisk_file() instead of hand-rolled unlink +
> > loop over rmdir?
> 
> I'd prefer to, and that was the original direction I was going to take.
> However, I wasn't quite sure what the code was trying to cover with the
> unlinkat vs unlink call, which remove_ondisk_file does not account for.
> Is there some relative path that unlink won't handle in the removal,
> which doesn't exist in a rebase trim? If there are no concerns there, I
> can send out a different diff later today.

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.