patcht/test-lib: make `test_dir_is_empty` more robust

While the `test_dir_is_empty` function appears correct in most normal use cases, it can fail when filenames contain newlines. I originally wrote this patch for the standalone Sharness library, but that library advises that such patches be sent to the Git mailing list first.

William Chargin (1):
  t/test-lib: make `test_dir_is_empty` more robust

 t/t0000-basic.sh        | 29 +++++++++++++++++++++++++++++
 t/test-lib-functions.sh |  2 +-
 2 files changed, 30 insertions(+), 1 deletion(-)

The previous behavior could incorrectly pass given a directory with a filename containing a newline. This patch changes the implementation to use `ls -A`, which is specified by POSIX. The output should be empty exactly if the directory is empty.

The newly added unit test fails before this change and passes after it.

Signed-off-by: William Chargin <wchargin@gmail.com>
---
 t/t0000-basic.sh        | 29 +++++++++++++++++++++++++++++
 t/test-lib-functions.sh |  2 +-
 2 files changed, 30 insertions(+), 1 deletion(-)

Hi,

William Chargin wrote:

This subject line will appear out of context in "git shortlog" output, so it's useful to pack in enough information to briefly summarize what the change does. How about something like

	test_dir_is_empty: avoid being confused by $'.\n.' file

or

	test_dir_is_empty: simplify by using "ls --almost-all"

?

[...]

I don't think "xargs -0" is present on all supported platforms. I'd be tempted to use

		>pathological_dir/$'.\n.'

but $'' is too recent of a shell feature to count on (e.g., dash doesn't support it). See t/t3600-rm.sh for an example of a portable way to do this.

Not all filesystems support newlines in filenames. I think we'd want to factor out the FUNNYNAMES prereq from there into a test_lazy_prereq so this test can be skipped on such filenames.

[...]

Another portability gotcha: wc output includes a space on Mac so this test would always return true there. How about

	if test -n "$(ls -A1 "$1")"

?

"ls -A" was added in POSIX.1-2017.  Its rationale section explains

	Earlier versions of this standard did not describe the BSD -A
	option (like -a, but dot and dot-dot are not written out). It
	has been added due to widespread implementation.

That's very recent, but the widespread implementation it mentions is less so. This would be our first use of "ls -A", so I'd be interested to hear from people on more obscure platforms. It does seem to be widespread.

Can you say a little more about where you ran into this? Did you discover it by code inspection?

I do think the resulting implementation using -A is simpler. Thanks for writing it.

Thanks and hope that helps, Jonathan

On Sun, Aug 5, 2018 at 12:20 AM Jonathan Nieder <jrnieder@gmail.com> wrote:

A simpler approach, without the portability concerns of -A, would be to remove the "." and ".." lines from the top of the listing:

    ls -f1 "$1" | sed '1,2d'

If we're worried about -f not being sufficiently portable, then an even simpler approach would be to check whether the output of 'ls -a1' has more lines than the two expected ("." and ".."):

    test $(ls -a1 "$1" | wc -l) -gt 2

I think I favor this final implementation over the others.

On Sun, Aug 05, 2018 at 01:23:05AM -0400, Eric Sunshine wrote:

Perhaps even simpler:

  test "$1" = "$(find "$1")"

That will recurse any subdirectories, possibly wasting time, but since the point is that we expect it to be empty, that's probably OK.

Like Junio said elsewhere, I am not sure if it is even worth caring too much about pathological cases in the test suite, where we control all of the paths. But using `find` does shave off one process invocation, too. :)

-Peff

On Sun, Aug 05, 2018 at 04:52:31PM -0400, Jeff King wrote:

Actually, I guess it needs to add "-print", since IIRC that is not the default on some old versions of "find".

-Peff

On Mon, Aug 6, 2018 at 9:02 AM Jeff King <peff@peff.net> wrote:

You recall correctly.

I tend to avoid 'find' in scripts when I don't need its recursive behavior due to *extremely* poor performance on Windows (at least that was the case years ago), especially when there are a lot of files in the directory being listed. However, for this usage, we expect the directory to be empty, so perhaps performance isn't an issue.

One caveat involves invocations of `test_must_fail test_dir_is_empty`, wherein we _don't_ actually expect the directory to be empty. It looks like there might not be any such invocations in Git, though, for what it's worth.

The solution that counts the lines of `ls -a` seems like it should be universally portable, both in POSIX and in practice, and it doesn't complicate the code at all. (I'd say that it's slightly simpler than the `egrep` approach in the status quo.) I've adjusted the patch to use Eric's version, which I like a bit more than mine; I'll send out the revised commit presently.

It's true that within Git we can simply try to avoid creating pathological file names. But Sharness is also exposed as a standalone library, and it seems clear that the public function therein should be correct in all cases. Assuming that the new implementation is sufficiently inoffensive, is there any harm in being perhaps a bit more robust than is practically needed, and also maintaining consistency with Sharness?

