threads / patch / 26167

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

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

## tl;dr

12 messages between Dec 31, 2010 and Jan 11, 2011. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Nguyen Thai Ngoc Duy· Dec 31, 2010, 13:00 UTC · lore
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
Johannes Sixt· Dec 31, 2010, 16:11 UTC · re: Nguyen Thai Ngoc Duy · lore
On Freitag, 31. Dezember 2010, Nguyen Thai Ngoc Duy wrote:
Show 19 quoted lines
> 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
Jonathan Nieder· Dec 31, 2010, 20:30 UTC · re: Johannes Sixt · lore
Johannes Sixt wrote:
> On Freitag, 31. Dezember 2010, Nguyen Thai Ngoc Duy wrote:
Show 9 quoted lines
>> 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?
Johannes Sixt· Dec 31, 2010, 22:21 UTC · re: Jonathan Nieder · lore
On Freitag, 31. Dezember 2010, Jonathan Nieder wrote:
Show 17 quoted lines
> 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
Junio C Hamano· Jan 2, 2011, 01:31 UTC · re: Johannes Sixt · lore
Johannes Sixt <j6t@kdbg.org> writes:
Show 20 quoted lines
> 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.

Nguyen Thai Ngoc Duy· Jan 1, 2011, 03:46 UTC · re: Johannes Sixt · lore
On Fri, Dec 31, 2010 at 11:11 PM, Johannes Sixt <j6t@kdbg.org> wrote:
Show 13 quoted lines
>> 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
Johannes Sixt· Jan 11, 2011, 07:44 UTC · lore

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

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(-)
Show changes to t/README +6 −0
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
Jonathan Nieder· Jan 11, 2011, 07:54 UTC · re: Johannes Sixt · lore

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

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.
Show 12 quoted lines
> --- 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.
Johannes Sixt· Jan 11, 2011, 08:15 UTC · re: Jonathan Nieder · lore

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

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?

Show 5 quoted lines
>       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
Jonathan Nieder· Jan 11, 2011, 08:37 UTC · re: Johannes Sixt · lore

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

Johannes Sixt wrote:
> Am 1/11/2011 8:54, schrieb Jonathan Nieder:
Show 9 quoted lines
>> 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.

Show 7 quoted lines
>>       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.

Johannes Sixt· Jan 11, 2011, 08:54 UTC · re: Jonathan Nieder · lore

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

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.

Show 10 quoted lines
>>>       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
Junio C Hamano· Jan 11, 2011, 17:47 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 7 quoted lines
> 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.

← back to recent threads