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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 17, 2014, 18:44 UTC
Message-ID: <xmqqwqcfl6jh.fsf@gitster.dls.corp.google.com>
In-Reply-To: <53A07BCE.5010100@web.de>

Jens Lehmann <Jens.Lehmann@web.de> writes:

Show 13 quoted lines

> Am 17.06.2014 00:49, schrieb Junio C Hamano:
>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>>> +		git checkout -b "add_sub1" &&
>>> +		git submodule add ./. sub1 &&
>> 
>> This is not technically wrong per-se, but having the project's
>> history itself as its own submodule *is* something nobody sane would
>> do in the real life.  Do we really have to do it this unusual way?
>
> I agree that this isn't a sane setup for real world usage, but I did
> that because it makes things easier when adding tests for recursive
> submodule update later, as we can then use the same test setup just
> one submodule level deeper.

Hmmm... ok....

Show 12 quoted lines

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

Show 7 quoted lines

>>> +		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

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

> Cool, that's much better. Due to the sometimes missing "sub1" I
> needed to modify it to drop the error and not fail:
>
>   sha1=$(git rev-parse HEAD:sub1 2>/dev/null || true) &&

The "HEAD:sub1" notation does require that the path exists in the specified tree-ish. Even if we tried to express the above in a more carefully written form:

	# We may or may not have sub1 in HEAD
        if "sub1 exists in HEAD"
        then
        	sha1=$(git rev-parse HEAD:sub1)
	else
		sha1= # empty
	fi

we would end up using "git rev-parse HEAD:sub1" to implement "sub1 exists in HEAD" part, so your updated alternative would be the best we could do, I would think.

Show 17 quoted lines

>>> +# 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, I would say, without loosening anything.

	$ test -f RelNotes; echo $?; test -e RelNotes; echo $?
        0
        0
        $ ln -s t tests; test -d tests; echo $?; test -e tests; echo $?
        0
        $ ln -s no-such x; test -f x; echo $?; test -e x; echo $?
        1
        1

Thanks.