[PATCH] mingw: hot-fix t5615

[PATCH] mingw: hot-fix t5615

[Johannes Schindelin]

That test made the incorrect assumption that the path separator character
is always a colon. On Windows, it is a semicolon instead.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/t5615-path-separator-v1
Fetch-It-Via: git fetch https://github.com/dscho/git t5615-path-separator-v1

	This is required, but not sufficient, to fix `master` on Windows.

 t/t5615-alternate-env.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
index 22d9d81..3aeffb6 100755
--- a/t/t5615-alternate-env.sh
+++ b/t/t5615-alternate-env.sh
@@ -37,8 +37,10 @@ test_expect_success 'access alternate via absolute path' '
 	EOF
 '
 
+sep=:
+test_have_prereq !MINGW || sep=\;
 test_expect_success 'access multiple alternates' '
-	check_obj "$(pwd)/one.git/objects:$(pwd)/two.git/objects" <<-EOF
+	check_obj "$(pwd)/one.git/objects$sep$(pwd)/two.git/objects" <<-EOF
 	$one blob
 	$two blob
 	EOF

base-commit: 0538b84027a8aba7e8b805e3ec8fceb3990023e5
-- 
2.10.1.583.g721a9e0

Re: [PATCH] mingw: hot-fix t5615

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> That test made the incorrect assumption that the path separator character
> is always a colon. On Windows, it is a semicolon instead.

Documentation/git.txt says that GIT_ALTERNATE_OBJECT_DIRECTORIES is
separated with ";" on Windows fairly clearly, and we should have
caught that.  

For the upcoming release there is no need for any further tweak on
your fix I am responding to, but in the longer term we would want to
turn this to path_sep=";" (or ":") and define it in the global
t/test-lib.sh, as it is plausible that we may want to prepend or
append to $PATH in the tests and that also needs ";" on Windows, no?

Are there other variables that is a list of paths that we care in
our tests?  I notice GIT_CEILING_DIRECTORIES does not have the
corresponding ": separated (on windows ; separated) list" in its
description in Documentation/git.txt but the documentation may need
to be fixed there as well?

Thanks for a quick fix.  Will apply on jk/alt-odb-cleanup and merge
down.

>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/t5615-path-separator-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git t5615-path-separator-v1
>
> 	This is required, but not sufficient, to fix `master` on Windows.
>
>  t/t5615-alternate-env.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
> index 22d9d81..3aeffb6 100755
> --- a/t/t5615-alternate-env.sh
> +++ b/t/t5615-alternate-env.sh
> @@ -37,8 +37,10 @@ test_expect_success 'access alternate via absolute path' '
>  	EOF
>  '
>  
> +sep=:
> +test_have_prereq !MINGW || sep=\;
>  test_expect_success 'access multiple alternates' '
> -	check_obj "$(pwd)/one.git/objects:$(pwd)/two.git/objects" <<-EOF
> +	check_obj "$(pwd)/one.git/objects$sep$(pwd)/two.git/objects" <<-EOF
>  	$one blob
>  	$two blob
>  	EOF
>
> base-commit: 0538b84027a8aba7e8b805e3ec8fceb3990023e5

Re: [PATCH] mingw: hot-fix t5615

[Johannes Sixt]

Am 11.11.2016 um 18:06 schrieb Junio C Hamano:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> That test made the incorrect assumption that the path separator character
>> is always a colon. On Windows, it is a semicolon instead.
>
> Documentation/git.txt says that GIT_ALTERNATE_OBJECT_DIRECTORIES is
> separated with ";" on Windows fairly clearly, and we should have
> caught that.
>
> For the upcoming release there is no need for any further tweak on
> your fix I am responding to, but in the longer term we would want to
> turn this to path_sep=";" (or ":") and define it in the global
> t/test-lib.sh, as it is plausible that we may want to prepend or
> append to $PATH in the tests and that also needs ";" on Windows, no?
>
> Are there other variables that is a list of paths that we care in
> our tests?  I notice GIT_CEILING_DIRECTORIES does not have the
> corresponding ": separated (on windows ; separated) list" in its
> description in Documentation/git.txt but the documentation may need
> to be fixed there as well?
>
> Thanks for a quick fix.  Will apply on jk/alt-odb-cleanup and merge
> down.

