Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Aug 5, 2018, 06:34 UTC
Message-ID: <20180805063448.GC44140@aiede.svl.corp.google.com>
In-Reply-To: <CAFW+GMBrRUQ88XZzLMVAiEGDjLDwLaW6Qi1mSGrXYHoSu8iJag@mail.gmail.com>

William Chargin wrote:

> Jonathan Nieder wrote:

Show 9 quoted lines

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

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.

[...]

>> I don't think "xargs -0" is present on all supported platforms
>
> Wow---I'm shocked that it's not POSIX, but you're right. (That makes
> `xargs` so much less useful!)

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.

[...]

Show 7 quoted lines

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

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.

[...]

Show 12 quoted lines

>> "ls -A" was added in POSIX.1-2017. [...]
>> 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.
>
> ...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!

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