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.

> 
> >  	}
> >  
> >  	got_fileindex_entry_mark_deleted_from_disk(ie);
> > blob - 9ea8be8b1e091a6c1974b3a81b2f05a550bcd266
> > file + regress/cmdline/rm.sh
> > --- regress/cmdline/rm.sh
> > +++ regress/cmdline/rm.sh
> > @@ -239,6 +239,18 @@ function test_rm_directory {
> >  		return 1
> >  	fi
> >  
> > +	(cd $testroot/wt && ls -l > $testroot/stdout)
> > +
> > +	echo -n '' > $testroot/stdout.expected
> > +
> > +	cmp -s $testroot/stdout.expected $testroot/stdout
> > +	ret="$?"
> > +	if [ "$ret" != "0" ]; then
> > +		diff -u $testroot/stdout.expected $testroot/stdout
> > +		test_done "$testroot" "$ret"
> > +		return 1
> > +	fi
> > +
> >  	test_done "$testroot" "$ret"
> >  }
>  
> Test change looks good.
> 
> In addition, below is a patch with more elaborate tests with several levels
> of removal. The new rm.sh test added here fails without your change, the
> revert test passes either way. I was concerned whether revert would still
> work with your patch, the answer is yes it does :-)

Great!

-- 

Tracey Emery