On Wed, Jun 14, 2017 at 01:15:44AM -0400, Jeff King wrote:
> > I couldn't reproduce either with my usual build, but I don't usually use
> > autoconf. Running:
> >
> > make configure
> > ./configure
> > make
> > (cd t && ./t1308-*)
> >
> > does fail for me. The problem is that the generated config.mak.autogen
> > sets the wrong value for FREAD_READS_DIRECTORIES (and overrides the
> > default entry for Linux from config.mak.uname. So the configure script
> > needs to be fixed.
>
> Actually, I'm not sure if configure.ac is wrong, or the new uses of
> FREAD_READS_DIRECTORIES. Because the test configure.ac actually checks:
Poking around more, I think the best thing is to just update the
configure script. The rationale is below.
-- >8 --
Subject: [PATCH] configure.ac: loosen FREAD_READS_DIRECTORIES test program
We added an FREAD_READS_DIRECTORIES Makefile knob long ago
in cba22528f (Add compat/fopen.c which returns NULL on
attempt to open directory, 2008-02-08) to handle systems
where reading from a directory returned garbage. This works
by catching the problem at the fopen() stage and returning
NULL.
More recently, we found that there is a class of systems
(including Linux) where fopen() succeeds but fread() fails.
Since the solution is the same (having fopen return NULL),
they use the same Makefile knob as of e2d90fd1c
(config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and
FreeBSD, 2017-05-03).
This works fine except for one thing: the autoconf test in
configure.ac to set FREAD_READS_DIRECTORIES actually checks
whether fread succeeds. Which means that on Linux systems,
the knob isn't set (and we even override the config.mak.uname
default). t1308 catches the failure.
We can fix this by tweaking the autoconf test to cover both
cases. In theory we might care about the distinction between
the traditional "fread reads directories" case and the new
"fopen opens directories". But since our solution catches
the problem at the fopen stage either way, we don't actually
need to know the difference. The "fopen" case is a superset.
This does mean the FREAD_READS_DIRECTORIES name is slightly
misleading. Probably FOPEN_OPENS_DIRECTORIES would be more
accurate. But it would be disruptive to simply change the
name (people's existing build configs would fail), and it's
not worth the complexity of handling both. Let's just add a
comment in the knob description.
Reported-by: Øyvind A. Holm <sunny@xxxxxxxxxxx>
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
Makefile | 3 ++-
configure.ac | 4 ++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index 7c621f7f7..33b888730 100644
--- a/Makefile
+++ b/Makefile
@@ -19,7 +19,8 @@ all::
# have been written to the final string if enough space had been available.
#
# Define FREAD_READS_DIRECTORIES if you are on a system which succeeds
-# when attempting to read from an fopen'ed directory.
+# when attempting to read from an fopen'ed directory (or even to fopen
+# it at all).
#
# Define NO_OPENSSL environment variable if you do not have OpenSSL.
# This also implies BLK_SHA1.
diff --git a/configure.ac b/configure.ac
index deeb968da..602383ed1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -869,9 +869,9 @@ AC_CACHE_CHECK([whether system succeeds to read fopen'ed directory],
[
AC_RUN_IFELSE(
[AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT],
- [[char c;
+ [[
FILE *f = fopen(".", "r");
- return f && fread(&c, 1, 1, f)]])],
+ return f)]])],
[ac_cv_fread_reads_directories=no],
[ac_cv_fread_reads_directories=yes])
])
--
2.13.1.766.g6bea926c5