Re: [PATCH v4] test_dir_is_empty: fix edge cases with newlines and hyphens

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 12, 2018, 19:50 UTC
Message-ID: <xmqq7ejqlclg.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180912183534.7486-1-wchargin@gmail.com>

William Chargin <wchargin@gmail.com> writes:

Show 7 quoted lines

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