Re: [PATCH] Fix expected values of setup tests on Windows

Re: [PATCH] Fix expected values of setup tests on Windows

[Nguyen Thai Ngoc Duy]

2010/12/31 Johannes Sixt <j6t@kdbg.org>:
> On Windows, bash stores absolute path names in shell variables in POSIX
> format that begins with a slash, rather than in drive-letter format; such
> a value is converted to the latter format when it is passed to a non-MSYS
> program such as git.

Hmm.. from test-lib.sh:

TEST_DIRECTORY=$(pwd)
test="trash directory.$(basename "$0" .sh)"
TRASH_DIRECTORY="$TEST_DIRECTORY/$test"

I'm just curious how these lines make $TRASH_DIRECTORY in POSIX format, while

here=$(pwd)

in your patch does not. Does bash auto convert value in
TRASH_DIRECTORY="$TE..." line?
-- 
Duy

Re: [PATCH] Fix expected values of setup tests on Windows

[Johannes Sixt]

On Freitag, 31. Dezember 2010, Nguyen Thai Ngoc Duy wrote:
> 2010/12/31 Johannes Sixt <j6t@kdbg.org>:
> > On Windows, bash stores absolute path names in shell variables in POSIX
> > format that begins with a slash, rather than in drive-letter format; such
> > a value is converted to the latter format when it is passed to a non-MSYS
> > program such as git.
>
> Hmm.. from test-lib.sh:
>
> TEST_DIRECTORY=$(pwd)
> test="trash directory.$(basename "$0" .sh)"
> TRASH_DIRECTORY="$TEST_DIRECTORY/$test"
>
> I'm just curious how these lines make $TRASH_DIRECTORY in POSIX format,
> while
>
> here=$(pwd)
>
> in your patch does not. Does bash auto convert value in
> TRASH_DIRECTORY="$TE..." line?

No. When this line is executed:

TEST_DIRECTORY=$(pwd)

$(pwd) still has its default behavior to return the POSIX style path. pwd is 
redefined to pwd -W only later.

I'm hesitant to redefine pwd earlier in test-lib.sh, though, because we would 
have to audit all uses of TEST_DIRECTORY for whether POSIX style paths or 
drive-letter paths are needed.

-- Hannes

Re: [PATCH] Fix expected values of setup tests on Windows

[Jonathan Nieder]

Johannes Sixt wrote:
> On Freitag, 31. Dezember 2010, Nguyen Thai Ngoc Duy wrote:

>> in your patch does not. Does bash auto convert value in
>> TRASH_DIRECTORY="$TE..." line?
>
> No. When this line is executed:
> 
> TEST_DIRECTORY=$(pwd)
> 
> $(pwd) still has its default behavior to return the POSIX style path. pwd is 
> redefined to pwd -W only later.

Would it make sense to change it to

 TEST_DIRECTORY=$PWD

for clarity and robustness against code movement, then?

Re: [PATCH] Fix expected values of setup tests on Windows

[Johannes Sixt]

On Freitag, 31. Dezember 2010, Jonathan Nieder wrote:
> Johannes Sixt wrote:
> > On Freitag, 31. Dezember 2010, Nguyen Thai Ngoc Duy wrote:
> >> in your patch does not. Does bash auto convert value in
> >> TRASH_DIRECTORY="$TE..." line?
> >
> > No. When this line is executed:
> >
> > TEST_DIRECTORY=$(pwd)
> >
> > $(pwd) still has its default behavior to return the POSIX style path. pwd
> > is redefined to pwd -W only later.
>
> Would it make sense to change it to
>
>  TEST_DIRECTORY=$PWD
>
> for clarity and robustness against code movement, then?

Yes, that would make sense.

-- Hannes

Re: [PATCH] Fix expected values of setup tests on Windows

[Nguyen Thai Ngoc Duy]

