Re: [PATCH 02/14] submodules: Add the lib-submodule-update.sh test library

[Jens Lehmann <Jens.Lehmann@web.de>]

Am 17.06.2014 20:44, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> Am 17.06.2014 00:49, schrieb Junio C Hamano:
>>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>>>> +		GIT_WORK_TREE=. git config --unset core.worktree
>>>
>>> Hmph.  What does GIT_WORK_TREE=. alone without GIT_DIR=<somewhere>
>>> do?  It's not like it is a workaround for "git config" that complains
>>> when you do not have a working tree, right?  Puzzled...
>>
>> It is, it overrides the core.worktree config that would stop us
>> from unsetting the core.worktree config with this error message:
>>
>>   fatal: Could not chdir to '../../../sub1': No such file or directory
>>
>> (We use the same pattern in git-submodule.sh and some other tests)
> 
> Is this a work-around for a bug in "git config"?  Or is this an
> expected failure and it is unusual and not realistic outside of test
> setup to want to unset core.worktree?  I am inclined to think it is
> the latter, but I dunno.

I didn't think deeply about that, but when I first encountered
this behavior it felt a bit strange that config does a chdir into
the work tree. I somehow expected it only to access the config
file in GIT_DIR and not the work tree, but I didn't care enough
to investigate further after I found this solution.

>>>> +		sha1=$(git ls-tree HEAD "sub1" 2>/dev/null | grep 160000 | tr '\t' ' ' | cut -d ' ' -f3) &&
>>>
>>> Why discard the standard error stream?
>>
>> Because we sometimes reset to commits where "sub1" isn't present:
>>
>>   fatal: Path 'sub1' does not exist in 'HEAD'
> 
> Huh?  We shouldn't.
> 
> 	$ git ls-tree HEAD no-such; echo $?
>         0

You are correct, it looks like I used rev-parse instead of
ls-files when producing that error.

> It discards errors that may happen in other situations, too---is
> that something we do not have to worry about?

I'll look deeper into that tomorrow. After all in some tests
"sub1" will be a file and not a submodule, and then we should
not try to populate it ...

>>>> +# Test that the given submodule at path "$1" contains the content according
>>>> +# to the submodule commit recorded in the superproject's commit "$2"
>>>> +test_submodule_content () {
>>>> +	if test $# != 2
>>>> +	then
>>>> +		echo "test_submodule_content needs two arguments"
>>>> +		return 1
>>>> +	fi &&
>>>> +	submodule="$1" &&
>>>> +	commit="$2" &&
>>>> +	test -d "$submodule"/ &&
>>>> +	if ! test -f "$submodule"/.git && ! test -d "$submodule"/.git
>>>
>>> I wonder if we can get away with a single "test -e" (we do not
>>> expect us to be creating device nodes or fifos there, do we?).
>>
>> But a symbolic link maybe?
> 
> Symlinks should pose no problems, ...

Oh, I forgot to add a smiley there, I haven't been serious about
that statement. I don't care too deeply about it but feel a bit
more confident with the two explicit tests. Do you want me to
change them to a single "test -e"?