{"thread":{"id":"50388","subject":"[PATCH] doc-diff: don't `cd_to_toplevel` before calling `usage`","startedAt":"2019-02-03T08:37:14Z","lastAt":"2019-02-08T04:34:32Z","messageCount":27,"participants":["Martin Ågren","Eric Sunshine","Jeff King","Johannes Schindelin","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"368430","messageId":"20190203083545.5877-1-martin.agren@gmail.com","threadId":"50388","inReplyTo":null,"subject":"[PATCH] doc-diff: don't `cd_to_toplevel` before calling `usage`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-02-03T08:35:45Z","receivedAt":"2019-02-03T08:37:14Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`usage` tries to call $0, which might very well be \"./doc-diff\", so if\nwe `cd_to_toplevel` before calling `usage`, we'll end with an error to\nthe effect of \"./doc-diff: not found\" rather than a friendly `doc-diff\n-h` output. Granted, all of these `usage` calls are in error paths, so\nwe're about to exit anyway, but the user experience of something like\n`(cd Documentation && ./doc-diff)` could be a bit better than\n\"./doc-diff: not found\".\n\nThis regressed in ad51743007 (\"doc-diff: add --clean mode to remove\ntemporary working gunk\", 2018-08-31) where we moved the call to\n`cd_to_toplevel` to much earlier. Move it back to where it was, and\nteach the \"--clean\" code to cd on its own. This way, we only cd once\nwe've verified the arguments.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Documentation/doc-diff | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/doc-diff b/Documentation/doc-diff\nindex dfd9418778..f820febf8f 100755\n--- a/Documentation/doc-diff\n+++ b/Documentation/doc-diff\n@@ -39,12 +39,11 @@ do\n \tshift\n done\n \n-cd_to_toplevel\n-tmp=Documentation/tmp-doc-diff\n-\n if test -n \"$clean\"\n then\n \ttest $# -eq 0 || usage\n+\tcd_to_toplevel\n+\ttmp=Documentation/tmp-doc-diff\n \tgit worktree remove --force \"$tmp/worktree\" 2>/dev/null\n \trm -rf \"$tmp\"\n \texit 0\n@@ -66,6 +65,9 @@ to=$1; shift\n from_oid=$(git rev-parse --verify \"$from\") || exit 1\n to_oid=$(git rev-parse --verify \"$to\") || exit 1\n \n+cd_to_toplevel\n+tmp=Documentation/tmp-doc-diff\n+\n if test -n \"$force\"\n then\n \trm -rf \"$tmp\"\n-- \n2.20.1.309.g16a465bc01\n\n"},{"id":"368431","messageId":"CAPig+cRN_Jw_vj5=RWZJWp-Wn6-=Vd9oZKQieMWJ02TjAHWwWg@mail.gmail.com","threadId":"50388","inReplyTo":"20190203083545.5877-1-martin.agren@gmail.com","subject":"Re: [PATCH] doc-diff: don't `cd_to_toplevel` before calling `usage`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-03T09:08:56Z","receivedAt":"2019-02-03T09:09:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 3, 2019 at 3:37 AM Martin Ågren <martin.agren@gmail.com> wrote:\n> `usage` tries to call $0, which might very well be \"./doc-diff\", so if\n> we `cd_to_toplevel` before calling `usage`, we'll end with an error to\n> the effect of \"./doc-diff: not found\" rather than a friendly `doc-diff\n> -h` output. Granted, all of these `usage` calls are in error paths, so\n> we're about to exit anyway, but the user experience of something like\n> `(cd Documentation && ./doc-diff)` could be a bit better than\n> \"./doc-diff: not found\".\n>\n> This regressed in ad51743007 (\"doc-diff: add --clean mode to remove\n> temporary working gunk\", 2018-08-31) where we moved the call to\n> `cd_to_toplevel` to much earlier. Move it back to where it was, and\n> teach the \"--clean\" code to cd on its own. This way, we only cd once\n> we've verified the arguments.\n\nThanks for spotting this; I wasn't aware of it when crafting ad51743007.\n\nI wonder if a more fruitful, longer-term fix which would save us from\nhaving to worry about this in the future, would be to make\ngit-sh-setup.sh remember the original $0 before cd_to_toplevel() and\nthen employ the original value when usage() re-execs with the -h\noption. That would also avoid the slightly ugly repeated\ncd_to_top_level() and 'tmp' assignment in this patch.\n\n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n"},{"id":"368432","messageId":"CAPig+cQYuwMmyw8_p863BeSU5RP-xdB=HFMa7EM4G3YALQj3_g@mail.gmail.com","threadId":"50388","inReplyTo":"CAPig+cRN_Jw_vj5=RWZJWp-Wn6-=Vd9oZKQieMWJ02TjAHWwWg@mail.gmail.com","subject":"Re: [PATCH] doc-diff: don't `cd_to_toplevel` before calling `usage`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-03T09:11:50Z","receivedAt":"2019-02-03T09:12:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 3, 2019 at 4:08 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> I wonder if a more fruitful, longer-term fix which would save us from\n> having to worry about this in the future, would be to make\n> git-sh-setup.sh remember the original $0 before cd_to_toplevel() and\n> then employ the original value when usage() re-execs with the -h\n> option. That would also avoid the slightly ugly repeated\n> cd_to_top_level() and 'tmp' assignment in this patch.\n\nBy \"original $0\", I meant a path which would be suitable for\nre-exec'ing (which wouldn't be the literal original $0). Sorry for the\nconfusion.\n"},{"id":"368433","messageId":"CAN0heSohuL=TBOLR+Q9=zrGhZrrjSgfvA=VeT-aA0gr57onAsg@mail.gmail.com","threadId":"50388","inReplyTo":"CAPig+cQYuwMmyw8_p863BeSU5RP-xdB=HFMa7EM4G3YALQj3_g@mail.gmail.com","subject":"Re: [PATCH] doc-diff: don't `cd_to_toplevel` before calling `usage`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-02-03T10:35:43Z","receivedAt":"2019-02-03T10:37:20Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sun, 3 Feb 2019 at 10:12, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Sun, Feb 3, 2019 at 4:08 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > I wonder if a more fruitful, longer-term fix which would save us from\n> > having to worry about this in the future, would be to make\n> > git-sh-setup.sh remember the original $0 before cd_to_toplevel() and\n> > then employ the original value when usage() re-execs with the -h\n> > option. That would also avoid the slightly ugly repeated\n> > cd_to_top_level() and 'tmp' assignment in this patch.\n>\n> By \"original $0\", I meant a path which would be suitable for\n> re-exec'ing (which wouldn't be the literal original $0). Sorry for the\n> confusion.\n\nI thought about this, and I probably should have said something about it\nin the commit message. My uneducated guess was that \"all\" other users are\nin $PATH and aren't being called like `./foobar`, but just `foobar`. Or,\nthey're internal helpers where the caller has already done the grunt\nsetup work, so their cd-ing is a no-op. I could be very wrong.\n\nTo be honest, I wasn't very tempted to risk breaking git-sh-setup\nonly(?) to help a development helper script like this. But let's see if\nI can spend some more time on this...\n\nThe only way I'd know how to do something like this would be with\nreadlink or realpath. Judging by d2addc3b96 (\"t7800: readlink may not be\navailable\", 2016-05-31), we can't count on readlink. And I'd expect that\ncommit to have switched to realpath if THAT were available everywhere.\nThat commit instead goes for \"ls | sed\" and notes that \"[t]his is no\nuniversal readlink replacement but works in the controlled test\nenvironment well enough.\"\n\nOk, so I am not too eager to try and tackle this with fallback\nstrategies and what-not. What would you say if I punted on this? I could\nadd something like this to the commit message:\n\n  A more general fix would be to teach git-sh-setup to save away the\n  absolute path for $0 and then use that, instead. I'm not aware of any\n  portable way of doing that, see, e.g., d2addc3b96 (\"t7800: readlink\n  may not be available\", 2016-05-31), so let's just fix this user\n  instead.\n\nWhat do you think? Thanks for your comments.\n\nMartin\n"},{"id":"368434","messageId":"CAPig+cT32oi5HLe7tokghrCmaiECwAXxON8U7DrpMYb3GCSNyQ@mail.gmail.com","threadId":"50388","inReplyTo":"CAN0heSohuL=TBOLR+Q9=zrGhZrrjSgfvA=VeT-aA0gr57onAsg@mail.gmail.com","subject":"Re: [PATCH] doc-diff: don't `cd_to_toplevel` before calling `usage`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-03T11:01:27Z","receivedAt":"2019-02-03T11:01:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 3, 2019 at 5:37 AM Martin Ågren <martin.agren@gmail.com> wrote:\n> On Sun, 3 Feb 2019 at 10:12, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > On Sun, Feb 3, 2019 at 4:08 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > > I wonder if a more fruitful, longer-term fix which would save us from\n> > > having to worry about this in the future, would be to make\n> > > git-sh-setup.sh remember the original $0 before cd_to_toplevel() and\n> > > then employ the original value when usage() re-execs with the -h\n> > > option. That would also avoid the slightly ugly repeated\n> > > cd_to_top_level() and 'tmp' assignment in this patch.\n> >\n> > By \"original $0\", I meant a path which would be suitable for\n> > re-exec'ing (which wouldn't be the literal original $0). Sorry for the\n> > confusion.\n>\n> Ok, so I am not too eager to try and tackle this with fallback\n> strategies and what-not. What would you say if I punted on this? I could\n> add something like this to the commit message:\n>\n>   A more general fix would be to teach git-sh-setup to save away the\n>   absolute path for $0 and then use that, instead. I'm not aware of any\n>   portable way of doing that, see, e.g., d2addc3b96 (\"t7800: readlink\n>   may not be available\", 2016-05-31), so let's just fix this user\n>   instead.\n>\n> What do you think? Thanks for your comments.\n\nPunting and extending the commit message like that sounds reasonable.\n"},{"id":"368435","messageId":"20190203110817.17242-1-martin.agren@gmail.com","threadId":"50388","inReplyTo":"CAPig+cT32oi5HLe7tokghrCmaiECwAXxON8U7DrpMYb3GCSNyQ@mail.gmail.com","subject":"[PATCH v2] doc-diff: don't `cd_to_toplevel` before calling `usage`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-02-03T11:08:17Z","receivedAt":"2019-02-03T11:08:50Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`usage` tries to call $0, which might very well be \"./doc-diff\", so if\nwe `cd_to_toplevel` before calling `usage`, we'll end with an error to\nthe effect of \"./doc-diff: not found\" rather than a friendly `doc-diff\n-h` output. Granted, all of these `usage` calls are in error paths, so\nwe're about to exit anyway, but the user experience of something like\n`(cd Documentation && ./doc-diff)` could be a bit better than\n\"./doc-diff: not found\".\n\nThis regressed in ad51743007 (\"doc-diff: add --clean mode to remove\ntemporary working gunk\", 2018-08-31) where we moved the call to\n`cd_to_toplevel` to much earlier. Move it back to where it was, and\nteach the \"--clean\" code to cd on its own. This way, we only cd once\nwe've verified the arguments.\n\nA more general fix would be to teach git-sh-setup to save away the\nabsolute path for $0 and then use that, instead. I'm not aware of any\nportable way of doing that, see, e.g., d2addc3b96 (\"t7800: readlink\nmay not be available\", 2016-05-31), so let's just fix this user\ninstead.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n\n > Punting and extending the commit message like that sounds reasonable.\n\n So here's a v2 doing exactly that.\n\n Documentation/doc-diff | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/doc-diff b/Documentation/doc-diff\nindex dfd9418778..f820febf8f 100755\n--- a/Documentation/doc-diff\n+++ b/Documentation/doc-diff\n@@ -39,12 +39,11 @@ do\n \tshift\n done\n \n-cd_to_toplevel\n-tmp=Documentation/tmp-doc-diff\n-\n if test -n \"$clean\"\n then\n \ttest $# -eq 0 || usage\n+\tcd_to_toplevel\n+\ttmp=Documentation/tmp-doc-diff\n \tgit worktree remove --force \"$tmp/worktree\" 2>/dev/null\n \trm -rf \"$tmp\"\n \texit 0\n@@ -66,6 +65,9 @@ to=$1; shift\n from_oid=$(git rev-parse --verify \"$from\") || exit 1\n to_oid=$(git rev-parse --verify \"$to\") || exit 1\n \n+cd_to_toplevel\n+tmp=Documentation/tmp-doc-diff\n+\n if test -n \"$force\"\n then\n \trm -rf \"$tmp\"\n-- \n2.20.1.309.g16a465bc01\n\n"},{"id":"368443","messageId":"20190203230152.GA25608@sigill.intra.peff.net","threadId":"50388","inReplyTo":"20190203110817.17242-1-martin.agren@gmail.com","subject":"Re: [PATCH v2] doc-diff: don't `cd_to_toplevel` before calling `usage`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-03T23:01:53Z","receivedAt":"2019-02-03T23:01:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 03, 2019 at 12:08:17PM +0100, Martin Ågren wrote:\n\n> `usage` tries to call $0, which might very well be \"./doc-diff\", so if\n> we `cd_to_toplevel` before calling `usage`, we'll end with an error to\n> the effect of \"./doc-diff: not found\" rather than a friendly `doc-diff\n> -h` output. Granted, all of these `usage` calls are in error paths, so\n> we're about to exit anyway, but the user experience of something like\n> `(cd Documentation && ./doc-diff)` could be a bit better than\n> \"./doc-diff: not found\".\n> \n> This regressed in ad51743007 (\"doc-diff: add --clean mode to remove\n> temporary working gunk\", 2018-08-31) where we moved the call to\n> `cd_to_toplevel` to much earlier. Move it back to where it was, and\n> teach the \"--clean\" code to cd on its own. This way, we only cd once\n> we've verified the arguments.\n> \n> A more general fix would be to teach git-sh-setup to save away the\n> absolute path for $0 and then use that, instead. I'm not aware of any\n> portable way of doing that, see, e.g., d2addc3b96 (\"t7800: readlink\n> may not be available\", 2016-05-31), so let's just fix this user\n> instead.\n\nThis (and the patch) seems quite reasonable as a fix.\n\nI actually think we could go further and drop the cd_to_toplevel\nentirely. IIRC, the only thing it accomplishes[1] is that we can\nconsistently refer to the tmp directory as Documentation/tmp-doc-diff.\n\nIt would probably be fine to:\n\n  - use \"$(git rev-parse --show-toplevel)/Documentation/tmp-doc-diff\".\n    In earlier iterations of the script, I think using an absolute path\n    bled through to the resulting diff, but we later cleaned that up\n    anyway.\n\n  - just use a relative \"tmp-doc-diff\". I doubt anybody actually wants\n    to do anything other than \"cd Documentation && ./doc-diff\" anyway.\n    This breaks \"./Documentation/doc-diff\", but it is not like you can\n    run \"t/t0000-basic.sh\" either.\n\n-Peff\n\n[1] I think also in early iterations I had some notion of using the cwd\n    to build things, but that quickly went out the window in favor of\n    worktrees.\n"},{"id":"368486","messageId":"20190204205037.32143-1-martin.agren@gmail.com","threadId":"50388","inReplyTo":"20190203230152.GA25608@sigill.intra.peff.net","subject":"[PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-02-04T20:50:37Z","receivedAt":"2019-02-04T20:51:40Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`usage` tries to call $0, which might very well be \"./doc-diff\", so if\nwe `cd_to_toplevel` before calling `usage`, we'll end with an error to\nthe effect of \"./doc-diff: not found\" rather than a friendly `doc-diff\n-h` output. This regressed in ad51743007 (\"doc-diff: add --clean mode to\nremove temporary working gunk\", 2018-08-31) where we moved the call to\n`cd_to_toplevel` to much earlier.\n\nA general fix might be to teach git-sh-setup to save away the absolute\npath for $0 and then use that, instead. I'm not aware of any portable\nway of doing that, see, e.g., d2addc3b96 (\"t7800: readlink may not be\navailable\", 2016-05-31).\n\nAn early version of this patch moved `cd_to_toplevel` back to where it\nwas before ad51743007 and taught the \"--clean\" code to cd on its own.\nBut let's try instead to get rid of the cd-ing entirely. We don't really\nneed it and we can work with absolute paths instead. There's just one\nuse of $PWD that we need to adjust by simply dropping it.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Thanks Peff for the suggestions. Trying not to cd at all seems sane to\n me. That it allows `./Documentation/doc-diff` is a bonus, I guess, but\n you're right that it's probably nothing anyone will use.\n\n I've verified that diffs produced by `./Documentation/doc-diff foo bar`\n and `./doc-diff foo bar` are identical, FWIW.\n\n Martin\n\n Documentation/doc-diff | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/doc-diff b/Documentation/doc-diff\nindex dfd9418778..32c83dd26f 100755\n--- a/Documentation/doc-diff\n+++ b/Documentation/doc-diff\n@@ -39,8 +39,7 @@ do\n \tshift\n done\n \n-cd_to_toplevel\n-tmp=Documentation/tmp-doc-diff\n+tmp=\"$(git rev-parse --show-toplevel)/Documentation/tmp-doc-diff\" || exit 1\n \n if test -n \"$clean\"\n then\n@@ -109,7 +108,7 @@ render_tree () {\n \t\tmake -j$parallel -C \"$tmp/worktree\" \\\n \t\t\tGIT_VERSION=omitted \\\n \t\t\tSOURCE_DATE_EPOCH=0 \\\n-\t\t\tDESTDIR=\"$PWD/$tmp/installed/$1+\" \\\n+\t\t\tDESTDIR=\"$tmp/installed/$1+\" \\\n \t\t\tinstall-man &&\n \t\tmv \"$tmp/installed/$1+\" \"$tmp/installed/$1\"\n \tfi &&\n-- \n2.20.1.390.gb5101f9297\n\n"},{"id":"368498","messageId":"20190204233444.GC2366@sigill.intra.peff.net","threadId":"50388","inReplyTo":"20190204205037.32143-1-martin.agren@gmail.com","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-04T23:34:45Z","receivedAt":"2019-02-04T23:34:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 04, 2019 at 09:50:37PM +0100, Martin Ågren wrote:\n\n> `usage` tries to call $0, which might very well be \"./doc-diff\", so if\n> we `cd_to_toplevel` before calling `usage`, we'll end with an error to\n> the effect of \"./doc-diff: not found\" rather than a friendly `doc-diff\n> -h` output. This regressed in ad51743007 (\"doc-diff: add --clean mode to\n> remove temporary working gunk\", 2018-08-31) where we moved the call to\n> `cd_to_toplevel` to much earlier.\n> \n> A general fix might be to teach git-sh-setup to save away the absolute\n> path for $0 and then use that, instead. I'm not aware of any portable\n> way of doing that, see, e.g., d2addc3b96 (\"t7800: readlink may not be\n> available\", 2016-05-31).\n> \n> An early version of this patch moved `cd_to_toplevel` back to where it\n> was before ad51743007 and taught the \"--clean\" code to cd on its own.\n> But let's try instead to get rid of the cd-ing entirely. We don't really\n> need it and we can work with absolute paths instead. There's just one\n> use of $PWD that we need to adjust by simply dropping it.\n\nThanks, this version looks great to me!\n\n-Peff\n"},{"id":"368516","messageId":"nycvar.QRO.7.76.6.1902051050090.41@tvgsbejvaqbjf.bet","threadId":"50388","inReplyTo":"20190204233444.GC2366@sigill.intra.peff.net","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-05T10:34:54Z","receivedAt":"2019-02-05T10:35:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff and Martin,\n\nOn Tue, 5 Feb 2019, Jeff King wrote:\n\n> On Mon, Feb 04, 2019 at 09:50:37PM +0100, Martin Ågren wrote:\n> \n> > `usage` tries to call $0, which might very well be \"./doc-diff\", so if\n> > we `cd_to_toplevel` before calling `usage`, we'll end with an error to\n> > the effect of \"./doc-diff: not found\" rather than a friendly `doc-diff\n> > -h` output. This regressed in ad51743007 (\"doc-diff: add --clean mode to\n> > remove temporary working gunk\", 2018-08-31) where we moved the call to\n> > `cd_to_toplevel` to much earlier.\n> > \n> > A general fix might be to teach git-sh-setup to save away the absolute\n> > path for $0 and then use that, instead. I'm not aware of any portable\n> > way of doing that, see, e.g., d2addc3b96 (\"t7800: readlink may not be\n> > available\", 2016-05-31).\n> > \n> > An early version of this patch moved `cd_to_toplevel` back to where it\n> > was before ad51743007 and taught the \"--clean\" code to cd on its own.\n> > But let's try instead to get rid of the cd-ing entirely. We don't really\n> > need it and we can work with absolute paths instead. There's just one\n> > use of $PWD that we need to adjust by simply dropping it.\n> \n> Thanks, this version looks great to me!\n\nPeff, you asked at the Contributors' Summit for a way to get notified when\nCI fails for your patch, and I was hesitant to add it (even if it would be\nstraight-forward, really) because of the false positives.\n\nThis is one such example, as the test fails:\n\n\thttps://dev.azure.com/gitgitgadget/git/_build/results?buildId=944\n\nIn particular, the tests t2024 and t5552 are broken for\nma/doc-diff-usage-fix on Windows. The reason seems to be that those are\nalready broken for the base commit that Junio picked:\njk/diff-rendered-docs (actually, not the tip of it, but the commit fixed\nby Martin's patch).\n\nOf course, I understand why Junio picks base commits that are far, far in\nthe past (and have regressions that current `master` does not have), it\nmakes sense from the point of view where the fixes should be as close to\nthe commits they fix.  The downside is that we cannot automate regression\ntesting more than we do now, with e.g. myself acting as curator of test\nfailures.\n\nCiao,\nDscho\nCiao,\n"},{"id":"368536","messageId":"xmqqimxyrs4w.fsf@gitster-ct.c.googlers.com","threadId":"50388","inReplyTo":"nycvar.QRO.7.76.6.1902051050090.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-05T18:45:35Z","receivedAt":"2019-02-05T18:45:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> In particular, the tests t2024 and t5552 are broken for\n> ma/doc-diff-usage-fix on Windows. The reason seems to be that those are\n> already broken for the base commit that Junio picked:\n> jk/diff-rendered-docs (actually, not the tip of it, but the commit fixed\n> by Martin's patch).\n>\n> Of course, I understand why Junio picks base commits that are far, far in\n> the past (and have regressions that current `master` does not have), it\n> makes sense from the point of view where the fixes should be as close to\n> the commits they fix.  The downside is that we cannot automate regression\n> testing more than we do now, with e.g. myself acting as curator of test\n> failures.\n\nI think by \"regressions that current master does not have\", you\nactually meant \"unrelated known breakages we have fixed since then\".\n\nIf you use topic-branch workflow, it is unavoidable and expected, as\neach topic has its focus and the topic to fix doc-diff are not about\nmaking checkout test or fetch negotiator test to pass on all\nplatforms.\n\nI am wondering if the automated testing can be made more useful by\nlimiting the scope of testing if it is run on individual topic.  For\nfour primary integration branches, we do want to ensure all tests\nkeep passing (or at least no tests that passed in the previous round\nstart failing), but it would match the topic-branch workflow better\nif the automated testing allowed an individual topic to rely on\nunrelated known breakages to be dealt with by other topics and newer\nintegration branches.\n\nFor a topic like doc-diff that is primarily meant for developers and\ndocumenters, it does not matter much, but for an old but important\nbug, forking the topic to fix it at a point close to the origin is\ncrucial---that is what would allow people to merge the fix to an\nolder maintenance track, without cherry-picking.  It is especially\ntrue when the bug being fixed is more severe than unrelated\nbreakages that have been fixed since then.\n\nPossible approaches I considered and rejected are:\n\n - Perhaps find updated tests in the topic and run only those?  We\n   can complain a topic that is meant as a \"fix\" to something if it\n   does not have test while trying to find such tests ;-)  But this\n   does not work if a \"fix\" has unintended consequences and breaks\n   existing tests that do not have apparent relation known to the\n   author of the patch.\n\n - Perhaps find the fork point, run tests to find known breakages\n   and exclude them?  This would simply be not practical, as it\n   doubles the number of tests run, for individual topic branches\n   because there are an order of magnitude more of them than the\n   primary integration branches.\n\nPossibly, if we limit the fork point to tagged releases, the latter\napproach might turn out to be doable.  For ma/doc-diff-usage-fix,\nthe base commit ad51743007 was v2.20.0-rc0~232, so we could round up\nand pick v2.20.0 as the base instead (the purpose of picking an older\npoint is to allow merging to older maintenance track, and it is\nunlikely that people would keep using v2.20.0-rc0 as the base of\ntheir forked distribution).  Then if the automated testing keeps\nrecord of known breakages (whether they are later fixed or still\nbroken) for these tagged releases, testing a new topic forked from\none of them and deciding not to worry about breakages of tests that\nare known to be broken (or perhaps deciding to skip these tests in\nthe first place) may become feasible.\n\nI dunno.  Limiting fork points to tagged releases for topics that\nare meant for the maintenance tracks may have other benefits by\nrestricting the shape of the resulting dag, so I am willing to try\nit as an experiment, whether it would help your workflow or not.\n\n\n"},{"id":"368585","messageId":"nycvar.QRO.7.76.6.1902061303570.41@tvgsbejvaqbjf.bet","threadId":"50388","inReplyTo":"xmqqimxyrs4w.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-06T12:20:47Z","receivedAt":"2019-02-06T12:21:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 5 Feb 2019, Junio C Hamano wrote:\n\n> I am wondering if the automated testing can be made more useful by\n> limiting the scope of testing if it is run on individual topic.  For\n> four primary integration branches, we do want to ensure all tests keep\n> passing (or at least no tests that passed in the previous round start\n> failing), but it would match the topic-branch workflow better if the\n> automated testing allowed an individual topic to rely on unrelated known\n> breakages to be dealt with by other topics and newer integration\n> branches.\n\nIt is a known technique to use code coverage to inform automated test runs\nwhich tests (or test cases) to run, but our code base is not yet\nconducive to that approach: we use too many scripts (try to determine code\ncoverage for scripts, will ya?), and you cannot really cherry-pick which\ntest cases to run: despite `test-lib.sh` offering the `--run=<range>`\noption, our test cases frequently rely on side effects by previous test\ncases in the same script.\n\nSo that obvious strategy simply is unavailable to us, at least at the\nmoment.\n\n> For a topic like doc-diff that is primarily meant for developers and\n> documenters, it does not matter much, but for an old but important bug,\n> forking the topic to fix it at a point close to the origin is\n> crucial---that is what would allow people to merge the fix to an older\n> maintenance track, without cherry-picking.  It is especially true when\n> the bug being fixed is more severe than unrelated breakages that have\n> been fixed since then.\n\nAs I said, I understand your reasoning.\n\n> Possible approaches I considered and rejected are:\n> \n>  - Perhaps find updated tests in the topic and run only those?  We\n>    can complain a topic that is meant as a \"fix\" to something if it\n>    does not have test while trying to find such tests ;-)  But this\n>    does not work if a \"fix\" has unintended consequences and breaks\n>    existing tests that do not have apparent relation known to the\n>    author of the patch.\n\nIndeed, and the test suite tries to catch exactly those unintended\nconsequences.\n\n>  - Perhaps find the fork point, run tests to find known breakages\n>    and exclude them?  This would simply be not practical, as it\n>    doubles the number of tests run, for individual topic branches\n>    because there are an order of magnitude more of them than the\n>    primary integration branches.\n\nI saw another strategy in action: accept the base commit chosen by the\ncontributor, and ask to back-port it to previous, still supported versions\n(unless an automated rebase managed to back-port already).\n\n> Possibly, if we limit the fork point to tagged releases, the latter\n> approach might turn out to be doable.  For ma/doc-diff-usage-fix,\n> the base commit ad51743007 was v2.20.0-rc0~232, so we could round up\n> and pick v2.20.0 as the base instead (the purpose of picking an older\n> point is to allow merging to older maintenance track, and it is\n> unlikely that people would keep using v2.20.0-rc0 as the base of\n> their forked distribution).  Then if the automated testing keeps\n> record of known breakages (whether they are later fixed or still\n> broken) for these tagged releases, testing a new topic forked from\n> one of them and deciding not to worry about breakages of tests that\n> are known to be broken (or perhaps deciding to skip these tests in\n> the first place) may become feasible.\n\nI kind of do that already in the CI builds of GitGitGadget, but not for\nall known fixed bugs.\n\nThe most prominent example is the lack of Windows Vista support in\ncurrent the Git for Windows SDK: a *ton* of your branches does not have\nthat fix, and I still wanted to have test coverage for those branches, so\nI introduced this snippet:\n\n  git grep _WIN32_WINNT.0x06 HEAD -- git-compat-util.h ||\n  export MAKEFLAGS=CFLAGS=-D_WIN32_WINNT=0x0502\n\nThis will let those branches build.\n\nLikewise, I added CI builds for all of your branches in gitster/git to\nmake it much easier to *act* on regression test failures.\n\nAs you can see, I am more than willing to pour an insane amount of time\njust for the sake of not forcing you to change your work style, as it\nseems to work for you and I would be the last person to ask anyone to\nchange anything that works for them.\n\nIn this instance, Peff remarked at the Contributors' Summit (BTW will you\nattend any of those in the future?) that he would love to have \"an\nautomated Ramsay\", i.e. a bot that reacts on test failures, notifies the\nauthor of the root cause, preferably with a patch. And all I thought when\nI sent the mail that started this here sub-thread was that I could\ndemonstrate to Peff that it is not that easy (even if writing that script\nthat would determine the patch author upon a failure would be easy, now\neven the bisect would be easy because we are working on linear topic\nbranches where you do not have to test 10,000 merge bases before testing\nthe 2 patches that could be the culprit).\n\nThank you, of course, for detailing your thoughts on that matter.\n\n> I dunno.  Limiting fork points to tagged releases for topics that\n> are meant for the maintenance tracks may have other benefits by\n> restricting the shape of the resulting dag, so I am willing to try\n> it as an experiment, whether it would help your workflow or not.\n\nIf you would like to experiment that, I'd like to learn the outcome. But I\ndo not strictly think that it is necessary.\n\nCiao,\nDscho\n"},{"id":"368613","messageId":"xmqqsgx0rfrq.fsf@gitster-ct.c.googlers.com","threadId":"50388","inReplyTo":"nycvar.QRO.7.76.6.1902061303570.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-06T17:24:57Z","receivedAt":"2019-02-06T17:25:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> For a topic like doc-diff that is primarily meant for developers and\n>> documenters, it does not matter much, but for an old but important bug,\n>> forking the topic to fix it at a point close to the origin is\n>> crucial---that is what would allow people to merge the fix to an older\n>> maintenance track, without cherry-picking.  It is especially true when\n>> the bug being fixed is more severe than unrelated breakages that have\n>> been fixed since then.\n>\n> As I said, I understand your reasoning.\n\nWhen I am following-up to the mailing list, not replying directly\nand solely to you, I may write things that I know you already know\nto help others reading from sidelines.  Just saying I agree, without\nsounding as if saying \"you do not have to say that\", would be better.\n\n>>  - Perhaps find the fork point, run tests to find known breakages\n>>    and exclude them?  This would simply be not practical, as it\n>>    doubles the number of tests run, for individual topic branches\n>>    because there are an order of magnitude more of them than the\n>>    primary integration branches.\n>\n> I saw another strategy in action: accept the base commit chosen by the\n> contributor, and ask to back-port it to previous, still supported versions\n> (unless an automated rebase managed to back-port already).\n\nIt sounds more like \"notice the base commit chosen by the\ncontributor, reject the series and ask to rebase on a fork point of\nmy choice\".  That's not all that different from \"notice the base\ncommit chosen by the contributor, rebase on a more sensible fork\npoint myself, and double-check the result by merging it to the base\ncommit chosen by the contributor and make sure there is no\nunmanageable conflict\".\n"},{"id":"368627","messageId":"20190206184903.GC10231@sigill.intra.peff.net","threadId":"50388","inReplyTo":"nycvar.QRO.7.76.6.1902051050090.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-06T18:49:03Z","receivedAt":"2019-02-06T18:49:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 05, 2019 at 11:34:54AM +0100, Johannes Schindelin wrote:\n\n> Peff, you asked at the Contributors' Summit for a way to get notified when\n> CI fails for your patch, and I was hesitant to add it (even if it would be\n> straight-forward, really) because of the false positives.\n> \n> This is one such example, as the test fails:\n> \n> \thttps://dev.azure.com/gitgitgadget/git/_build/results?buildId=944\n> \n> In particular, the tests t2024 and t5552 are broken for\n> ma/doc-diff-usage-fix on Windows. The reason seems to be that those are\n> already broken for the base commit that Junio picked:\n> jk/diff-rendered-docs (actually, not the tip of it, but the commit fixed\n> by Martin's patch).\n> \n> Of course, I understand why Junio picks base commits that are far, far in\n> the past (and have regressions that current `master` does not have), it\n> makes sense from the point of view where the fixes should be as close to\n> the commits they fix.  The downside is that we cannot automate regression\n> testing more than we do now, with e.g. myself acting as curator of test\n> failures.\n\nThanks for this real-world example; it's definitely something I hadn't\nthought too hard about.\n\nI think this is a specific case of more general problems with testing.\nWe strive to have every commit pass all of the tests, so that people\nbuilding on top have a known-good base. But there are many reasons that\nmay fail in practice (not exhaustive, but just things I've personally\nseen):\n\n  - some tests are flaky, and will fail intermittently\n\n  - some tests _used_ to pass, but no longer do if the environment has\n    changed (e.g., relying on behavior of system tools that change)\n\n  - historical mistakes, where \"all tests pass\" was only true on one\n    platform but not others (which I think is what's going on here)\n\nAnd these are a problem not just for automated CI, but for running \"make\ntest\" locally. I don't think we'll ever get 100% accuracy, so at some\npoint we have to accept some annoying false positives. The question is\nhow often they come up, and whether they drown out real problems.\nTesting under Linux, my experience with the first two is that they're\nuncommon enough not to be a huge burden.\n\nThe third class seems like it is likely to be a lot more common for\nWindows builds, since we haven't historically run tests on them. But it\nwould also in theory be a thing that would get better going forward, as\nwe fix all of those test failures (and new commits are less likely to be\nbuilt on truly ancient history).\n\nSo IMHO this isn't really a show-stopper problem, so much as something\nthat is a sign of the maturing test/CI setup (I say \"maturing\", not\n\"mature\", as it seems we've probably still got a ways to go). As far as\nnotifications go, it probably makes sense for them to be something that\nrequires the user to sign up for anyway, so at that point they're making\ntheir own choice about whether the signal to noise ratio is acceptable.\n\nI also think there are ways to automate away some of these problems\n(e.g., flake detection by repeating test failures, re-running failures\non parent commits to check whether a patch actually introduced the\nproblem). But implementing those is non-trivial work, so I am certainly\nnot asking you to do it.\n\n-Peff\n"},{"id":"368628","messageId":"20190206185520.GD10231@sigill.intra.peff.net","threadId":"50388","inReplyTo":"xmqqimxyrs4w.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-06T18:55:20Z","receivedAt":"2019-02-06T18:55:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 05, 2019 at 10:45:35AM -0800, Junio C Hamano wrote:\n\n>  - Perhaps find the fork point, run tests to find known breakages\n>    and exclude them?  This would simply be not practical, as it\n>    doubles the number of tests run, for individual topic branches\n>    because there are an order of magnitude more of them than the\n>    primary integration branches.\n\nI think this can be limited to the tests that failed, which makes things\nmuch faster. I.e., we run the tests at the tip of topic X and see that\nt1234 fails. We then go back to the fork point and we just need to run\nt1234 again. If it succeeds, then we blame X for the failure. If it\nfails, then we consider it a false positive.\n\nYou do pay the price to do a full build at the fork point, but in my\nexperience that is much quicker than the tests.\n\n-Peff\n"},{"id":"368708","messageId":"nycvar.QRO.7.76.6.1902071518340.41@tvgsbejvaqbjf.bet","threadId":"50388","inReplyTo":"20190206184903.GC10231@sigill.intra.peff.net","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-07T14:26:21Z","receivedAt":"2019-02-07T14:26:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 6 Feb 2019, Jeff King wrote:\n\n> On Tue, Feb 05, 2019 at 11:34:54AM +0100, Johannes Schindelin wrote:\n> \n> > Peff, you asked at the Contributors' Summit for a way to get notified\n> > when CI fails for your patch, and I was hesitant to add it (even if it\n> > would be straight-forward, really) because of the false positives.\n> > \n> > This is one such example, as the test fails:\n> > \n> > \thttps://dev.azure.com/gitgitgadget/git/_build/results?buildId=944\n> > \n> > In particular, the tests t2024 and t5552 are broken for\n> > ma/doc-diff-usage-fix on Windows. The reason seems to be that those are\n> > already broken for the base commit that Junio picked:\n> > jk/diff-rendered-docs (actually, not the tip of it, but the commit fixed\n> > by Martin's patch).\n> > \n> > Of course, I understand why Junio picks base commits that are far, far in\n> > the past (and have regressions that current `master` does not have), it\n> > makes sense from the point of view where the fixes should be as close to\n> > the commits they fix.  The downside is that we cannot automate regression\n> > testing more than we do now, with e.g. myself acting as curator of test\n> > failures.\n> \n> Thanks for this real-world example; it's definitely something I hadn't\n> thought too hard about.\n> \n> I think this is a specific case of more general problems with testing.\n> We strive to have every commit pass all of the tests, so that people\n> building on top have a known-good base. But there are many reasons that\n> may fail in practice (not exhaustive, but just things I've personally\n> seen):\n> \n>   - some tests are flaky, and will fail intermittently\n> \n>   - some tests _used_ to pass, but no longer do if the environment has\n>     changed (e.g., relying on behavior of system tools that change)\n> \n>   - historical mistakes, where \"all tests pass\" was only true on one\n>     platform but not others (which I think is what's going on here)\n> \n> And these are a problem not just for automated CI, but for running \"make\n> test\" locally. I don't think we'll ever get 100% accuracy, so at some\n> point we have to accept some annoying false positives. The question is\n> how often they come up, and whether they drown out real problems.\n> Testing under Linux, my experience with the first two is that they're\n> uncommon enough not to be a huge burden.\n> \n> The third class seems like it is likely to be a lot more common for\n> Windows builds, since we haven't historically run tests on them. But it\n> would also in theory be a thing that would get better going forward, as\n> we fix all of those test failures (and new commits are less likely to be\n> built on truly ancient history).\n\nIndeed, you are absolutely right: things *are* getting better.\n\nTo me, a big difference is the recent speed-up, making it possible for me\nto pay more attention to individual branches (which are now tested, too),\nand if I see any particular breakage in `pu` or `jch` that I saw already\nin the individual branches, I won't bother digging further.\n\nThat is already a *huge* relief for me.\n\nGranted, I had originally hoped to speed up the test suite, so that it\nwould be faster locally. But I can use the cloud as my computer, too.\n\n> So IMHO this isn't really a show-stopper problem, so much as something\n> that is a sign of the maturing test/CI setup (I say \"maturing\", not\n> \"mature\", as it seems we've probably still got a ways to go). As far as\n> notifications go, it probably makes sense for them to be something that\n> requires the user to sign up for anyway, so at that point they're making\n> their own choice about whether the signal to noise ratio is acceptable.\n\nMaybe. I do not even know whether there is an option for that in Azure\nPipelines, maybe GitHub offers that?\n\nIn any case, I just wanted to corroborate with a real-world example what I\nmentioned at the Contributors' Summit: that I would like to not script\nthat thing yet where contributors are automatically notified when their\nbranches don't pass.\n\n> I also think there are ways to automate away some of these problems\n> (e.g., flake detection by repeating test failures, re-running failures\n> on parent commits to check whether a patch actually introduced the\n> problem). But implementing those is non-trivial work, so I am certainly\n> not asking you to do it.\n\nIndeed. It might be a lot more common than just Git, too, in which case I\nmight catch the interest of some of my colleagues who could then implement\na solid solution that works not only for us, but for everybody using Azure\nPipelines.\n\nSpeaking of which... can we hook it up with https://github.com/git/git,\nnow that the Azure Pipelines support is in `master`? I sent you and Junio\nan invitation to https://dev.azure.com/git/git, so that either you or\nJunio (who are the only owners of the GitHub repository) can set it up. If\nyou want me to help, please do not hesitate to ping me on IRC.\n\nCiao,\nDscho\n"},{"id":"368709","messageId":"nycvar.QRO.7.76.6.1902071632280.41@tvgsbejvaqbjf.bet","threadId":"50388","inReplyTo":"20190206185520.GD10231@sigill.intra.peff.net","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-07T15:41:57Z","receivedAt":"2019-02-07T15:42:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 6 Feb 2019, Jeff King wrote:\n\n> On Tue, Feb 05, 2019 at 10:45:35AM -0800, Junio C Hamano wrote:\n> \n> >  - Perhaps find the fork point, run tests to find known breakages\n> >    and exclude them?  This would simply be not practical, as it\n> >    doubles the number of tests run, for individual topic branches\n> >    because there are an order of magnitude more of them than the\n> >    primary integration branches.\n> \n> I think this can be limited to the tests that failed, which makes things\n> much faster. I.e., we run the tests at the tip of topic X and see that\n> t1234 fails. We then go back to the fork point and we just need to run\n> t1234 again. If it succeeds, then we blame X for the failure. If it\n> fails, then we consider it a false positive.\n\nIf you mean merge bases by fork points, I wrote an Azure Pipeline to do\nthat (so that I could use the cloud as kind of a fast computer), but that\nwas still too slow.\n\nEven when there are even only as much as 12 merge bases to test (which is\nthe current number of merge bases between `next` and `pu`), a build takes\nroughly 6 minutes on Windows, and many tests take 1 minute or more to run\n(offenders like t7003 and t7610 take over 400 seconds, i.e. roughly 6\nminutes), we are talking about roughly 1.5h *just* to test the merge\nbases.\n\nAnd I sadly have to report that that's not the end of it. Back when I\nimplemented the automatic bisect after failed builds (for details, see\nhttps://github.com/git-for-windows/build-extra/commit/c7e01e82c), I had to\nturn it off real quickly because the dumb bisect between `next` and `pu`\nregularly ran into the 4h timeout.\n\nCiao,\nDscho\n\n> You do pay the price to do a full build at the fork point, but in my\n> experience that is much quicker than the tests.\n> \n> -Peff\n> \n"},{"id":"368712","messageId":"xmqqy36rpki1.fsf@gitster-ct.c.googlers.com","threadId":"50388","inReplyTo":"nycvar.QRO.7.76.6.1902071632280.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-07T17:37:58Z","receivedAt":"2019-02-07T17:38:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Even when there are even only as much as 12 merge bases to test (which is\n> the current number of merge bases between `next` and `pu`),...\n> ...\n> And I sadly have to report that that's not the end of it. Back when I\n> implemented the automatic bisect after failed builds (for details, see\n> https://github.com/git-for-windows/build-extra/commit/c7e01e82c), I had to\n> turn it off real quickly because the dumb bisect between `next` and `pu`\n> regularly ran into the 4h timeout.\n\nWould it make it easier for you if you substituted all the mention\nof 'next' in your message with 'pu^{/^### match next}'?  \n\nThat mid-point between 'master' and 'pu' is designed to contain\nexactly the same set of non-merge commits 'next' has, with the tree\nthat is identical to that of 'next', and from there to the tip of\n'pu' forms a single strand of merges of tips of topic branches that\nare not yet merged to 'next' (by definition, it itself is the merge\nbase of it and 'pu').\n\nBisecting along the first-parent chain from there to the tip of 'pu'\nwould let us identify which merge is faulty as the first-and-quick\npass and currently there are about 20 merges in that range on the\nfirst-parent chain.\n\n"},{"id":"368735","messageId":"20190207204502.GA28893@sigill.intra.peff.net","threadId":"50388","inReplyTo":"nycvar.QRO.7.76.6.1902071518340.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-07T20:45:02Z","receivedAt":"2019-02-07T20:45:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 07, 2019 at 03:26:21PM +0100, Johannes Schindelin wrote:\n\n> > So IMHO this isn't really a show-stopper problem, so much as something\n> > that is a sign of the maturing test/CI setup (I say \"maturing\", not\n> > \"mature\", as it seems we've probably still got a ways to go). As far as\n> > notifications go, it probably makes sense for them to be something that\n> > requires the user to sign up for anyway, so at that point they're making\n> > their own choice about whether the signal to noise ratio is acceptable.\n> \n> Maybe. I do not even know whether there is an option for that in Azure\n> Pipelines, maybe GitHub offers that?\n\nNo, I don't think so. Probably the route there would be to make a\ncomment on the commit or PR that would then go to the user as a\nnotification (from which they can then decide on email delivery, etc).\n\n> In any case, I just wanted to corroborate with a real-world example what I\n> mentioned at the Contributors' Summit: that I would like to not script\n> that thing yet where contributors are automatically notified when their\n> branches don't pass.\n\nFair enough. As an alternative, do you know offhand if there's an easy\nmachine-readable way to get the CI results? If I could poll it with curl\nand generate my own notifications, that would be fine for me.\n\n> > I also think there are ways to automate away some of these problems\n> > (e.g., flake detection by repeating test failures, re-running failures\n> > on parent commits to check whether a patch actually introduced the\n> > problem). But implementing those is non-trivial work, so I am certainly\n> > not asking you to do it.\n> \n> Indeed. It might be a lot more common than just Git, too, in which case I\n> might catch the interest of some of my colleagues who could then implement\n> a solid solution that works not only for us, but for everybody using Azure\n> Pipelines.\n\nYes, agreed. :)\n\n> Speaking of which... can we hook it up with https://github.com/git/git,\n> now that the Azure Pipelines support is in `master`? I sent you and Junio\n> an invitation to https://dev.azure.com/git/git, so that either you or\n> Junio (who are the only owners of the GitHub repository) can set it up. If\n> you want me to help, please do not hesitate to ping me on IRC.\n\nI'm happy to. I walked through the Azure setup/login procedure, but I'm\nnot sure what to do next.\n\n-Peff\n"},{"id":"368736","messageId":"20190207204910.GB28893@sigill.intra.peff.net","threadId":"50388","inReplyTo":"nycvar.QRO.7.76.6.1902071632280.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-07T20:49:10Z","receivedAt":"2019-02-07T20:49:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 07, 2019 at 04:41:57PM +0100, Johannes Schindelin wrote:\n\n> > I think this can be limited to the tests that failed, which makes things\n> > much faster. I.e., we run the tests at the tip of topic X and see that\n> > t1234 fails. We then go back to the fork point and we just need to run\n> > t1234 again. If it succeeds, then we blame X for the failure. If it\n> > fails, then we consider it a false positive.\n> \n> If you mean merge bases by fork points, I wrote an Azure Pipeline to do\n> that (so that I could use the cloud as kind of a fast computer), but that\n> was still too slow.\n> \n> Even when there are even only as much as 12 merge bases to test (which is\n> the current number of merge bases between `next` and `pu`), a build takes\n> roughly 6 minutes on Windows, and many tests take 1 minute or more to run\n> (offenders like t7003 and t7610 take over 400 seconds, i.e. roughly 6\n> minutes), we are talking about roughly 1.5h *just* to test the merge\n> bases.\n\nI was assuming you're testing individual topics from gitster/git here\n(which admittedly is more CPU in total than just the integration\nbranches, but it at least parallelizes well).\n\nSo with that assumption, I was thinking that you'd just look for the\nmerge-base of HEAD and master, which should give you a single point for\nmost topics. For inter-twined topics there may be more merge bases, but\nI actually think for our purposes here, just testing the most recent one\nis probably OK. I.e., we're just trying to have a vague sense of whether\nthe test failure is due to new commits or old.\n\nI think Junio's suggestion to just pick some common release points would\nwork OK in practice, too. It's possible that some other topic made it to\nmaster with a breakage, but in most cases, I think these sorts of\nfailures are often more coarsely-grained (especially if Junio pays\nattention to the CI results before merging).\n\n-Peff\n"},{"id":"368741","messageId":"nycvar.QRO.7.76.6.1902072233070.41@tvgsbejvaqbjf.bet","threadId":"50388","inReplyTo":"xmqqy36rpki1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-07T21:34:31Z","receivedAt":"2019-02-07T21:34:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 7 Feb 2019, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > Even when there are even only as much as 12 merge bases to test (which is\n> > the current number of merge bases between `next` and `pu`),...\n> > ...\n> > And I sadly have to report that that's not the end of it. Back when I\n> > implemented the automatic bisect after failed builds (for details, see\n> > https://github.com/git-for-windows/build-extra/commit/c7e01e82c), I had to\n> > turn it off real quickly because the dumb bisect between `next` and `pu`\n> > regularly ran into the 4h timeout.\n> \n> Would it make it easier for you if you substituted all the mention\n> of 'next' in your message with 'pu^{/^### match next}'?  \n\nI was working on this in 2017, and could not make it to work as I wanted.\nEver since, that bisect code has been dormant.\n\nIn the meantime, I was able to parallelize the test suite enough to make\nit feasible to test the topic branches. That usually takes care of things\nreally quickly, and I just bite the bullet and bisect manually.\n\nCiao,\nDscho\n\n> \n> That mid-point between 'master' and 'pu' is designed to contain\n> exactly the same set of non-merge commits 'next' has, with the tree\n> that is identical to that of 'next', and from there to the tip of\n> 'pu' forms a single strand of merges of tips of topic branches that\n> are not yet merged to 'next' (by definition, it itself is the merge\n> base of it and 'pu').\n> \n> Bisecting along the first-parent chain from there to the tip of 'pu'\n> would let us identify which merge is faulty as the first-and-quick\n> pass and currently there are about 20 merges in that range on the\n> first-parent chain.\n> \n> \n"},{"id":"368742","messageId":"nycvar.QRO.7.76.6.1902072234390.41@tvgsbejvaqbjf.bet","threadId":"50388","inReplyTo":"20190207204910.GB28893@sigill.intra.peff.net","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-07T21:42:51Z","receivedAt":"2019-02-07T21:43:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 7 Feb 2019, Jeff King wrote:\n\n> On Thu, Feb 07, 2019 at 04:41:57PM +0100, Johannes Schindelin wrote:\n> \n> > > I think this can be limited to the tests that failed, which makes things\n> > > much faster. I.e., we run the tests at the tip of topic X and see that\n> > > t1234 fails. We then go back to the fork point and we just need to run\n> > > t1234 again. If it succeeds, then we blame X for the failure. If it\n> > > fails, then we consider it a false positive.\n> > \n> > If you mean merge bases by fork points, I wrote an Azure Pipeline to do\n> > that (so that I could use the cloud as kind of a fast computer), but that\n> > was still too slow.\n> > \n> > Even when there are even only as much as 12 merge bases to test (which is\n> > the current number of merge bases between `next` and `pu`), a build takes\n> > roughly 6 minutes on Windows, and many tests take 1 minute or more to run\n> > (offenders like t7003 and t7610 take over 400 seconds, i.e. roughly 6\n> > minutes), we are talking about roughly 1.5h *just* to test the merge\n> > bases.\n> \n> I was assuming you're testing individual topics from gitster/git here\n> (which admittedly is more CPU in total than just the integration\n> branches, but it at least parallelizes well).\n\nIndeed. And there, I can usually figure out really quickly myself (but\nmanually) what is going wrong.\n\nHopefully we have Azure Pipelines enabled on https://github.com/git/git\nsoon, with PR builds that include Windows (unlike our current Travis\nbuilds), so that contributors have an easier time to test their code in an\nautomated fashion.\n\nI also have on my backlog a task to include `sparse` in the Azure\nPipelines jobs. That should take care of even more things in a purely\nautomated fashion, as long as the contributors look at those builds.\n\n> So with that assumption, I was thinking that you'd just look for the\n> merge-base of HEAD and master, which should give you a single point for\n> most topics. For inter-twined topics there may be more merge bases, but\n> I actually think for our purposes here, just testing the most recent one\n> is probably OK. I.e., we're just trying to have a vague sense of whether\n> the test failure is due to new commits or old.\n\nOh, I am typically looking only at the latest commits up until I hit a\nmerge commit. Usually that already tells me enough, and if not, a bisect\nis really quick on a linear history.\n\nI guess my dumb branch^{/^Merge} to find the next merge commit works, but\nit is a bit unsatisfying that we do not have a more robust way to say\n\"traverse the commit history until finding a merge commit, then use that\".\n\n> I think Junio's suggestion to just pick some common release points would\n> work OK in practice, too. It's possible that some other topic made it to\n> master with a breakage, but in most cases, I think these sorts of\n> failures are often more coarsely-grained (especially if Junio pays\n> attention to the CI results before merging).\n\nIf Junio wants to experiment with that, sure, I'm all for it.\n\nCiao,\nDscho\n"},{"id":"368743","messageId":"nycvar.QRO.7.76.6.1902072245020.41@tvgsbejvaqbjf.bet","threadId":"50388","inReplyTo":"20190207204502.GA28893@sigill.intra.peff.net","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-07T21:57:48Z","receivedAt":"2019-02-07T21:57:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 7 Feb 2019, Jeff King wrote:\n\n> On Thu, Feb 07, 2019 at 03:26:21PM +0100, Johannes Schindelin wrote:\n> \n> > > So IMHO this isn't really a show-stopper problem, so much as\n> > > something that is a sign of the maturing test/CI setup (I say\n> > > \"maturing\", not \"mature\", as it seems we've probably still got a\n> > > ways to go). As far as notifications go, it probably makes sense for\n> > > them to be something that requires the user to sign up for anyway,\n> > > so at that point they're making their own choice about whether the\n> > > signal to noise ratio is acceptable.\n> > \n> > Maybe. I do not even know whether there is an option for that in Azure\n> > Pipelines, maybe GitHub offers that?\n> \n> No, I don't think so. Probably the route there would be to make a\n> comment on the commit or PR that would then go to the user as a\n> notification (from which they can then decide on email delivery, etc).\n\nAh, but that won't notify you when a Check failed. So that still would\nrequire some scripting.\n\n> > In any case, I just wanted to corroborate with a real-world example\n> > what I mentioned at the Contributors' Summit: that I would like to not\n> > script that thing yet where contributors are automatically notified\n> > when their branches don't pass.\n> \n> Fair enough. As an alternative, do you know offhand if there's an easy\n> machine-readable way to get the CI results? If I could poll it with curl\n> and generate my own notifications, that would be fine for me.\n\nThere is a REST API:\n\nhttps://docs.microsoft.com/en-us/rest/api/azure/devops/build/builds/list?view=azure-devops-rest-5.0\n\nSo this would give you the latest 5 failed builds:\n\ncurl \"https://dev.azure.com/gitgitgadget/git/_apis/build/builds?definitions=6&resultFilter=failed&\\$top=5\"\n\nI did not find a way to filter by user, or by branch name with wildcards,\nthough.\n\n> > Speaking of which... can we hook it up with https://github.com/git/git,\n> > now that the Azure Pipelines support is in `master`? I sent you and Junio\n> > an invitation to https://dev.azure.com/git/git, so that either you or\n> > Junio (who are the only owners of the GitHub repository) can set it up. If\n> > you want me to help, please do not hesitate to ping me on IRC.\n> \n> I'm happy to. I walked through the Azure setup/login procedure, but I'm\n> not sure what to do next.\n\nThe next step would be to install Azure Pipelines from the Marketplace and\nactivate it for git/git. There *should* be a wizard or something to walk\nyou through...\n\nCiao,\nDscho\n"},{"id":"368762","messageId":"20190208005852.GT10587@szeder.dev","threadId":"50388","inReplyTo":"20190207204502.GA28893@sigill.intra.peff.net","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-02-08T00:58:52Z","receivedAt":"2019-02-08T00:58:58Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Feb 07, 2019 at 03:45:02PM -0500, Jeff King wrote:\n> Fair enough. As an alternative, do you know offhand if there's an easy\n> machine-readable way to get the CI results? If I could poll it with curl\n> and generate my own notifications, that would be fine for me.\n\nWell, what do you mean by \"CI results\"?  Getting whether a build\nsucceeded, failed, still running, etc.?  Sure.  Getting which\nparticular test script (semantic patch, documentation...) caused the\nbuild failure?  Nope. [1]\n\nTravis CI has a REST API (note that you have to sign in with GitHub\naccount to view its docs, and then need an access token to use the\nAPI):\n\n  https://developer.travis-ci.org/gettingstarted\n\nThey also offer a command line client for this API:\n\n  https://github.com/travis-ci/travis.rb\n\nDepending on what you want that in itself might already be enough for\nyou.  It wasn't for me, as I have a very particular idea about how I\nprefer to view my CI results, but neither the website nor the CLI\nclient offer such a compact _and_ detailed view like this:\n\n  ccccccccc 2175  pu\n  ccccccccc 2174  sg/ci-parallel-build\n  ccccccccc 2173  js/fuzz-commit-graph-update\n  ccccccccc 2172  js/mingw-host-cpu\n  PsscsPscc 2171  dl/submodule-set-branch\n  PPXsPPPPP 2170  kl/pretty-doc-markup-fix\n  PPPPPPPPP 2169  en/combined-all-paths\n\n('c' - created, 's' - started, 'P' - passed, 'X' - failed)\n\nNothing that can't be achived with good screenful of Python/Ruby/etc\nscripting... including colors matching the website's color scheme! :)\n\n\n[1] Although since we include the trash directory of the failed test\n    script in the logs, surrounded by clear marker lines containing\n    the failed test script's name, it wouldn't be too hard to get it\n    programmatically, either.\n\n"},{"id":"368769","messageId":"20190208025133.GB11392@sigill.intra.peff.net","threadId":"50388","inReplyTo":"20190208005852.GT10587@szeder.dev","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-08T02:51:33Z","receivedAt":"2019-02-08T02:51:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 08, 2019 at 01:58:52AM +0100, SZEDER Gábor wrote:\n\n> On Thu, Feb 07, 2019 at 03:45:02PM -0500, Jeff King wrote:\n> > Fair enough. As an alternative, do you know offhand if there's an easy\n> > machine-readable way to get the CI results? If I could poll it with curl\n> > and generate my own notifications, that would be fine for me.\n> \n> Well, what do you mean by \"CI results\"?  Getting whether a build\n> succeeded, failed, still running, etc.?  Sure.  Getting which\n> particular test script (semantic patch, documentation...) caused the\n> build failure?  Nope. [1]\n\nIdeally yeah, I'd like to see the verbose (even \"-x\") log of the failed\ntest. But even an indication of a failure is enough that I know I can\nstart digging (and bonus if I can then dig into the log with a script\nand parse it myself).\n\n> Travis CI has a REST API (note that you have to sign in with GitHub\n> account to view its docs, and then need an access token to use the\n> API):\n\nThanks, I may poke around that.  In this particular case, though, I was\nwondering about the Azure Pipelines builds that Dscho has put together.\n\n> Depending on what you want that in itself might already be enough for\n> you.  It wasn't for me, as I have a very particular idea about how I\n> prefer to view my CI results, but neither the website nor the CLI\n> client offer such a compact _and_ detailed view like this:\n> \n>   ccccccccc 2175  pu\n>   ccccccccc 2174  sg/ci-parallel-build\n>   ccccccccc 2173  js/fuzz-commit-graph-update\n>   ccccccccc 2172  js/mingw-host-cpu\n>   PsscsPscc 2171  dl/submodule-set-branch\n>   PPXsPPPPP 2170  kl/pretty-doc-markup-fix\n>   PPPPPPPPP 2169  en/combined-all-paths\n\nMostly I just want to see the status of my own topics (ideally as soon\nas they're available, but even polling to get them within a few hours\nwould be OK). I run the tests locally, of course, but sometimes problems\nshow up on other platforms.\n\n-Peff\n"},{"id":"368770","messageId":"20190208025318.GC11392@sigill.intra.peff.net","threadId":"50388","inReplyTo":"nycvar.QRO.7.76.6.1902072245020.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-08T02:53:18Z","receivedAt":"2019-02-08T02:53:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 07, 2019 at 10:57:48PM +0100, Johannes Schindelin wrote:\n\n> > Fair enough. As an alternative, do you know offhand if there's an easy\n> > machine-readable way to get the CI results? If I could poll it with curl\n> > and generate my own notifications, that would be fine for me.\n> \n> There is a REST API:\n> \n> https://docs.microsoft.com/en-us/rest/api/azure/devops/build/builds/list?view=azure-devops-rest-5.0\n> \n> So this would give you the latest 5 failed builds:\n> \n> curl \"https://dev.azure.com/gitgitgadget/git/_apis/build/builds?definitions=6&resultFilter=failed&\\$top=5\"\n> \n> I did not find a way to filter by user, or by branch name with wildcards,\n> though.\n\nThanks. I'll play around with that. If I can get the data out at all,\nI'm sure I can massage it into some useful form with perl. That's what\nit's for, after all. :)\n\n> > I'm happy to. I walked through the Azure setup/login procedure, but I'm\n> > not sure what to do next.\n> \n> The next step would be to install Azure Pipelines from the Marketplace and\n> activate it for git/git. There *should* be a wizard or something to walk\n> you through...\n\nOK, I'll take a look (but probably not until tomorrow).\n\n-Peff\n"},{"id":"368776","messageId":"20190208043428.GA16432@sigill.intra.peff.net","threadId":"50388","inReplyTo":"20190208025318.GC11392@sigill.intra.peff.net","subject":"Re: [PATCH v3] doc-diff: don't `cd_to_toplevel`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-08T04:34:28Z","receivedAt":"2019-02-08T04:34:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 07, 2019 at 09:53:18PM -0500, Jeff King wrote:\n\n> > > I'm happy to. I walked through the Azure setup/login procedure, but I'm\n> > > not sure what to do next.\n> > \n> > The next step would be to install Azure Pipelines from the Marketplace and\n> > activate it for git/git. There *should* be a wizard or something to walk\n> > you through...\n> \n> OK, I'll take a look (but probably not until tomorrow).\n\nI did that, and it magically set things up, but under a \"peff\"\norganization. It seems that I don't have sufficient permissions on the\n\"git\" organization (on Azure) to do it.\n\n-Peff\n"}]}