Re-added -hackers.
Indeed, I left it out by accident. I tried to bounce the original mail but
it did not work.
Thanks for reviewing.
On Fri, Dec 27, 2019 at 05:22:47PM +0100, Fabien COELHO wrote:
The implementation simply extends an existing functions with a boolean to
allow for sub-directories. However, the function does not seem to show
subdir contents recursively. Should it be the case?
STM that "//"-comments are not project policy.
Sure, but the patch is less important than the design, which needs to be
addressed first. The goal is to somehow show tmpfiles (or at least dirs) used
by parallel workers. I mentioned a few possible ways, of which this was the
simplest to implement. Showing files beneath the dir is probably good, but
need to decide how to present it. Should there be a column for the dir (null
if not a shared filesets)? Or some other presentation, like a boolean column
"is_shared_fileset".
Why not simply showing the files underneath their directories?
/path/to/tmp/file1
/path/to/tmp/subdir1/file2
In which case probably showing the directory itself is not useful,
and the is_dir column could be dropped?
I'm unconvinced by the skipping condition:
+ if (!S_ISREG(attrib.st_mode) &&
+ (!dir_ok && S_ISDIR(attrib.st_mode)))
continue;
which is pretty hard to read. ISTM you meant "not A and not (B and C)"
instead?
I can write it as two ifs.
Hmmm. Not sure it would help that much. At least the condition must be
right. Also, the comment should be updated.
And, it's probably better to say:
if (!ISDIR() || !dir_ok)
I cannot say I like it. dir_ok is cheaper to test so could come first.
..which is same as: !(B && C), as you said.
Ok, so you confirm that the condition was wrong.
Catalog update should be a date + number? Maybe this is best left to
the committer?
Yes, but I preferred to include it, causing a deliberate conflict, to ensure
it's not forgotten.
Ok.
--
Fabien.