Thanks for all the suggestions and feedback so far!

Best, WC

While the `test_dir_is_empty` function appears correct in most normal use cases, it can fail when filenames contain newlines. 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.

The newly added unit test fails before this change and passes after it.

Signed-off-by: William Chargin <wchargin@gmail.com>
---
This patch depends on "t: factor out FUNNYNAMES as shared lazy prereq"
(2018-08-06), available from `wc/make-funnynames-shared-lazy-prereq`.
The code will work on master, but the test will not run due to a missing
prereq.

I originally wrote this patch for the standalone Sharness library, but that library advises that such patches be sent to the Git mailing list first.

 t/t0000-basic.sh        | 31 +++++++++++++++++++++++++++++++
 t/test-lib-functions.sh |  2 +-
 2 files changed, 32 insertions(+), 1 deletion(-)

On Sun, Aug 12, 2018 at 12:07 AM William Chargin <wchargin@gmail.com> wrote:

We usually avoid "touch" unless the timestamp of the file is significant. In this case, it isn't, so it would be more idiomatic to say simply:

    >nonempty_dir/some_file &&

This is an abuse of test_must_fail() which is intended strictly for testing 'git' invocations which might fail for reasons other than the expected one (for instance, git might crash). Instead, you should just use '!', like this:

    ! test_dir_is_empty nonempty_dir

Same comments as above.

Thanks for the review.

Makes sense. Will change as you suggest.

Interesting. I didn't infer this from the docs on `test_must_fail` in `t/test-lib-functions.sh`. Sharness, which is supposed to be independent of Git, explicitly says to use `test_must_fail` instead of `!`. (Admittedly, the implementations are different, but only slightly: within Git, a Valgrind error 126 is a failure, not success.)

I also see other uses of `test_must_fail` throughout the codebase: e.g., with `kill`, `test`, `test_cmp`, `run_sub_test_lib_test`, etc. as the target command. Are these invocations in error?

