[PATCH] D85690: [split-file] Fix sys::fs::remove() on Solaris after D83834

Mon Aug 10 16:13:15 PDT 2020

dblaikie added inline comments.

================
Comment at: llvm/tools/split-file/split-file.cpp:158-159

   // Delete output if it is a file or an empty directory, so that we can create
   // a directory.
   sys::fs::file_status status;
----------------
MaskRay wrote:
> dblaikie wrote:
> > Perhaps it should (at least, this was my original idea/suggestion when I proposed this behavior - not sure if I was clear/specified that, though) delete the contents even if it is non-empty. This would help make tests more hermetic/less likely to have the weird state of "oh, I accidentally made a test that changes behavior when run a second time/with files left around" (we see this sometimes and need to explicitly rm things in the test - sometimes then cleaning up the test later once the bad files are no longer present/problematic)
> I'd like to avoid recursive remove, which can easily cause damage to the system (e.g. `split-file %s .` will delete the current directory)
> 
> I have thought about hermetic issues, but in the end I don't think leaving files with split-file can more easily trap the user.
> 
> If the user wants to remove the directory, make it clear with an explicit `rm -fr %t`.
Sure - though given the fairly narrow usage of the tool, predominantly in LLVM test cases where the directory given is a unique subdirectory for just this test files tests, it seemed like it'd be a chance to make tests more reliable by not having lots of leftover files. (I wouldn't be totally averse to that directory deleting having some heuristic about there not being too many files in the directory - since it's only expected to be cleaning up a small test directory).

I don't think split-file will make the situation worse - but seemed like an opportunity for it to make the situation better.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85690/new/

https://reviews.llvm.org/D85690