On Fri, Dec 31, 2010 at 11:11 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> in your patch does not. Does bash auto convert value in
>> TRASH_DIRECTORY="$TE..." line?
>
> No. When this line is executed:
>
> TEST_DIRECTORY=$(pwd)
>
> $(pwd) still has its default behavior to return the POSIX style path. pwd is
> redefined to pwd -W only later.
>
> I'm hesitant to redefine pwd earlier in test-lib.sh, though, because we would
> have to audit all uses of TEST_DIRECTORY for whether POSIX style paths or
> drive-letter paths are needed.

Ah I missed that. Thanks. Ack from me.
-- 
Duy

Re: [PATCH] Fix expected values of setup tests on Windows

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> On Freitag, 31. Dezember 2010, Jonathan Nieder wrote:
>> Johannes Sixt wrote:
>> > On Freitag, 31. Dezember 2010, Nguyen Thai Ngoc Duy wrote:
>> >> in your patch does not. Does bash auto convert value in
>> >> TRASH_DIRECTORY="$TE..." line?
>> >
>> > No. When this line is executed:
>> >
>> > TEST_DIRECTORY=$(pwd)
>> >
>> > $(pwd) still has its default behavior to return the POSIX style path. pwd
>> > is redefined to pwd -W only later.
>>
>> Would it make sense to change it to
>>
>>  TEST_DIRECTORY=$PWD
>>
>> for clarity and robustness against code movement, then?
>
> Yes, that would make sense.

It will be very much appreciated to add a few sentences to clarify this to
"Do's and don'ts" section of t/README if you are re-rolling this.  Thanks.

[PATCH 2/2] t/README: hint about using $(pwd) rather than $PWD in tests

[Johannes Sixt]

From: Johannes Sixt <j6t@kdbg.org>

This adds just a "do it this way" instruction without a lot of explanation,
because the details are too complex to be explained at this point.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/README |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index 90718b4..f98ebb3 100644
--- a/t/README
+++ b/t/README
@@ -283,6 +283,12 @@ Do:
    Tests that are likely to smoke out future regressions are better
    than tests that just inflate the coverage metrics.
 
+ - When a test checks for an absolute path that a git command generated,
+   construct the expected value using $(pwd) rather than $PWD,
+   $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference on
+   Windows, where the shell (MSYS bash) mangles absolute path names.
+   For details, see the commit message of 4114156ae9.
+
 Don't:
 
  - exit() within a <script> part.
-- 
1.7.4.rc1.1258.g84aa

Re: [PATCH 2/2] t/README: hint about using $(pwd) rather than $PWD in tests

[Jonathan Nieder]

Johannes Sixt wrote:

> This adds just a "do it this way" instruction without a lot of explanation,
> because the details are too complex to be explained at this point.

Thanks, looks very useful.

> --- a/t/README
> +++ b/t/README
> @@ -283,6 +283,12 @@ Do:
>     Tests that are likely to smoke out future regressions are better
>     than tests that just inflate the coverage metrics.
>  
> + - When a test checks for an absolute path that a git command generated,
> +   construct the expected value using $(pwd) rather than $PWD,
> +   $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference on
> +   Windows, where the shell (MSYS bash) mangles absolute path names.
> +   For details, see the commit message of 4114156ae9.
> +

Perhaps it is also worth explaining the cases where $PWD is needed?

	By contrast, when a passing a path to git or constructing a URL,
	use $PWD.  It makes a difference on Windows, where

	 - $(pwd) is a Windows-style path such as git might output, and
	 - $PWD is a Unix-style path that the shell (MSYS bash) will
	   mangle before passing to native apps like git.

Re: [PATCH 2/2] t/README: hint about using $(pwd) rather than $PWD in tests

[Johannes Sixt]

Am 1/11/2011 8:54, schrieb Jonathan Nieder:
> Perhaps it is also worth explaining the cases where $PWD is needed?
>
> 	By contrast, when a passing a path to git or constructing a URL,
> 	use $PWD.

The first part of the "or" is not true: you can pass the result of $(pwd)
to a command, and it means the same as $PWD; I would even recommend
against $PWD so that a reader does not have to wonder "why pass $PWD, but
check for $(pwd)?"

