* [PATCH] mingw: hot-fix t5615 @ 2016-11-11 16:29 Johannes Schindelin 2016-11-11 17:06 ` Junio C Hamano 2016-11-11 18:12 ` [PATCH] mingw: hot-fix t5615 Jeff King 0 siblings, 2 replies; 17+ messages in thread From: Johannes Schindelin @ 2016-11-11 16:29 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] mingw: hot-fix t5615 2016-11-11 16:29 [PATCH] mingw: hot-fix t5615 Johannes Schindelin @ 2016-11-11 17:06 ` Junio C Hamano 2016-11-11 17:11 ` Johannes Sixt 2016-11-11 18:12 ` [PATCH] mingw: hot-fix t5615 Jeff King 1 sibling, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2016-11-11 17:06 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git, Jeff King 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] mingw: hot-fix t5615 2016-11-11 17:06 ` Junio C Hamano @ 2016-11-11 17:11 ` Johannes Sixt 2016-11-11 17:31 ` [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables Johannes Sixt 0 siblings, 1 reply; 17+ messages in thread From: Johannes Sixt @ 2016-11-11 17:11 UTC (permalink / raw) To: Junio C Hamano, Johannes Schindelin; +Cc: git, Jeff King 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables 2016-11-11 17:11 ` Johannes Sixt @ 2016-11-11 17:31 ` Johannes Sixt 2016-11-11 18:16 ` Jeff King 2016-11-11 18:55 ` Junio C Hamano 0 siblings, 2 replies; 17+ messages in thread From: Johannes Sixt @ 2016-11-11 17:31 UTC (permalink / raw) To: Junio C Hamano, Johannes Schindelin; +Cc: git, Jeff King 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables 2016-11-11 17:31 ` [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables Johannes Sixt @ 2016-11-11 18:16 ` Jeff King 2016-11-11 18:55 ` Junio C Hamano 1 sibling, 0 replies; 17+ messages in thread From: Jeff King @ 2016-11-11 18:16 UTC (permalink / raw) To: Johannes Sixt; +Cc: Junio C Hamano, Johannes Schindelin, git 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables 2016-11-11 17:31 ` [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables Johannes Sixt 2016-11-11 18:16 ` Jeff King @ 2016-11-11 18:55 ` Junio C Hamano 2016-11-12 11:40 ` Johannes Schindelin 1 sibling, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2016-11-11 18:55 UTC (permalink / raw) To: Johannes Sixt; +Cc: Johannes Schindelin, git, Jeff King 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables 2016-11-11 18:55 ` Junio C Hamano @ 2016-11-12 11:40 ` Johannes Schindelin 2016-11-13 1:13 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Johannes Schindelin @ 2016-11-12 11:40 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Sixt, git, Jeff King 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables 2016-11-12 11:40 ` Johannes Schindelin @ 2016-11-13 1:13 ` Junio C Hamano 2016-11-14 9:11 ` Lars Schneider 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2016-11-13 1:13 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Johannes Sixt, git, Jeff King 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables 2016-11-13 1:13 ` Junio C Hamano @ 2016-11-14 9:11 ` Lars Schneider 2016-11-14 16:35 ` Torsten Bögershausen 2016-11-14 17:21 ` Junio C Hamano 0 siblings, 2 replies; 17+ messages in thread From: Lars Schneider @ 2016-11-14 9:11 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin, Johannes Sixt, git, Jeff King > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables 2016-11-14 9:11 ` Lars Schneider @ 2016-11-14 16:35 ` Torsten Bögershausen 2016-11-14 17:01 ` Jeff King 2016-11-14 17:21 ` Junio C Hamano 1 sibling, 1 reply; 17+ messages in thread From: Torsten Bögershausen @ 2016-11-14 16:35 UTC (permalink / raw) To: Lars Schneider, Junio C Hamano Cc: Johannes Schindelin, Johannes Sixt, git, Jeff King 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables 2016-11-14 16:35 ` Torsten Bögershausen @ 2016-11-14 17:01 ` Jeff King 2016-11-14 19:45 ` Ramsay Jones 0 siblings, 1 reply; 17+ messages in thread From: Jeff King @ 2016-11-14 17:01 UTC (permalink / raw) To: Torsten Bögershausen Cc: Lars Schneider, Junio C Hamano, Johannes Schindelin, Johannes Sixt, git 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables 2016-11-14 17:01 ` Jeff King @ 2016-11-14 19:45 ` Ramsay Jones 0 siblings, 0 replies; 17+ messages in thread From: Ramsay Jones @ 2016-11-14 19:45 UTC (permalink / raw) To: Jeff King, Torsten Bögershausen Cc: Lars Schneider, Junio C Hamano, Johannes Schindelin, Johannes Sixt, git, pranit.bauva 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables 2016-11-14 9:11 ` Lars Schneider 2016-11-14 16:35 ` Torsten Bögershausen @ 2016-11-14 17:21 ` Junio C Hamano 2016-11-15 16:54 ` Johannes Schindelin 1 sibling, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2016-11-14 17:21 UTC (permalink / raw) To: Lars Schneider; +Cc: Johannes Schindelin, Johannes Sixt, git, Jeff King 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables 2016-11-14 17:21 ` Junio C Hamano @ 2016-11-15 16:54 ` Johannes Schindelin 2016-11-16 9:47 ` Johannes Schindelin 0 siblings, 1 reply; 17+ messages in thread From: Johannes Schindelin @ 2016-11-15 16:54 UTC (permalink / raw) To: Junio C Hamano; +Cc: Lars Schneider, Johannes Sixt, git, Jeff King 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables 2016-11-15 16:54 ` Johannes Schindelin @ 2016-11-16 9:47 ` Johannes Schindelin 2016-11-16 21:51 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Johannes Schindelin @ 2016-11-16 9:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: Lars Schneider, Johannes Sixt, git, Jeff King 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables 2016-11-16 9:47 ` Johannes Schindelin @ 2016-11-16 21:51 ` Junio C Hamano 0 siblings, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2016-11-16 21:51 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy Cc: Johannes Schindelin, Lars Schneider, Johannes Sixt, git, Jeff King 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] mingw: hot-fix t5615 2016-11-11 16:29 [PATCH] mingw: hot-fix t5615 Johannes Schindelin 2016-11-11 17:06 ` Junio C Hamano @ 2016-11-11 18:12 ` Jeff King 1 sibling, 0 replies; 17+ messages in thread From: Jeff King @ 2016-11-11 18:12 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-11-16 21:51 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-11 16:29 [PATCH] mingw: hot-fix t5615 Johannes Schindelin 2016-11-11 17:06 ` Junio C Hamano 2016-11-11 17:11 ` Johannes Sixt 2016-11-11 17:31 ` [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables Johannes Sixt 2016-11-11 18:16 ` Jeff King 2016-11-11 18:55 ` Junio C Hamano 2016-11-12 11:40 ` Johannes Schindelin 2016-11-13 1:13 ` Junio C Hamano 2016-11-14 9:11 ` Lars Schneider 2016-11-14 16:35 ` Torsten Bögershausen 2016-11-14 17:01 ` Jeff King 2016-11-14 19:45 ` Ramsay Jones 2016-11-14 17:21 ` Junio C Hamano 2016-11-15 16:54 ` Johannes Schindelin 2016-11-16 9:47 ` Johannes Schindelin 2016-11-16 21:51 ` Junio C Hamano 2016-11-11 18:12 ` [PATCH] mingw: hot-fix t5615 Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).