(I'm nevertheless happy to change this as you suggest.)

I'll make these changes locally and hold on to the patch, waiting for others' input.

Best, WC

On Sun, Aug 12, 2018 at 2:33 AM William Chargin <wchargin@gmail.com> wrote:

This sort of knowledge is, perhaps unfortunately, spread around in too many places. In this case, it's mentioned in t/README. The relevant excerpt:

    Don't use '! git cmd' when you want to make sure the git command
    exits with failure in a controlled way by calling "die()".
    Instead, use 'test_must_fail git cmd'.  This will signal a failure
    if git dies in an unexpected way (e.g. segfault).

    On the other hand, don't use test_must_fail for running regular
    platform commands; just use '! cmd'.  We are not in the business
    of verifying that the world given to us sanely works.

It probably wouldn't hurt to update the comment block above test_must_fail() in t/test-functions-lib.sh.

Looks that way, even run_sub_test_lib_test().

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`.

The newly added unit test fails before this change and passes after it.

Signed-off-by: William Chargin <wchargin@gmail.com>
---
This patch depends on "t: factor out FUNNYNAMES as shared lazy prereq"
(2018-08-06), which is now in master.

I originally wrote this patch for the standalone Sharness library, but that library advises that such patches be sent to the Git mailing list first.

Tested on GNU/Linux (Mint 18.2) and macOS (10.13).

 t/t0000-basic.sh        | 43 +++++++++++++++++++++++++++++++++++++++++
 t/test-lib-functions.sh |  2 +-
 2 files changed, 44 insertions(+), 1 deletion(-)

William Chargin <wchargin@gmail.com> writes:

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...

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`.

The newly added unit test fails before this change and passes after it.

Signed-off-by: William Chargin <wchargin@gmail.com>
---
This patch depends on "t: factor out FUNNYNAMES as shared lazy prereq"
(2018-08-06), which is now in master.

I originally wrote this patch for the standalone Sharness library, but that library advises that such patches be sent to the Git mailing list first.

Tested on GNU/Linux (Mint 18.2) and macOS (10.13).

 t/t0000-basic.sh        | 43 +++++++++++++++++++++++++++++++++++++++++
 t/test-lib-functions.sh |  2 +-
 2 files changed, 44 insertions(+), 1 deletion(-)

Hi,

I'm happy to do so. I think that "simplify" is misleading, because this is a bug fix, not a refactoring. I like your first suggestion, though it exceeds the 50-character soft limit. What do you think of:

        test_dir_is_empty: find files with newline in name

?

Wow---I'm shocked that it's not POSIX, but you're right. (That makes `xargs` so much less useful!)

t/t3600-rm.sh seems to just literally embed the newline into the argument to `touch`. I can do that. (I intentionally avoided $'' for the reason that you mention.)

This makes sense. Would you like me to send in a separate patch to add this `test_lazy_prereq` to `t/test-lib.sh`, fixing up existing uses (of which there are several), and then rebase this patch on top of it?

That's good to know. I can use `test -n "$(ls -A1 "$1")"` as you suggest, although...

...if you prefer, a variant of `test "$(ls -a1 | wc -l)" -eq 2` should satisfy all the crtieria that you mention above (POSIX since forever, should work on Mac). The assumption is that a file with a newline character may take up more than one line, but every file must take up _at least_ one line, and we get two lines from `.` and `..`. If this assumption is false, then I will have learned yet another new thing!

Sure. Yes. I have a build script that accepts a target output directory, and rejects if the directory is nonempty. I used `ls -A | wc -l` to implement this check. When testing the script with Sharness, I ran across `test_dir_is_empty`. I was curious about the implementation, having recently implemented the same test myself. The `egrep` in the implementation stood out to me as suspicious, and so it was easy to come up with an explicit counterexample.

You're welcome. Thank you for the detailed and helpful review.

Best, WC

William Chargin wrote:

Thanks. I think we can ignore the 50-character soft limit. It's often too low and expressivity is more important. So if you like my first suggestion, then I'd say start with that and tweak to your taste from there.

[...]

To be clear, as Junio mentioned, POSIX is useful as a guide to what *might* be portable, but what we care about is what is portable to the platforms used in practice.

[...]

Thanks for the offer! Yes, that would be nice: such a patch would be nice as a cleanup in its own right, too.

Such a patch would be helpful at any time. For rebasing this patch, my advice would be to hold off until the discussion has settled down a bit. If we're lucky, people in other time zones might have an idea beyond the ones we've come up with.

[...]

As a piece of trivia, '.' and '..' are allowed not to exist. So this test can have false negatives and false positives.

Filesystems omitting them are quite rare so this might work reasonably well in practice, though.

Thanks, Jonathan

Jonathan Nieder <jrnieder@gmail.com> writes:

Is that "too recent"? I thought it was bashism, not even in POSIX, but I may be mistaken.

Quite honestly, our tests are still run inside a sort-of controlled environment, so if it _requires_ use of things we have avoided so far, like "ls -A" and "xargs -0", in order to be resistant to funnyly-named files like dot-LF-dot, I would say it is not worth worrying about them--instead we can simply refrain from using such a pathological name, can't we?

"ls -A" may be in POSIX, but our attitude generally is to avoid saying things like "it is in POSIX so it's your platform's fault that it is not yet supported". We instead say "even it may be in POSIX, in real life many people don't have it, so let's avoid it". And "xargs -0" I do not think is.

Hi,

Junio C Hamano wrote:

You're right. I got a little ahead of myself: it's not part of POSIX yet but is likely to be so once the details get ironed out: http://austingroupbugs.net/view.php?id=249

The "xargs -0" is a bit of a red herring. That construct is definitely not needed for the test it was used in.

For "ls -A", I agree with you that the benefit is not very high, so the cost would have to be pretty low for this to be worth it. But given the lineage of "ls -A", I feel there's a chance that it's widespread enough that it would meet that bar.

Indeed.

Thanks, Jonathan

Hi,

William Chargin wrote:

This information belongs in the commit message, since it's useful context for understanding the motivation behind the patch when encountering it with e.g. "git log". That's part of why I recommend never sending a separate cover-letter email for a single-patch series.

See [1] from Documentation/SubmittingPatches for more on this subject.

Thanks for writing it! Continuing the note about administrivia, this kind of cover letter material that you want to not be part of the commit message can go below the three-dashes delimiter when you send a patch. There's more about this at [2].

Thanks again, Jonathan

[1] https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html#describe-changes [2] https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html#send-patches

Hi Jonathan,

Agreed: I included it at the start of the commit message, though I
suppose that the wording in the cover letter is clearer, so I've amended
the commit to use that wording instead.

Perfect; this is what I was missing. Thanks. Let me try again. :-)

While the `test_dir_is_empty` function appears correct in most normal use cases, it can fail when filenames contain newlines. This patch changes the implementation to use `ls -A`, which is specified by POSIX. The output should be empty exactly if the directory is empty.

The newly added unit test fails before this change and passes after it.

Signed-off-by: William Chargin <wchargin@gmail.com>
---

I originally wrote this patch for the standalone Sharness library, but that library advises that such patches be sent to the Git mailing list first.

 t/t0000-basic.sh        | 29 +++++++++++++++++++++++++++++
 t/test-lib-functions.sh |  2 +-
 2 files changed, 30 insertions(+), 1 deletion(-)

Am 05.08.2018 um 06:20 schrieb William Chargin:

Did you accidentally resend the same patch in this v2? I can't spot the difference to v1.

-- Hannes