Re: [PATCH v4] test_dir_is_empty: fix edge cases with newlines and hyphens
flat view
From: Junio C Hamano <hidden>
Date: 2018-09-12 19:50:24
William Chargin [off-list ref] writes:
While the `test_dir_is_empty` function appears correct in most normal use cases, it can improperly pass if a directory contains a filename with a newline, and can improperly fail if an empty directory looks like an argument to `ls`. This patch changes the implementation to check that the output of `ls -a` has at most two lines (for `.` and `..`), which should be better behaved, and adds the `--` delimiter before the directory name when invoking `ls`.
AFIAK dot and dot-dot are allowed not to exist; "at most two" is not a good test. Quite honestly, our tests are still run inside a sort-of controlled environment, so if it _requires_ use of things we have avoided depending on, like "ls -A" and "xargs -0", or the fact that most filesystems always have "." and ".." even in an empty directory, in order to be resistant to funnily-named files like dot-LF-dot, I would say it is not worth worrying about these funny names--instead we can simply refrain from using such a pathological name, can't we? In other words, is there a real-world need in the context of our test suite for this change? Also, I find that its support for directories whose names begin with a dash red-herring. All the test scripts in our test suite knows that they can prefix "./" to avoid problems, i.e. test_dir_is_empty ./--wat So it appears that the only problematic case is when we create a directory, create a file or a directory whose name is dot-LF-dot and nothing else, and then do something that ought to cause that file to disappear, and make sure that the directory is empty, e.g. mkdir empty && echo foo >"empty/$dotLFdot" && git add "empty/$dotLFdot" && git reset --hard && test_dir_is_empty empty We do want to make sure funny names can be added with "git add" and "git reset --hard" to HEAD that lacked those paths with funny names to remove them correctly. But the funny names used in such a test do not have to be $dotLFdot; you can use "${dotLFdot}X" instead in the above and can ensure whatever the original test wanted to ensure. So...