A simpler fix is to use $PWD instead of $(pwd). I'll submit a patch in a 
moment.

-- Hannes

[PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables

[Johannes Sixt]

We have to use $PWD instead of $(pwd) because on Windows the latter
would add a C: style path to bash's Unix-style $PATH variable, which
becomes confused by the colon after the drive letter. ($PWD is a
Unix-style path.)

In the case of GIT_ALTERNATE_OBJECT_DIRECTORIES, bash on Windows
assembles a Unix-style path list with the colon as separators. It
converts the value to a Windows-style path list with the semicolon as
path separator when it forwards the variable to git.exe. The same
confusion happens when bash's original value is contaminated with
Windows style paths.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Am 11.11.2016 um 18:11 schrieb Johannes Sixt:
> Am 11.11.2016 um 18:06 schrieb Junio C Hamano:
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>>> That test made the incorrect assumption that the path separator
>>> character
>>> is always a colon. On Windows, it is a semicolon instead.
>>
>> Documentation/git.txt says that GIT_ALTERNATE_OBJECT_DIRECTORIES is
>> separated with ";" on Windows fairly clearly, and we should have
>> caught that.
>>
>> For the upcoming release there is no need for any further tweak on
>> your fix I am responding to, but in the longer term we would want to
>> turn this to path_sep=";" (or ":") and define it in the global
>> t/test-lib.sh, as it is plausible that we may want to prepend or
>> append to $PATH in the tests and that also needs ";" on Windows, no?

When the MSYS program such as bash invokes a non-MSYS program, it
translates the Unix-style paths in arguments and environment variables
to Windows stlye. We only have to ensure that we inject only Unix-style
paths in these places so as not to confuse the conversion algorithm.
Most of the time, we do not have to worry.

On the other hand, when we write a path to a file that git.exe consumes
or receive a path from git.exe, i.e., when the path travels through
stdout and stdin, no automatic translation happens (which is quite
understandable), and we have do the translation explicitly. An example
for such a case is when we write a .git/info/alternates file via the
shell.

> A simpler fix is to use $PWD instead of $(pwd). I'll submit a patch in a
> moment.

Here it is. I had proposed the t0021 part earlier, but it fell through
the cracks during the temporary maintainer change.

 t/t0021-conversion.sh    | 2 +-
 t/t5615-alternate-env.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 9ff502773d..b93cd44546 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -4,7 +4,7 @@ test_description='blob conversion via gitattributes'
 
 . ./test-lib.sh
 
-TEST_ROOT="$(pwd)"
+TEST_ROOT="$PWD"
 PATH=$TEST_ROOT:$PATH
 
 write_script <<\EOF "$TEST_ROOT/rot13.sh"
diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
index 22d9d8178b..eec4137ca5 100755
--- a/t/t5615-alternate-env.sh
+++ b/t/t5615-alternate-env.sh
@@ -31,14 +31,14 @@ test_expect_success 'objects inaccessible without alternates' '
 '
 
 test_expect_success 'access alternate via absolute path' '
-	check_obj "$(pwd)/one.git/objects" <<-EOF
+	check_obj "$PWD/one.git/objects" <<-EOF
 	$one blob
 	$two missing
 	EOF
 '
 
 test_expect_success 'access multiple alternates' '
-	check_obj "$(pwd)/one.git/objects:$(pwd)/two.git/objects" <<-EOF
+	check_obj "$PWD/one.git/objects:$PWD/two.git/objects" <<-EOF
 	$one blob
 	$two blob
 	EOF
-- 
2.11.0.rc0.55.gd967357

Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables

[Jeff King]

On Fri, Nov 11, 2016 at 06:31:48PM +0100, Johannes Sixt wrote:

> We have to use $PWD instead of $(pwd) because on Windows the latter
> would add a C: style path to bash's Unix-style $PATH variable, which
> becomes confused by the colon after the drive letter. ($PWD is a
> Unix-style path.)
> 
> In the case of GIT_ALTERNATE_OBJECT_DIRECTORIES, bash on Windows
> assembles a Unix-style path list with the colon as separators. It
> converts the value to a Windows-style path list with the semicolon as
> path separator when it forwards the variable to git.exe. The same
> confusion happens when bash's original value is contaminated with
> Windows style paths.

So on reading your original I wondered why you did not need to use the
";", and that explains it. But wow, it's subtle.

I don't know what people typically write in the wild, and if it's worth
actually testing explicitly the ";" case. I'll leave that to Windows
people to debate.

-Peff

Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> We have to use $PWD instead of $(pwd) because on Windows the latter
> would add a C: style path to bash's Unix-style $PATH variable, which
> becomes confused by the colon after the drive letter. ($PWD is a
> Unix-style path.)
>
> In the case of GIT_ALTERNATE_OBJECT_DIRECTORIES, bash on Windows
> assembles a Unix-style path list with the colon as separators. It
> converts the value to a Windows-style path list with the semicolon as
> path separator when it forwards the variable to git.exe. The same
> confusion happens when bash's original value is contaminated with
> Windows style paths.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> Am 11.11.2016 um 18:11 schrieb Johannes Sixt:
>> Am 11.11.2016 um 18:06 schrieb Junio C Hamano:
>>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>> ...
>
> When the MSYS program such as bash invokes a non-MSYS program, it
> translates the Unix-style paths in arguments and environment variables
> to Windows stlye. We only have to ensure that we inject only Unix-style
> paths in these places so as not to confuse the conversion algorithm.
> Most of the time, we do not have to worry.
>
> On the other hand, when we write a path to a file that git.exe consumes
> or receive a path from git.exe, i.e., when the path travels through
> stdout and stdin, no automatic translation happens (which is quite
> understandable), and we have do the translation explicitly. An example
> for such a case is when we write a .git/info/alternates file via the
> shell.
>
>> A simpler fix is to use $PWD instead of $(pwd). I'll submit a patch in a
>> moment.
>
> Here it is. I had proposed the t0021 part earlier, but it fell through
> the cracks during the temporary maintainer change.

Thanks.  Dscho, does this fix both of these issues to you?

>  t/t0021-conversion.sh    | 2 +-
>  t/t5615-alternate-env.sh | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index 9ff502773d..b93cd44546 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -4,7 +4,7 @@ test_description='blob conversion via gitattributes'
>  
>  . ./test-lib.sh
>  
> -TEST_ROOT="$(pwd)"
> +TEST_ROOT="$PWD"
>  PATH=$TEST_ROOT:$PATH
>  
>  write_script <<\EOF "$TEST_ROOT/rot13.sh"
> diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
> index 22d9d8178b..eec4137ca5 100755
> --- a/t/t5615-alternate-env.sh
> +++ b/t/t5615-alternate-env.sh
> @@ -31,14 +31,14 @@ test_expect_success 'objects inaccessible without alternates' '
>  '
>  
>  test_expect_success 'access alternate via absolute path' '
> -	check_obj "$(pwd)/one.git/objects" <<-EOF
> +	check_obj "$PWD/one.git/objects" <<-EOF
>  	$one blob
>  	$two missing
>  	EOF
>  '
>  
>  test_expect_success 'access multiple alternates' '
> -	check_obj "$(pwd)/one.git/objects:$(pwd)/two.git/objects" <<-EOF
> +	check_obj "$PWD/one.git/objects:$PWD/two.git/objects" <<-EOF
>  	$one blob
>  	$two blob
>  	EOF

Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables

[Johannes Schindelin]

Hi Junio,

On Fri, 11 Nov 2016, Junio C Hamano wrote:

> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > We have to use $PWD instead of $(pwd) because on Windows the latter
> > would add a C: style path to bash's Unix-style $PATH variable, which
> > becomes confused by the colon after the drive letter. ($PWD is a
> > Unix-style path.)
> >
> > In the case of GIT_ALTERNATE_OBJECT_DIRECTORIES, bash on Windows
> > assembles a Unix-style path list with the colon as separators. It
> > converts the value to a Windows-style path list with the semicolon as
> > path separator when it forwards the variable to git.exe. The same
> > confusion happens when bash's original value is contaminated with
> > Windows style paths.
> >
> > Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> > ---
> > Am 11.11.2016 um 18:11 schrieb Johannes Sixt:
> >> Am 11.11.2016 um 18:06 schrieb Junio C Hamano:
> >>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >>> ...
> >
> > When the MSYS program such as bash invokes a non-MSYS program, it
> > translates the Unix-style paths in arguments and environment variables
> > to Windows stlye. We only have to ensure that we inject only Unix-style
> > paths in these places so as not to confuse the conversion algorithm.
> > Most of the time, we do not have to worry.
> >
> > On the other hand, when we write a path to a file that git.exe consumes
> > or receive a path from git.exe, i.e., when the path travels through
> > stdout and stdin, no automatic translation happens (which is quite
> > understandable), and we have do the translation explicitly. An example
> > for such a case is when we write a .git/info/alternates file via the
> > shell.
> >
> >> A simpler fix is to use $PWD instead of $(pwd). I'll submit a patch in a
> >> moment.
> >
> > Here it is. I had proposed the t0021 part earlier, but it fell through
> > the cracks during the temporary maintainer change.
> 
> Thanks.  Dscho, does this fix both of these issues to you?

Apparently it does because the CI jobs for `master` and for `next` pass.
The one for `pu` still times out, of course.

Ciao,
Dscho

Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Thanks.  Dscho, does this fix both of these issues to you?
>
> Apparently it does because the CI jobs for `master` and for `next` pass.

OK, thanks for a quick confirmation.

> The one for `pu` still times out, of course.

Earlier you said 'pu' did even not build, but do we know where this
"still times out" comes from?  As long as I don't merge anything
prematurely, which I need to be careful about, it shouldn't impact
the upcoming release, but we'd need to figure it out before moving
things forward post release.

Thanks.

Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables

[Lars Schneider]

> On 13 Nov 2016, at 02:13, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>>> Thanks.  Dscho, does this fix both of these issues to you?
>> 
>> Apparently it does because the CI jobs for `master` and for `next` pass.
> 
> OK, thanks for a quick confirmation.
> 
>> The one for `pu` still times out, of course.
> 
> Earlier you said 'pu' did even not build, but do we know where this
> "still times out" comes from?  As long as I don't merge anything
> prematurely, which I need to be careful about, it shouldn't impact
> the upcoming release, but we'd need to figure it out before moving
> things forward post release.

What is the goal for 'pu'?

(1) Builds clean on all platforms + passes all tests
(2) Builds clean on all platforms
(3) Builds clean on Linux
(4) Just makes new topics easily available to a broader audience

My understanding was always (4) but the discussion above sounds 
more like (1) or (2)?

--

Git 'pu' does not compile on macOS right now:
builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used uninitialized 
whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
...

More info here:
https://api.travis-ci.org/jobs/175417712/log.txt?deansi=true

--

Cheers,
Lars

Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables

[Torsten Bögershausen]

On 14.11.16 10:11, Lars Schneider wrote:
> 
>> On 13 Nov 2016, at 02:13, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>>>> Thanks.  Dscho, does this fix both of these issues to you?
>>>
>>> Apparently it does because the CI jobs for `master` and for `next` pass.
>>
>> OK, thanks for a quick confirmation.
>>
>>> The one for `pu` still times out, of course.
>>
>> Earlier you said 'pu' did even not build, but do we know where this
>> "still times out" comes from?  As long as I don't merge anything
>> prematurely, which I need to be careful about, it shouldn't impact
>> the upcoming release, but we'd need to figure it out before moving
>> things forward post release.
> 
> What is the goal for 'pu'?
> 

> (1) Builds clean on all platforms + passes all tests
Yes
> (2) Builds clean on all platforms
Yes
> (3) Builds clean on Linux
Yes
> (4) Just makes new topics easily available to a broader audience
Yes

> 
> My understanding was always (4) but the discussion above sounds 
> more like (1) or (2)?
All commits should work on all platforms - in the ideal world there is no problem.

From time to time things sneak in, which are not portable.
(in the sense that not all "supported" compile/run tests without breakages)

And if everybody reports breakages and problems found on the pu
branch, there is a good chance that they don't reach next or master.

Does this make sense ?

> 
> --
> 
> Git 'pu' does not compile on macOS right now:
> builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used uninitialized 
> whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
> ...
> 
> More info here:
> https://api.travis-ci.org/jobs/175417712/log.txt?deansi=true
> 
> --
> 
> Cheers,
> Lars
> 

Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables

[Jeff King]

On Mon, Nov 14, 2016 at 05:35:56PM +0100, Torsten Bögershausen wrote:

> > What is the goal for 'pu'?
> 
> > (1) Builds clean on all platforms + passes all tests
> Yes
> > (2) Builds clean on all platforms
> Yes
> > (3) Builds clean on Linux
> Yes
> > (4) Just makes new topics easily available to a broader audience
> Yes

I'd have answered differently, though I think in the end we agree on the
outcome.

I think the only thing that matters is (4). It _usually_ builds and
passes all tests, but not always. But the point is that nobody should
care in particular about "pu". What we care about is whether the
individual topics will build and pass before they are merged to master
(or even "next").

So "pu" is a tool, because you can test all of the topics at once and
find out early if there are any problems. And it's good to investigate
problems there before topics hit next (though they are also often caught
in review, or by people trying the broken topic on their various
platforms, or sometimes Junio even pushes out a known-broken state in pu
and mentions it in "What's cooking").

So yes, it should do all of those things, but we don't necessarily
expect that it will never be broken. That's expected to happen from time
to time, and the purpose of the branch. With respect to Lars' original
point:

> > Git 'pu' does not compile on macOS right now:
> > builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used uninitialized 
> > whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]

