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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 14, 2008, 03:54 UTC
Message-ID: <7v4p174diu.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1229201231-12586-1-git-send-email-marcel@oak.homeunix.org>

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

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

Hmm, really...?

Show 17 quoted lines

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

Show 22 quoted lines

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