Re: [PATCH v3] git-sh-setup: Fix scripts whose PWD is a symlink into a git work-dir

[Junio C Hamano <gitster@pobox.com>]

"Marcel M. Cary" <marcel@oak.homeunix.org> writes:

> I also removed the "pwd -P" from the unit test.

Hmm, really...?

>> > +# Prove that the remote end really is a repo, and other commands
>> > +# work fine in this context.
>> > +#
>> > +test_debug "
>> > +    test_expect_success 'pushing from symlinked subdir' '
>> > +
>> > +        git push
>> > +    '
>> > +"
>> 
>> Why should this be hidden inside test_debug?
>
> I'm not particularly trying to test "git push" or "git pull" in general
> here.  That's also why the other "git pull" was in a test_debug.  I
> thought it was really only useful to someone trying to understand the
> contents of the test file.  There are other files that cover push and
> pull.  Do you think these test cases should run all the time here?

I'd say so.  Your supporting argument could be "See, push works just fine
with this layout, but pull doesn't because it is a shell script that can
be fooled, and this change is to fix the inconsistencies between them."
Having these test enabled would be a good way to do so.  Then it becomes
irrelevant if "jump into the middle of a directory hierarchy sideways via
symlink" is worth supporting or not ;-)

But whether it is inside test_debug or not, the test should check not just
the exit status from 'git push' but also check what happened to the
receiving repository at least to make sure it is pushing to the location
you are expecting it to.

> diff --git a/t/t2300-cd-to-toplevel.sh b/t/t2300-cd-to-toplevel.sh
> new file mode 100755
> index 0000000..05854b4
> --- /dev/null
> +++ b/t/t2300-cd-to-toplevel.sh
> @@ -0,0 +1,37 @@
> +#!/bin/sh
> +
> +test_description='cd_to_toplevel'
> +
> +. ./test-lib.sh
> +
> +test_cd_to_toplevel () {
> +	test_expect_success "$2" '
> +		(
> +			cd '"'$1'"' &&
> +			. git-sh-setup &&
> +			cd_to_toplevel &&
> +			[ "$(pwd -P)" = "$TOPLEVEL" ]
> +		)
> +	'
> +}

The quoting of $1 here is a bit tricky, but I think it is good enough for
directory names used in tests that use this function.