The second part I don't know whether it is true: I haven't noticed a
pattern where people did it the wrong way, therefore, I'don't even know
whether $PWD is really *always* required. Do *you* know?

>       It makes a difference on Windows, where
> 
> 	 - $(pwd) is a Windows-style path such as git might output, and
> 	 - $PWD is a Unix-style path that the shell (MSYS bash) will
> 	   mangle before passing to native apps like git.

This information is already included by reference to 4114156ae9.

-- Hannes

Re: [PATCH 2/2] t/README: hint about using $(pwd) rather than $PWD in tests

[Jonathan Nieder]

Johannes Sixt wrote:
> Am 1/11/2011 8:54, schrieb Jonathan Nieder:

>> Perhaps it is also worth explaining the cases where $PWD is needed?
>>
>> 	By contrast, when a passing a path to git or constructing a URL,
>> 	use $PWD.
>
> The first part of the "or" is not true: you can pass the result of $(pwd)
> to a command, and it means the same as $PWD; I would even recommend
> against $PWD so that a reader does not have to wonder "why pass $PWD, but
> check for $(pwd)?"

I _think_ that passing $PWD always gives the right result.  By
contrast, constructions like

	PATH=$(pwd)/bin:$PATH

break iirc.

I suspect that the reader will end up wondering "why does this have to
be so complicated" no matter what.

> The second part I don't know whether it is true: I haven't noticed a
> pattern where people did it the wrong way, therefore, I'don't even know
> whether $PWD is really *always* required. Do *you* know?

24f1136 is one example.  I don't know of any utility that treats

	file://c:/foo/bar/baz

as a URL representing a resource on localhost (and msys bash has no
rewriting rule for it), so in that particular case
(file://$directory), $PWD really does seem to be always required.

>>       It makes a difference on Windows, where
>>
>> 	 - $(pwd) is a Windows-style path such as git might output, and
>> 	 - $PWD is a Unix-style path that the shell (MSYS bash) will
>> 	   mangle before passing to native apps like git.
>
> This information is already included by reference to 4114156ae9.

... but if we can summarize it nicely, we can save the reader a
step, no?

Anyway, what you have already written is useful; clearing up these
details would just be icing on the top.

Re: [PATCH 2/2] t/README: hint about using $(pwd) rather than $PWD in tests

[Johannes Sixt]

Am 1/11/2011 9:37, schrieb Jonathan Nieder:
> I suspect that the reader will end up wondering "why does this have to
> be so complicated" no matter what.

Unfortunately, yes. Therefore, I'd like to keep the paragraph minimal,
focused on how expected values should be constructed, which is where
errors will happen primarily.

>>>       It makes a difference on Windows, where
>>>
>>> 	 - $(pwd) is a Windows-style path such as git might output, and
>>> 	 - $PWD is a Unix-style path that the shell (MSYS bash) will
>>> 	   mangle before passing to native apps like git.
>>
>> This information is already included by reference to 4114156ae9.
> 
> ... but if we can summarize it nicely, we can save the reader a
> step, no?

I don't think so: it's not complete enough. Readers will ask: "So what?"
Digging archives or a three paragraph follow-up explanation on the ML will
be required anyway.

> Anyway, what you have already written is useful; clearing up these
> details would just be icing on the top.

OK, thanks for a review.

-- Hannes

Re: [PATCH 2/2] t/README: hint about using $(pwd) rather than $PWD in tests

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 1/11/2011 9:37, schrieb Jonathan Nieder:
>> I suspect that the reader will end up wondering "why does this have to
>> be so complicated" no matter what.
>
> Unfortunately, yes. Therefore, I'd like to keep the paragraph minimal,
> focused on how expected values should be constructed, which is where
> errors will happen primarily.

I really was hoping for something simple like "never use $PWD" myself,
though X-<.

Anyway, will queue for 1.7.4.  Thanks.