The next step is to make sure that the topic author is aware (in this
case, one assumes it's pb/bisect).

Better still is to make a patch that can either be applied on top, or
squashed as appropriate. I know that Ramsay Jones does this, for
example, with some of his sparse-related checks, and I'm pretty sure
from the turnaround-time that he runs it against "pu".

-Peff

Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables

[Ramsay Jones]

On 14/11/16 17:01, Jeff King wrote:
> On Mon, Nov 14, 2016 at 05:35:56PM +0100, Torsten Bögershausen wrote:
> 
>>> Git 'pu' does not compile on macOS right now:
>>> builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used uninitialized 
>>> whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
> 
> The next step is to make sure that the topic author is aware (in this
> case, one assumes it's pb/bisect).

[+cc Pranit]

Yep, I had a quick squint, and it looks like the compiler is correct.
It should be complaining about the 'bad_syn' variable for exactly the
same reason: namely, whenever the if condition is true, the only exit
from that block is via 'goto finish' which bypasses the initialisation
of 'good_syn' and 'bad_syn'.

> Better still is to make a patch that can either be applied on top, or
> squashed as appropriate. 

No patch this time, but it simply requires those variables to be
initialised to NULL in their declarations. :-D

>                            I know that Ramsay Jones does this, for
> example, with some of his sparse-related checks, and I'm pretty sure
> from the turnaround-time that he runs it against "pu".

Yep, the idea being to catch these simple problems before the topic
reaches 'next'.

ATB,
Ramsay Jones

Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

>> On 13 Nov 2016, at 02:13, Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>> Earlier you said 'pu' did even not build, but do we know where this
>> "still times out" comes from?  As long as I don't merge anything
>> prematurely, which I need to be careful about, it shouldn't impact
>> the upcoming release, but we'd need to figure it out before moving
>> things forward post release.
>
> What is the goal for 'pu'?
>
> (1) Builds clean on all platforms + passes all tests
> (2) Builds clean on all platforms
> (3) Builds clean on Linux
> (4) Just makes new topics easily available to a broader audience
>
> My understanding was always (4) but the discussion above sounds 
> more like (1) or (2)?

The purpose of 'pu' is none of the above, but its intended effect
for people other than me is (4).  It is primarily meant as a way for
me to avoid having to go back to the mailing list archive to find
topics that I felt that were potentially interesting but not yet
ready and may want to get commented on later.  When queued on 'pu',
it is not unusual that I haven't really looked at the patches yet,
and it is not surprising if it does not build on any platform.

When queued to 'pu', the reason of the initial "not yet ready"
assessment may not have anything to do with the quality of the
patches but based on the phase of the development (e.g. during a
feature-freeze, it is less efficient use of our time to take new
topics to 'next'), so what was queued on 'pu' may get merged to
'next' without any update, after getting looked at by me or by
other people and deemed to be worth trying out.

Dscho's mention of 'still times out' may be an indiciation that
something unspecified on 'pu' is not ready to be merged to 'next',
but blocking all of 'pu' with a blanket statement is not useful,
and that was where my response comes from.  We need to know more
to say "this particular topic is not ready", so that we can unblock
other innocent topics.

Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables

[Johannes Schindelin]

Hi Junio,

On Mon, 14 Nov 2016, Junio C Hamano wrote:

> Dscho's mention of 'still times out' may be an indiciation that
> something unspecified on 'pu' is not ready to be merged to 'next',
> but blocking all of 'pu' with a blanket statement is not useful,
> and that was where my response comes from.

Until the time when the test suite takes less than the insane three hours
to run, I am afraid that a blanket statement on `pu` is the best I can do.

Ciao,
Johannes

Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables

[Johannes Schindelin]

Hi,

On Tue, 15 Nov 2016, Johannes Schindelin wrote:

> On Mon, 14 Nov 2016, Junio C Hamano wrote:
> 
> > Dscho's mention of 'still times out' may be an indiciation that
> > something unspecified on 'pu' is not ready to be merged to 'next',
> > but blocking all of 'pu' with a blanket statement is not useful,
> > and that was where my response comes from.
> 
> Until the time when the test suite takes less than the insane three hours
> to run, I am afraid that a blanket statement on `pu` is the best I can do.

Well, I should add that the test suite does not take 3 hours to run for
`pu` these days.

It used to time out after four hours until two days ago (I think; I was a
bit too busy with other CI work to pay close attention to the constantly
failing `pu` job, with quite a few failing `next`s and even a couple of
failing `master`s thrown in).

As of two days ago, the test suite takes no time at all. The build already
fails (which makes me wonder why a couple of patch series I contributed
had such a hard time getting into `pu` when they compiled and tested
just fine, whereas some obviously non-building stuff gets into `pu`
already, makes no sense to me).

This is the offending part from last night's build:

-- snipsnap --
2016-11-16T00:31:57.5321220Z copy.c: In function 'copy_dir_1':
2016-11-16T00:31:57.5321220Z copy.c:369:8: error: implicit declaration of function 'lchown' [-Werror=implicit-function-declaration]
2016-11-16T00:31:57.5321220Z     if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
2016-11-16T00:31:57.5321220Z         ^~~~~~
2016-11-16T00:31:57.5321220Z copy.c:391:7: error: implicit declaration of function 'mknod' [-Werror=implicit-function-declaration]
2016-11-16T00:31:57.5321220Z    if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0)
2016-11-16T00:31:57.5321220Z        ^~~~~
2016-11-16T00:31:57.5321220Z copy.c:405:7: error: implicit declaration of function 'utimes' [-Werror=implicit-function-declaration]
2016-11-16T00:31:57.5321220Z    if (utimes(dest, times) < 0)
2016-11-16T00:31:57.5321220Z        ^~~~~~
2016-11-16T00:31:57.5321220Z copy.c:407:7: error: implicit declaration of function 'chown' [-Werror=implicit-function-declaration]
2016-11-16T00:31:57.5321220Z    if (chown(dest, source_stat.st_uid, source_stat.st_gid) < 0) {
2016-11-16T00:31:57.5321220Z        ^~~~~
2016-11-16T00:31:57.7982432Z     CC ctype.o
2016-11-16T00:31:58.1418929Z cc1.exe: all warnings being treated as errors
2016-11-16T00:31:58.6368128Z make: *** [Makefile:1988: copy.o] Error 1

Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> This is the offending part from last night's build:
>
> -- snipsnap --
> 2016-11-16T00:31:57.5321220Z copy.c: In function 'copy_dir_1':
> 2016-11-16T00:31:57.5321220Z copy.c:369:8: error: implicit declaration of function 'lchown' [-Werror=implicit-function-declaration]
> 2016-11-16T00:31:57.5321220Z     if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
> 2016-11-16T00:31:57.5321220Z         ^~~~~~
> 2016-11-16T00:31:57.5321220Z copy.c:391:7: error: implicit declaration of function 'mknod' [-Werror=implicit-function-declaration]
> 2016-11-16T00:31:57.5321220Z    if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0)
> 2016-11-16T00:31:57.5321220Z        ^~~~~
> 2016-11-16T00:31:57.5321220Z copy.c:405:7: error: implicit declaration of function 'utimes' [-Werror=implicit-function-declaration]
> 2016-11-16T00:31:57.5321220Z    if (utimes(dest, times) < 0)
> 2016-11-16T00:31:57.5321220Z        ^~~~~~
> 2016-11-16T00:31:57.5321220Z copy.c:407:7: error: implicit declaration of function 'chown' [-Werror=implicit-function-declaration]
> 2016-11-16T00:31:57.5321220Z    if (chown(dest, source_stat.st_uid, source_stat.st_gid) < 0) {
> 2016-11-16T00:31:57.5321220Z        ^~~~~
> 2016-11-16T00:31:57.7982432Z     CC ctype.o
> 2016-11-16T00:31:58.1418929Z cc1.exe: all warnings being treated as errors
> 2016-11-16T00:31:58.6368128Z make: *** [Makefile:1988: copy.o] Error 1

That looks like a part of the new 'instead of run_command("cp -R"),
let's borrow code from somewhere and do that ourselves' in the
nd/worktree-move topic.

The offending part is this:

+	if (S_ISBLK(source_stat.st_mode) ||
+	    S_ISCHR(source_stat.st_mode) ||
+	    S_ISSOCK(source_stat.st_mode) ||
+	    S_ISFIFO(source_stat.st_mode)) {
+		if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0)
+			return error_errno(_("can't create '%s'"), dest);
+	} else
+		return error(_("unrecognized file '%s' with mode %x"),
+			     source, source_stat.st_mode);

I think all of this is meant to be used to copy what is in the
working tree, and what is in the working tree is meant to be tracked
by Git, none of the four types that triggers mknod() would be
relevant for our purpose.  The simplest and cleanest would be to
make the above to return error("unsupported filetype").

I do not mind run_command("cp -R") and get rid of this code
altogether; that might be a more portable and sensible approach.

Re: [PATCH] mingw: hot-fix t5615

[Jeff King]

On Fri, Nov 11, 2016 at 05:29:33PM +0100, Johannes Schindelin wrote:

> That test made the incorrect assumption that the path separator character
> is always a colon. On Windows, it is a semicolon instead.

Oof, sorry about that. I remember being careful about the ";" while
doing the original alt-odb work, but forgot about it while making the
recent fix in 37a95862c.

The patch itself looks like obviously correct (or at least obviously in
the right direction, since it sounds like more is needed).

-Peff