t1308-config-set.sh fails on current master

t1308-config-set.sh fails on current master

[Øyvind A. Holm]

[-- Attachment #1: Type: text/plain, Size: 2327 bytes --]

t1308-config-set.sh fails on current master (v2.13.1-449-g02a2850ad58e). 
The error is introduced by commit e2d90fd1c33a ("config.mak.uname: set 
FREAD_READS_DIRECTORIES for Linux and FreeBSD"). Reverting the commit 
results in a conflict, but the test works on the commit before, 
02912f477586.

Tested on

  Debian GNU/Linux 8.8 (jessie)
  Linux Mint 18 Sarah

Test output:

  $ ./t1308-config-set.sh
  ok 1 - setup default config
  ok 2 - get value for a simple key
  ok 3 - get value for a key with value as an empty string
  ok 4 - get value for a key with value as NULL
  ok 5 - upper case key
  ok 6 - mixed case key
  ok 7 - key and value with mixed case
  ok 8 - key with case sensitive subsection
  ok 9 - key with case insensitive section header
  ok 10 - key with case insensitive section header & variable
  ok 11 - find value with misspelled key
  ok 12 - find value with the highest priority
  ok 13 - find integer value for a key
  ok 14 - find string value for a key
  ok 15 - check line error when NULL string is queried
  ok 16 - find integer if value is non parse-able
  ok 17 - find bool value for the entered key
  ok 18 - find multiple values
  ok 19 - find value from a configset
  ok 20 - find value with highest priority from a configset
  ok 21 - find value_list for a key from a configset
  ok 22 - proper error on non-existent files
  not ok 23 - proper error on directory "files"
  #
  #               echo "Error (-1) reading configuration file a-directory." >expect &&
  #               mkdir a-directory &&
  #               test_expect_code 2 test-config configset_get_value foo.bar a-directory 2>output &&
  #               grep "^warning:" output &&
  #               grep "^Error" output >actual &&
  #               test_cmp expect actual
  #
  ok 24 - proper error on non-accessible files
  ok 25 - proper error on error in default config files
  ok 26 - proper error on error in custom config files
  ok 27 - check line errors for malformed values
  ok 28 - error on modifying repo config without repo
  ok 29 - iteration shows correct origins
  # failed 1 among 29 test(s)
  1..29
  $

Øyvind

N 60.376° E 5.3334°
OpenPGP fingerprint: A006 05D6 E676 B319 55E2  E77E FB0C BEE8 94A5 06E5
2daabdde-509d-11e7-a17a-db5caa6d21d3

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: t1308-config-set.sh fails on current master

[Jonathan Nieder]

Hi Øyvind,

Øyvind A. Holm wrote:

> t1308-config-set.sh fails on current master (v2.13.1-449-g02a2850ad58e). 
> The error is introduced by commit e2d90fd1c33a ("config.mak.uname: set 
> FREAD_READS_DIRECTORIES for Linux and FreeBSD"). Reverting the commit 
> results in a conflict, but the test works on the commit before, 
> 02912f477586.
>
> Tested on
>
>   Debian GNU/Linux 8.8 (jessie)
>   Linux Mint 18 Sarah

Interesting.  I'm not able to reproduce it, but of course that doesn't
mean much.

> Test output:
> 
>   $ ./t1308-config-set.sh
>   ok 1 - setup default config
>   ok 2 - get value for a simple key
>   ok 3 - get value for a key with value as an empty string
>   ok 4 - get value for a key with value as NULL
>   ok 5 - upper case key
>   ok 6 - mixed case key
>   ok 7 - key and value with mixed case
>   ok 8 - key with case sensitive subsection
>   ok 9 - key with case insensitive section header
>   ok 10 - key with case insensitive section header & variable
>   ok 11 - find value with misspelled key
>   ok 12 - find value with the highest priority
>   ok 13 - find integer value for a key
>   ok 14 - find string value for a key
>   ok 15 - check line error when NULL string is queried
>   ok 16 - find integer if value is non parse-able
>   ok 17 - find bool value for the entered key
>   ok 18 - find multiple values
>   ok 19 - find value from a configset
>   ok 20 - find value with highest priority from a configset
>   ok 21 - find value_list for a key from a configset
>   ok 22 - proper error on non-existent files
>   not ok 23 - proper error on directory "files"
>   #
>   #               echo "Error (-1) reading configuration file a-directory." >expect &&
>   #               mkdir a-directory &&
>   #               test_expect_code 2 test-config configset_get_value foo.bar a-directory 2>output &&
>   #               grep "^warning:" output &&
>   #               grep "^Error" output >actual &&
>   #               test_cmp expect actual
>   #
>   ok 24 - proper error on non-accessible files
>   ok 25 - proper error on error in default config files
>   ok 26 - proper error on error in custom config files
>   ok 27 - check line errors for malformed values
>   ok 28 - error on modifying repo config without repo
>   ok 29 - iteration shows correct origins
>   # failed 1 among 29 test(s)
>   1..29

What is the output of the following command?

	./t1308-config-set.sh --run=1,23 -x -v -i

Other diagnostics:

- what is the output of "env"?
- cat ../GIT-BUILD-OPTIONS
- if you run that under 'strace -f -o /tmp/strace.out', does the
  strace.out say anything interesting?

Thanks,
Jonathan

Re: t1308-config-set.sh fails on current master

[Øyvind A. Holm]

[-- Attachment #1: Type: text/plain, Size: 14002 bytes --]

Hi, Jonathan, thanks for having a look at this.

On 2017-06-13 18:25:35, Jonathan Nieder wrote:
> Øyvind A. Holm wrote:
> > t1308-config-set.sh fails on current master 
> > (v2.13.1-449-g02a2850ad58e). The error is introduced by commit 
> > e2d90fd1c33a ("config.mak.uname: set FREAD_READS_DIRECTORIES for 
> > Linux and FreeBSD"). Reverting the commit results in a conflict, but 
> > the test works on the commit before, 02912f477586.
> >
> > Tested on
> >
> >   Debian GNU/Linux 8.8 (jessie)
> >   Linux Mint 18 Sarah
>
> Interesting.  I'm not able to reproduce it, but of course that doesn't
> mean much.

I'll admit that I have a somewhat special build system, but it's been 
working great since I created it 7 months ago, and I run the test suite 
every time I install a new git. I'm using the Makefile located at

  https://gitlab.com/sunny256/src-other/blob/master/devel/git/Makefile

It's only doing regular stuff like "make configure", "./configure", etc, 
but I'm mentioning it in case the Makefile reveals something 
interesting. The git installation is in a non-standard location, the 
newest version of git I've installed is for example located under 
/usr/src-other/pool/git.master.v2.13.1-394-g41dd4330a121/ .

> What is the output of the following command?
>
> ./t1308-config-set.sh --run=1,23 -x -v -i

Initialized empty Git repository in /home/sunny/src/git/src-other/devel/git/git/t/trash directory.t1308-config-set/.git/
expecting success: 
	cat >.git/config <<-\EOF
	[case]
		penguin = very blue
		Movie = BadPhysics
		UPPERCASE = true
		MixedCase = true
		my =
		foo
		baz = sam
	[Cores]
		WhatEver = Second
		baz = bar
	[cores]
		baz = bat
	[CORES]
		baz = ball
	[my "Foo bAr"]
		hi = mixed-case
	[my "FOO BAR"]
		hi = upper-case
	[my "foo bar"]
		hi = lower-case
	[case]
		baz = bat
		baz = hask
	[lamb]
		chop = 65
		head = none
	[goat]
		legs = 4
		head = true
		skin = false
		nose = 1
		horns
	EOF

+ cat
ok 1 - setup default config

skipping test: get value for a simple key 
	check_config get_value case.penguin "very blue"

ok 2 # skip get value for a simple key (--run)

skipping test: get value for a key with value as an empty string 
	check_config get_value case.my ""

ok 3 # skip get value for a key with value as an empty string (--run)

skipping test: get value for a key with value as NULL 
	check_config get_value case.foo "(NULL)"

ok 4 # skip get value for a key with value as NULL (--run)

skipping test: upper case key 
	check_config get_value case.UPPERCASE "true" &&
	check_config get_value case.uppercase "true"

ok 5 # skip upper case key (--run)

skipping test: mixed case key 
	check_config get_value case.MixedCase "true" &&
	check_config get_value case.MIXEDCASE "true" &&
	check_config get_value case.mixedcase "true"

ok 6 # skip mixed case key (--run)

skipping test: key and value with mixed case 
	check_config get_value case.Movie "BadPhysics"

ok 7 # skip key and value with mixed case (--run)

skipping test: key with case sensitive subsection 
	check_config get_value "my.Foo bAr.hi" "mixed-case" &&
	check_config get_value "my.FOO BAR.hi" "upper-case" &&
	check_config get_value "my.foo bar.hi" "lower-case"

ok 8 # skip key with case sensitive subsection (--run)

skipping test: key with case insensitive section header 
	check_config get_value cores.baz "ball" &&
	check_config get_value Cores.baz "ball" &&
	check_config get_value CORES.baz "ball" &&
	check_config get_value coreS.baz "ball"

ok 9 # skip key with case insensitive section header (--run)

skipping test: key with case insensitive section header & variable 
	check_config get_value CORES.BAZ "ball" &&
	check_config get_value cores.baz "ball" &&
	check_config get_value cores.BaZ "ball" &&
	check_config get_value cOreS.bAz "ball"

ok 10 # skip key with case insensitive section header & variable (--run)

skipping test: find value with misspelled key 
	check_config expect_code 1 get_value "my.fOo Bar.hi" "Value not found for \"my.fOo Bar.hi\""

ok 11 # skip find value with misspelled key (--run)

skipping test: find value with the highest priority 
	check_config get_value case.baz "hask"

ok 12 # skip find value with the highest priority (--run)

skipping test: find integer value for a key 
	check_config get_int lamb.chop 65

ok 13 # skip find integer value for a key (--run)

skipping test: find string value for a key 
	check_config get_string case.baz hask &&
	check_config expect_code 1 get_string case.ba "Value not found for \"case.ba\""

ok 14 # skip find string value for a key (--run)

skipping test: check line error when NULL string is queried 
	test_expect_code 128 test-config get_string case.foo 2>result &&
	test_i18ngrep "fatal: .*case\.foo.*\.git/config.*line 7" result

ok 15 # skip check line error when NULL string is queried (--run)

skipping test: find integer if value is non parse-able 
	check_config expect_code 128 get_int lamb.head

ok 16 # skip find integer if value is non parse-able (--run)

skipping test: find bool value for the entered key 
	check_config get_bool goat.head 1 &&
	check_config get_bool goat.skin 0 &&
	check_config get_bool goat.nose 1 &&
	check_config get_bool goat.horns 1 &&
	check_config get_bool goat.legs 1

ok 17 # skip find bool value for the entered key (--run)

skipping test: find multiple values 
	check_config get_value_multi case.baz sam bat hask

ok 18 # skip find multiple values (--run)

skipping test: find value from a configset 
	cat >config2 <<-\EOF &&
	[case]
		baz = lama
	[my]
		new = silk
	[case]
		baz = ball
	EOF
	echo silk >expect &&
	test-config configset_get_value my.new config2 .git/config >actual &&
	test_cmp expect actual

ok 19 # skip find value from a configset (--run)

skipping test: find value with highest priority from a configset 
	echo hask >expect &&
	test-config configset_get_value case.baz config2 .git/config >actual &&
	test_cmp expect actual

ok 20 # skip find value with highest priority from a configset (--run)

skipping test: find value_list for a key from a configset 
	cat >except <<-\EOF &&
	sam
	bat
	hask
	lama
	ball
	EOF
	test-config configset_get_value case.baz config2 .git/config >actual &&
	test_cmp expect actual

ok 21 # skip find value_list for a key from a configset (--run)

skipping test: proper error on non-existent files 
	echo "Error (-1) reading configuration file non-existent-file." >expect &&
	test_expect_code 2 test-config configset_get_value foo.bar non-existent-file 2>actual &&
	test_cmp expect actual

ok 22 # skip proper error on non-existent files (--run)

expecting success: 
	echo "Error (-1) reading configuration file a-directory." >expect &&
	mkdir a-directory &&
	test_expect_code 2 test-config configset_get_value foo.bar a-directory 2>output &&
	grep "^warning:" output &&
	grep "^Error" output >actual &&
	test_cmp expect actual

+ echo Error (-1) reading configuration file a-directory.
+ mkdir a-directory
+ test_expect_code 2 test-config configset_get_value foo.bar a-directory
Value not found for "foo.bar"
error: last command exited with $?=1
not ok 23 - proper error on directory "files"
#	
#		echo "Error (-1) reading configuration file a-directory." >expect &&
#		mkdir a-directory &&
#		test_expect_code 2 test-config configset_get_value foo.bar a-directory 2>output &&
#		grep "^warning:" output &&
#		grep "^Error" output >actual &&
#		test_cmp expect actual
#	


> Other diagnostics:
>
> - what is the output of "env"?

GIT_PS1_SHOWDIRTYSTATE=1
SSH_AGENT_PID=22038
XDG_SESSION_ID=31
DSN=svn+ssh://sunny256@developer.skolelinux.no/repos
TERM=screen-256color-bce
SHELL=/bin/bash
SSH_CLIENT=37.253.243.58 43994 22
GL=git@gitlab.com:sunny256
LC_NUMERIC=C
OLDPWD=/home/sunny/src/git/src-other/devel/git/git
AFVROOT=/home/sunny/afvroot
GH=git@github.com:sunny256
LNS=sunny@sunbase.org:/home/sunny/src/git
SSH_TTY=/dev/pts/1
WHOIS_SERVER=whois.dotster.com
USER=sunny
HISTFILESIZE=10000000
LS_COLORS=no=00:fi=00:di=01;34:ln=01;36:pi=40;33:so=01;35:bd=40;33;01:cd=40;33;01:or=01;37;41:ex=01;32:*.bat=01;32:*.btm=01;32:*.cmd=01;32:*.com=01;32:*.csh=01;32:*.exe=01;32:*.sh=01;32:*.Z=01;31:*.arj=01;31:*.bz=01;31:*.bz2=01;31:*.cpio=01;31:*.deb=01;31:*.gz=01;31:*.lzh=01;31:*.rpm=01;31:*.tar=01;31:*.taz=01;31:*.tgz=01;31:*.tz=01;31:*.z=01;31:*.zip=01;31:*.avi=01;35:*.bmp=01;35:*.gif=01;35:*.jpg=01;35:*.mov=01;35:*.mpg=01;35:*.png=01;35:*.ppm=01;35:*.tga=01;35:*.tif=01;35:*.xbm=01;35:*.xpm=01;35:*.html=01;33:*.txt=01;33:*.utf8=01;33:*~=00;34:
MTOOLS_DATE_STRING=yyyy-mm-dd
MTOOLS_TWENTY_FOUR_HOUR_CLOCK=1
SSH_AUTH_SOCK=/tmp/ssh-VdBkCiAwUqdc/agent.22035
TERMCAP=SC|screen-256color-bce|VT 100/ANSI X3.64 virtual terminal:\
	:DO=\E[%dB:LE=\E[%dD:RI=\E[%dC:UP=\E[%dA:bs:bt=\E[Z:\
	:cd=\E[J:ce=\E[K:cl=\E[H\E[J:cm=\E[%i%d;%dH:ct=\E[3g:\
	:do=^J:nd=\E[C:pt:rc=\E8:rs=\Ec:sc=\E7:st=\EH:up=\EM:\
	:le=^H:bl=^G:cr=^M:it#8:ho=\E[H:nw=\EE:ta=^I:is=\E)0:\
	:li#56:co#180:am:xn:xv:LP:sr=\EM:al=\E[L:AL=\E[%dL:\
	:cs=\E[%i%d;%dr:dl=\E[M:DL=\E[%dM:dc=\E[P:DC=\E[%dP:\
	:im=\E[4h:ei=\E[4l:mi:IC=\E[%d@:ks=\E[?1h\E=:\
	:ke=\E[?1l\E>:vi=\E[?25l:ve=\E[34h\E[?25h:vs=\E[34l:\
	:ti=\E[?1049h:te=\E[?1049l:us=\E[4m:ue=\E[24m:so=\E[3m:\
	:se=\E[23m:mb=\E[5m:md=\E[1m:mh=\E[2m:mr=\E[7m:\
	:me=\E[m:ms:\
	:Co#8:pa#64:AF=\E[3%dm:AB=\E[4%dm:op=\E[39;49m:AX:\
	:vb=\Eg:G0:as=\E(0:ae=\E(B:\
	:ac=\140\140aaffggjjkkllmmnnooppqqrrssttuuvvwwxxyyzz{{||}}~~..--++,,hhII00:\
	:po=\E[5i:pf=\E[4i:Km=\E[M:k0=\E[10~:k1=\EOP:k2=\EOQ:\
	:k3=\EOR:k4=\EOS:k5=\E[15~:k6=\E[17~:k7=\E[18~:\
	:k8=\E[19~:k9=\E[20~:k;=\E[21~:F1=\E[23~:F2=\E[24~:\
	:F3=\E[1;2P:F4=\E[1;2Q:F5=\E[1;2R:F6=\E[1;2S:\
	:F7=\E[15;2~:F8=\E[17;2~:F9=\E[18;2~:FA=\E[19;2~:kb=\x7f:\
	:K2=\EOE:kB=\E[Z:kF=\E[1;2B:kR=\E[1;2A:*4=\E[3;2~:\
	:*7=\E[1;2F:#2=\E[1;2H:#3=\E[2;2~:#4=\E[1;2D:%c=\E[6;2~:\
	:%e=\E[5;2~:%i=\E[1;2C:kh=\E[1~:@1=\E[1~:kH=\E[4~:\
	:@7=\E[4~:kN=\E[6~:kP=\E[5~:kI=\E[2~:kD=\E[3~:ku=\EOA:\
	:kd=\EOB:kr=\EOC:kl=\EOD:km:
GR=sunny@sunbase.org:/home/sunny/repos/Git
PAGER=less -S
NCDU_SHELL=mc
PATH=/home/sunny/bin/Local/sunbase:/home/sunny/bin:/usr/src-other/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/src-other/prg/git/libexec/git-core:/opt/git-annex
MAIL=/home/sunny/mail/inbox
STY=21459.vanl
BC_ENV_ARGS=-l --quiet
LC_COLLATE=C
PP=git@github.com:piratpartiet
PWD=/home/sunny/src/git/src-other/devel/git/git/t
IRCSERVER=irc.homelien.no
MAILDIR=/home/sunny/mail
BMS=sunny@bellmann:/home/sunny/src/git
EDITOR=v
SESS_UUID=,bash_profile/cf9306e4-e476-11e6-ab83-db5caa6d21d3,screen/e9db0984-e476-11e6-b0ae-db5caa6d21d3,ssh-agent/fc7d8b3e-e476-11e6-8155-db5caa6d21d3,
LANG=en_GB.UTF-8
BB=git@bitbucket.org:sunny256
SUUID_LOGDIR=/home/sunny/uuids
TZ=CET
WHOIS_HIDE=1
SLX=svn+ssh://sunny256@svn.skolelinux.no/repos/skolelinux
SVN_SSH=ssh
PS1=\[\033[0m\033[1;33m\]$(date +"%Y-%m-%d %T") \[\033[01;31m\]\u\[\033[1;35m\]@\[\033[01;34m\]\h\[\033[m\]:\[\033[1;32m\]\w\033[1;33m\]$(__git_ps1 " (%s)")\[\033[m\]\n\$ 
GIT_PS1_SHOWUNTRACKEDFILES=1
IRCNAME=sunny
HISTCONTROL=ignoreboth
BM=sunny@bellmann:/home/sunny/repos/Git
NCURSES_NO_UTF8_ACS=1
HOME=/home/sunny
SHLVL=4
IRCNICK=sunny256
LESSCHARSET=utf-8
GIT_PS1_SHOWSTASHSTATE=1
SUUID_EDITOR=vim
LOGNAME=sunny
LESS=-RSX
WINDOW=2
CVS_RSH=ssh
GIT_PS1_SHOWUPSTREAM=verbose
SSH_CONNECTION=37.253.243.58 43994 178.79.142.16 22
PROMPT_COMMAND=history -a
CVSEDITOR=/home/sunny/bin/cvs-editor
TREE_CHARSET=UTF-8
XDG_RUNTIME_DIR=/run/user/1000
SR=svn+ssh://sunny@sunbase.org/home/sunny/repos/Svn
RSYNC_RSH=ssh
LC_TIME=en_DK.UTF-8
HISTTIMEFORMAT=%F %T 
GHH=https://sunny256@github.com/sunny256
_=/usr/local/bin/env


> - cat ../GIT-BUILD-OPTIONS

SHELL_PATH='/bin/sh'
PERL_PATH='/usr/bin/perl'
DIFF='diff'
PYTHON_PATH='/usr/bin/python'
TAR='tar'
NO_CURL=''
NO_EXPAT=''
USE_LIBPCRE=''
NO_PERL=''
NO_PYTHON=''
NO_UNIX_SOCKETS=''
PAGER_ENV='LESS=FRX LV=-c'
DC_SHA1='YesPlease'
NO_GETTEXT=''
GETTEXT_POISON=''


> - if you run that under 'strace -f -o /tmp/strace.out', does the
>   strace.out say anything interesting?

In fact, when running the test under strace, another test fails too, and 
it's also reproducible:

$ strace -f -o /tmp/strace.out ./t1308-config-set.sh
ok 1 - setup default config
[snip]
ok 22 - proper error on non-existent files
not ok 23 - proper error on directory "files"
#
#               echo "Error (-1) reading configuration file a-directory." >expect &&
#               mkdir a-directory &&
#               test_expect_code 2 test-config configset_get_value foo.bar a-directory 2>output &&
#               grep "^warning:" output &&
#               grep "^Error" output >actual &&
#               test_cmp expect actual
#
not ok 24 - proper error on non-accessible files
#
#               chmod -r .git/config &&
#               test_when_finished "chmod +r .git/config" &&
#               echo "Error (-1) reading configuration file .git/config." >expect &&
#               test_expect_code 2 test-config configset_get_value foo.bar .git/config 2>output &&
#               grep "^warning:" output &&
#               grep "^Error" output >actual &&
#               test_cmp expect actual
#
ok 25 - proper error on error in default config files
ok 26 - proper error on error in custom config files
ok 27 - check line errors for malformed values
ok 28 - error on modifying repo config without repo
ok 29 - iteration shows correct origins
# failed 2 among 29 test(s)
1..29
$

I didn't see any red flags in the strace output, but I've put the file 
for download at

  http://sunbase.org/strace.git-v2.13.1-449-g02a2850ad58e.out

Thanks,
Øyvind

N 60.376° E 5.3334°
OpenPGP fingerprint: A006 05D6 E676 B319 55E2  E77E FB0C BEE8 94A5 06E5
6f7b6448-50a2-11e7-8259-db5caa6d21d3

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: t1308-config-set.sh fails on current master

[Jeff King]

On Wed, Jun 14, 2017 at 04:17:40AM +0200, Øyvind A. Holm wrote:

> > Interesting.  I'm not able to reproduce it, but of course that doesn't
> > mean much.
> 
> I'll admit that I have a somewhat special build system, but it's been 
> working great since I created it 7 months ago, and I run the test suite 
> every time I install a new git. I'm using the Makefile located at
> 
>   https://gitlab.com/sunny256/src-other/blob/master/devel/git/Makefile
> 
> It's only doing regular stuff like "make configure", "./configure", etc, 
> but I'm mentioning it in case the Makefile reveals something 
> interesting. The git installation is in a non-standard location, the 
> newest version of git I've installed is for example located under 
> /usr/src-other/pool/git.master.v2.13.1-394-g41dd4330a121/ .

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.

-Peff

Re: t1308-config-set.sh fails on current master

[Jeff King]

On Wed, Jun 14, 2017 at 01:02:15AM -0400, Jeff King wrote:

> On Wed, Jun 14, 2017 at 04:17:40AM +0200, Øyvind A. Holm wrote:
> 
> > > Interesting.  I'm not able to reproduce it, but of course that doesn't
> > > mean much.
> > 
> > I'll admit that I have a somewhat special build system, but it's been 
> > working great since I created it 7 months ago, and I run the test suite 
> > every time I install a new git. I'm using the Makefile located at
> > 
> >   https://gitlab.com/sunny256/src-other/blob/master/devel/git/Makefile
> > 
> > It's only doing regular stuff like "make configure", "./configure", etc, 
> > but I'm mentioning it in case the Makefile reveals something 
> > interesting. The git installation is in a non-standard location, the 
> > newest version of git I've installed is for example located under 
> > /usr/src-other/pool/git.master.v2.13.1-394-g41dd4330a121/ .
> 
> 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:

  FILE *f = fopen(".", "r");
  return f && fread(&c, 1, 1, f);

I.e., it sees that not only do we fopen() a directory, but we actually
read garbage from it. Whereas on Linux, we fopen the file and then the
read gets EISDIR.

So it's not true that FREAD_READS_DIRECTORIES; this is more like
FOPEN_OPENS_DIRECTORIES.

Just looking at how the macro is used, I think we want to handle both
cases the same (by doing an fstat check after fopen). So I think it
would be OK to continue to use FREAD_READS_DIRECTORIES for both cases,
and just fix the configure script. It may be worth updating the macro
name for clarity, though.

-Peff

[PATCH] configure.ac: loosen FREAD_READS_DIRECTORIES test program

[Jeff King]

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@sunbase.org>
Signed-off-by: Jeff King <peff@peff.net>
---
 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

Re: [PATCH] configure.ac: loosen FREAD_READS_DIRECTORIES test program

[Øyvind A. Holm]

[-- Attachment #1: Type: text/plain, Size: 734 bytes --]

On 2017-06-14 01:30:18, Jeff King wrote:
> On Wed, Jun 14, 2017 at 01:15:44AM -0400, Jeff King wrote:
> > 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

Yes, this patch fixes t1308. I also ran the whole test suite with the 
patch, everything succeeds.

Thanks,
Øyvind

N 60.376° E 5.3334°
OpenPGP fingerprint: A006 05D6 E676 B319 55E2  E77E FB0C BEE8 94A5 06E5
17e7451e-50f0-11e7-a287-db5caa6d21d3

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 181 bytes --]