From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Wed, 03 May 2017 15:33:07 GMT
Subject: Re: [PATCH v2 21/21] t1308: add a test case on open a config directory
Message-ID: <alpine.DEB.2.20.1705031730040.3480@virtualbox>
In-Reply-To: <20170503101706.9223-22-pclouds@gmail.com>

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> We don't support config-as-a-directory (maybe someday we will?). Make
> sure we consistently fail in this case, which should happen on platforms
> where fopen(<directory>) returns non-NULL if FREAD_READS_DIRECTORIES is
> defined.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

How about saying that we make sure that this is the case by adding a test
case? Otherwise the reader might (and this reader certainly did) expect a
code change in a .c file.

I reviewed this patch series, and am happy with the overall diff.

My main problem: it should be organized in a more intuitively-graspable
way, i.e. put all the fopen() -> fopen_or_warn() changes into a single
patch, separate out changes (drive-by test fixing for Windows and, ahem,
whitespace changes) into their own commits. Then those commits should be
ordered to relate an easy-flowing story.

Other than that: well done,
Dscho