t1308-config-set.sh fails on current master

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

Hi Øyvind,

Øyvind A. Holm wrote:

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

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

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

On 2017-06-13 18:25:35, Jonathan Nieder wrote:

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

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
#	

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=:\
	: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

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=''

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

On Wed, Jun 14, 2017 at 04:17:40AM +0200, Øyvind A. Holm 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.

-Peff

On Wed, Jun 14, 2017 at 01:02:15AM -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:

  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

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

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(-)

On 2017-06-14 01:30:18, Jeff King wrote:

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