{"thread":{"id":"54729","subject":"[PATCH v2 1/2] t7900: fix test failures when invoked individually via --run","startedAt":"2020-11-30T04:44:34Z","lastAt":"2020-12-01T12:33:03Z","messageCount":9,"participants":["Eric Sunshine","Ævar Arnfjörð Bjarmason","Derrick Stolee","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"410987","messageId":"20201130044224.12298-2-sunshine@sunshineco.com","threadId":"54729","inReplyTo":"20201130044224.12298-1-sunshine@sunshineco.com","subject":"[PATCH v2 1/2] t7900: fix test failures when invoked individually via --run","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-11-30T04:42:23Z","receivedAt":"2020-11-30T04:44:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"A couple tests use `rm expect` to remove a file created by earlier\ntests. The presence of this file is immaterial at the point the attempt\nis made to remove it, yet if it doesn't exist, the test fails\nunnecessarily. One case in which the file may validly not exist is when\n--run or GIT_SKIP_TESTS is used to selectively run particular tests. Fix\nthese pointless failures.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n t/t7900-maintenance.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 0246e4ce30..ef3aec3253 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -429,7 +429,7 @@ test_expect_success !MINGW 'start and stop macOS maintenance' '\n \tEOF\n \ttest_cmp expect actual &&\n \n-\trm expect &&\n+\trm -f expect &&\n \tfor frequency in hourly daily weekly\n \tdo\n \t\tPLIST=\"$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist\" &&\n@@ -491,7 +491,6 @@ test_expect_success 'start and stop Windows maintenance' '\n \t# stop does not unregister the repo\n \tgit config --get --global maintenance.repo \"$(pwd)\" &&\n \n-\trm expect &&\n \tprintf \"/delete /tn Git Maintenance (%s) /f\\n\" \\\n \t\thourly daily weekly >expect &&\n \ttest_cmp expect args\n-- \n2.29.2.576.ga3fc446d84\n\n"},{"id":"410988","messageId":"20201130044224.12298-3-sunshine@sunshineco.com","threadId":"54729","inReplyTo":"20201130044224.12298-1-sunshine@sunshineco.com","subject":"[PATCH v2 2/2] t7900: make macOS-specific test work on Windows","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-11-30T04:42:24Z","receivedAt":"2020-11-30T04:44:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Although `git maintenance start` and `git maintenance stop` necessarily\ninvoke platform-specific scheduling utilities, their related tests have\nbeen carefully crafted -- with one minor exception -- to work correctly\non any platform, thus improving overall coverage. The exception is that\nthe macOS-specific test fails on Windows due to non-portable use of\n`$(id -u)` and comparison involving the value of $HOME.\n\nIn particular, on Windows, the value of getuid() called by the C code is\nnot guaranteed to be the same as `$(id -u)` invoked by the test. This is\nbecause `git.exe` is a native Windows program, whereas the utility\nprograms run by the test script mostly utilize the MSYS2 runtime, which\nemulates a POSIX-like environment. Since the purpose of the test is to\ncheck that the input to the hook is well-formed, the actual user ID is\nimmaterial, thus we can work around the problem by making the the test\nUID-agnostic.\n\nAs for comparison of $HOME, it suffers from the typical shortcoming on\nWindows in which the same path may be represented two different ways\ndepending upon its source (i.e. as a Windows path\n`C:/git-sdk-64/usr/src/git/foo` versus as a Unix path\n`/usr/src/git/foo`).\n\nFix both problems and drop the !MINGW prerequisite from the\nmacOS-specific test, thus allowing the test to run on Windows, as well.\n\nHelped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n t/t7900-maintenance.sh | 15 ++++++++-------\n 1 file changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex ef3aec3253..514977a838 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -408,11 +408,12 @@ test_expect_success 'start preserves existing schedule' '\n \tgrep \"Important information!\" cron.txt\n '\n \n-test_expect_success !MINGW 'start and stop macOS maintenance' '\n-\tuid=$(id -u) &&\n+test_expect_success 'start and stop macOS maintenance' '\n+\t# ensure $HOME can be compared against hook arguments on all platforms\n+\tpfx=$(cd \"$HOME\" && pwd) &&\n \n \twrite_script print-args <<-\\EOF &&\n-\techo $* >>args\n+\techo $* | sed \"s:gui/[0-9][0-9]*:gui/[UID]:\" >>args\n \tEOF\n \n \trm -f args &&\n@@ -432,11 +433,11 @@ test_expect_success !MINGW 'start and stop macOS maintenance' '\n \trm -f expect &&\n \tfor frequency in hourly daily weekly\n \tdo\n-\t\tPLIST=\"$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist\" &&\n+\t\tPLIST=\"$pfx/Library/LaunchAgents/org.git-scm.git.$frequency.plist\" &&\n \t\ttest_xmllint \"$PLIST\" &&\n \t\tgrep schedule=$frequency \"$PLIST\" &&\n-\t\techo \"bootout gui/$uid $PLIST\" >>expect &&\n-\t\techo \"bootstrap gui/$uid $PLIST\" >>expect || return 1\n+\t\techo \"bootout gui/[UID] $PLIST\" >>expect &&\n+\t\techo \"bootstrap gui/[UID] $PLIST\" >>expect || return 1\n \tdone &&\n \ttest_cmp expect args &&\n \n@@ -446,7 +447,7 @@ test_expect_success !MINGW 'start and stop macOS maintenance' '\n \t# stop does not unregister the repo\n \tgit config --get --global maintenance.repo \"$(pwd)\" &&\n \n-\tprintf \"bootout gui/$uid $HOME/Library/LaunchAgents/org.git-scm.git.%s.plist\\n\" \\\n+\tprintf \"bootout gui/[UID] $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\\n\" \\\n \t\thourly daily weekly >expect &&\n \ttest_cmp expect args &&\n \tls \"$HOME/Library/LaunchAgents\" >actual &&\n-- \n2.29.2.576.ga3fc446d84\n\n"},{"id":"410989","messageId":"20201130044224.12298-1-sunshine@sunshineco.com","threadId":"54729","inReplyTo":null,"subject":"[PATCH v2 0/2] make macOS `git maintenance` test work on Windows","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-11-30T04:42:22Z","receivedAt":"2020-11-30T04:44:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This is a re-roll of [1] which makes the macOS-specific test of `git\nmaintenance start` and `git maintenance stop` work correctly on Windows\n(since the tests have been otherwise carefully crafted to work on any\nplatform even though the `start` and `stop` commands themselves are\nnecessarily platform-specific).\n\nv2 makes the macOS-specific test in t7900 UID-agnostic, as suggested by\nÆvar[2], thus the patch which added `test-tool getuid` has been dropped.\n\nThis series is built atop ds/maintenance-part-4.\n\n[1]: https://lore.kernel.org/git/20201127075054.31174-1-sunshine@sunshineco.com/\n[2]: https://lore.kernel.org/git/87o8jikfh7.fsf@evledraar.gmail.com/\n\nEric Sunshine (2):\n  t7900: fix test failures when invoked individually via --run\n  t7900: make macOS-specific test work on Windows\n\n t/t7900-maintenance.sh | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\nRange-diff against v1:\n1:  9f9b26de93 = 1:  d535669fb9 t7900: fix test failures when invoked individually via --run\n2:  07c9e9bd69 < -:  ---------- test-tool: add `getuid` subcommand\n3:  01c6a3229d ! 2:  8561d08bcd t7900: make macOS-specific test work on Windows\n    @@ Commit message\n         invoke platform-specific scheduling utilities, their related tests have\n         been carefully crafted -- with one minor exception -- to work correctly\n         on any platform, thus improving overall coverage. The exception is that\n    -    the macOS-specific test fails on Windows due to unportable use of\n    -    `$(id -u)` and comparison involving the value of $HOME which suffers\n    -    from the typical shortcoming on that platform in which the same path may\n    -    be represented two different ways depending upon its source (i.e. as a\n    -    Windows path `C:/git-sdk-64/usr/src/git/foo` versus as a Unix path\n    -    `/usr/src/git/foo`). Fix both problems and drop the !MINGW prerequisite\n    -    from the macOS-specific test, thus allowing the test to run on Windows,\n    -    as well.\n    +    the macOS-specific test fails on Windows due to non-portable use of\n    +    `$(id -u)` and comparison involving the value of $HOME.\n     \n    +    In particular, on Windows, the value of getuid() called by the C code is\n    +    not guaranteed to be the same as `$(id -u)` invoked by the test. This is\n    +    because `git.exe` is a native Windows program, whereas the utility\n    +    programs run by the test script mostly utilize the MSYS2 runtime, which\n    +    emulates a POSIX-like environment. Since the purpose of the test is to\n    +    check that the input to the hook is well-formed, the actual user ID is\n    +    immaterial, thus we can work around the problem by making the the test\n    +    UID-agnostic.\n    +\n    +    As for comparison of $HOME, it suffers from the typical shortcoming on\n    +    Windows in which the same path may be represented two different ways\n    +    depending upon its source (i.e. as a Windows path\n    +    `C:/git-sdk-64/usr/src/git/foo` versus as a Unix path\n    +    `/usr/src/git/foo`).\n    +\n    +    Fix both problems and drop the !MINGW prerequisite from the\n    +    macOS-specific test, thus allowing the test to run on Windows, as well.\n    +\n    +    Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n         Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n     \n      ## t/t7900-maintenance.sh ##\n    @@ t/t7900-maintenance.sh: test_expect_success 'start preserves existing schedule'\n     -test_expect_success !MINGW 'start and stop macOS maintenance' '\n     -\tuid=$(id -u) &&\n     +test_expect_success 'start and stop macOS maintenance' '\n    -+\tuid=$(test-tool getuid) &&\n     +\t# ensure $HOME can be compared against hook arguments on all platforms\n     +\tpfx=$(cd \"$HOME\" && pwd) &&\n      \n      \twrite_script print-args <<-\\EOF &&\n    - \techo $* >>args\n    +-\techo $* >>args\n    ++\techo $* | sed \"s:gui/[0-9][0-9]*:gui/[UID]:\" >>args\n    + \tEOF\n    + \n    + \trm -f args &&\n     @@ t/t7900-maintenance.sh: test_expect_success !MINGW 'start and stop macOS maintenance' '\n      \trm -f expect &&\n      \tfor frequency in hourly daily weekly\n    @@ t/t7900-maintenance.sh: test_expect_success !MINGW 'start and stop macOS mainten\n     +\t\tPLIST=\"$pfx/Library/LaunchAgents/org.git-scm.git.$frequency.plist\" &&\n      \t\ttest_xmllint \"$PLIST\" &&\n      \t\tgrep schedule=$frequency \"$PLIST\" &&\n    - \t\techo \"bootout gui/$uid $PLIST\" >>expect &&\n    +-\t\techo \"bootout gui/$uid $PLIST\" >>expect &&\n    +-\t\techo \"bootstrap gui/$uid $PLIST\" >>expect || return 1\n    ++\t\techo \"bootout gui/[UID] $PLIST\" >>expect &&\n    ++\t\techo \"bootstrap gui/[UID] $PLIST\" >>expect || return 1\n    + \tdone &&\n    + \ttest_cmp expect args &&\n    + \n     @@ t/t7900-maintenance.sh: test_expect_success !MINGW 'start and stop macOS maintenance' '\n      \t# stop does not unregister the repo\n      \tgit config --get --global maintenance.repo \"$(pwd)\" &&\n      \n     -\tprintf \"bootout gui/$uid $HOME/Library/LaunchAgents/org.git-scm.git.%s.plist\\n\" \\\n    -+\tprintf \"bootout gui/$uid $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\\n\" \\\n    ++\tprintf \"bootout gui/[UID] $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\\n\" \\\n      \t\thourly daily weekly >expect &&\n      \ttest_cmp expect args &&\n      \tls \"$HOME/Library/LaunchAgents\" >actual &&\n-- \n2.29.2.576.ga3fc446d84\n\n"},{"id":"410991","messageId":"87blffjkne.fsf@evledraar.gmail.com","threadId":"54729","inReplyTo":"20201130044224.12298-1-sunshine@sunshineco.com","subject":"Re: [PATCH v2 0/2] make macOS `git maintenance` test work on Windows","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-11-30T09:20:00Z","receivedAt":"2020-11-30T09:20:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 30 2020, Eric Sunshine wrote:\n\n> v2 makes the macOS-specific test in t7900 UID-agnostic, as suggested by\n> Ævar[2], thus the patch which added `test-tool getuid` has been dropped.\n\nLGTM. FWIW I think your v1 is fine too, just meant to comment on the\nbasis of \"you could also do it like that\". Having a C program call\ngetuid() is fine, so is faking it. If you prefer the latter, cool.\n\nAs an aside and just because this is fresh in my mind, not really\nrelevant to this patch as-such:\n\nI did wonder \"why not just call perl -e 'print $<' ?\"  first. But then\nfound (by reading the Perl source[1], didn't actually test this) that it\nfakes up UID=0 for everything on Windows.\n\nI couldn't find any \"is root?\" in our tree that relies on Perl's $< in a\nbad way (the couuple of occurances seem innocuous), we have some \"id -u\"\nchecks, but those also seem OK if it returned 0 on Windows (what does it\nreturn?). Seems the worst we'd do there is unintentionally skip some\n\"skip this as root\" tests.\n\nAlso as another aside (but your patch is fine as it is), my suggestion\nused Perl+Perl RX but you switched it to sed+BRE. Do we want to avoid\n\"sed -E\"? I wondered that for something else the other day, we have\nthis:\n\n    t/check-non-portable-shell.pl:  /\\bsed\\s+-[^efn]\\s+/ and err 'sed option not portable (use only -n, -e, -f)';\n\nSo maybe it means \"nothing but -nef, or maybe \"don't use -efn\". The ERE\n(-E and -r) options aren't mentioned, and a naïve log search of of \"sed\n-E\" and \"sed -r\" in t/ returns nothing.\n\nIt's also my impression that just using $(\"$PERL_PATH\" -e ...) is fine,\nand at least to my reading the Perl RX is more readable with look-behind\nassertions, but I'm biased by familiarity with it.\n\nOur PERL prereq & NO_PERL=YesPlease is just for \"this may require a\nnon-ancient Perl\" & \"don't install Perl for runtime stuff\"\nrespectively. Is that not the case and we'd like to avoid new perl\ninvocations where possible?\n\nI don't really care either way (or, if your switch in this case was just\na personal preference, also fine), but if we are trying to somewhat\ndiscourage perl use (and maybe eventually get rid of it entirely) that\nwould be a useful t/README doc update.\n\nI know Johannes (CC'd) has (this is from wetware memory) wanted to\n(understandably) not need to bother with Perl as part of GFW, but I\ncan't remember if that's for a reason covered by NO_PERL=YesPlease,\ni.e. packaging it up, or whether it's also to not wanting to provide a\nperl for the test suite.\n\n1. https://github.com/Perl/perl5/blob/v5.33.4/win32/win32.c#L1079-L1092\n"},{"id":"410995","messageId":"e4647058-3bb6-7aa4-c245-95e3f4b3bc66@gmail.com","threadId":"54729","inReplyTo":"20201130044224.12298-3-sunshine@sunshineco.com","subject":"Re: [PATCH v2 2/2] t7900: make macOS-specific test work on Windows","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-11-30T13:12:25Z","receivedAt":"2020-11-30T13:13:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/29/2020 11:42 PM, Eric Sunshine wrote:\n> Although `git maintenance start` and `git maintenance stop` necessarily\n> invoke platform-specific scheduling utilities, their related tests have\n> been carefully crafted -- with one minor exception -- to work correctly\n> on any platform, thus improving overall coverage. The exception is that\n> the macOS-specific test fails on Windows due to non-portable use of\n> `$(id -u)` and comparison involving the value of $HOME.\n\nI appreciate your time making this change, as I was unable to work\naround these issues myself.\n\n> In particular, on Windows, the value of getuid() called by the C code is\n> not guaranteed to be the same as `$(id -u)` invoked by the test. This is\n> because `git.exe` is a native Windows program, whereas the utility\n> programs run by the test script mostly utilize the MSYS2 runtime, which\n> emulates a POSIX-like environment. Since the purpose of the test is to\n> check that the input to the hook is well-formed, the actual user ID is\n> immaterial, thus we can work around the problem by making the the test\n> UID-agnostic.\n> \n> As for comparison of $HOME, it suffers from the typical shortcoming on\n> Windows in which the same path may be represented two different ways\n> depending upon its source (i.e. as a Windows path\n> `C:/git-sdk-64/usr/src/git/foo` versus as a Unix path\n> `/usr/src/git/foo`).\n> \n> Fix both problems and drop the !MINGW prerequisite from the\n> macOS-specific test, thus allowing the test to run on Windows, as well.\n> \n> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n> ---\n>  t/t7900-maintenance.sh | 15 ++++++++-------\n>  1 file changed, 8 insertions(+), 7 deletions(-)\n> \n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index ef3aec3253..514977a838 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -408,11 +408,12 @@ test_expect_success 'start preserves existing schedule' '\n>  \tgrep \"Important information!\" cron.txt\n>  '\n>  \n> -test_expect_success !MINGW 'start and stop macOS maintenance' '\n> -\tuid=$(id -u) &&\n> +test_expect_success 'start and stop macOS maintenance' '\n> +\t# ensure $HOME can be compared against hook arguments on all platforms\n> +\tpfx=$(cd \"$HOME\" && pwd) &&\n\n> -\t\tPLIST=\"$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist\" &&\n> +\t\tPLIST=\"$pfx/Library/LaunchAgents/org.git-scm.git.$frequency.plist\" &&\n\nThis pair of changes make sense to get around the $HOME issue\nthat caused me to surrender the effort.\n\n>  \twrite_script print-args <<-\\EOF &&\n> -\techo $* >>args\n> +\techo $* | sed \"s:gui/[0-9][0-9]*:gui/[UID]:\" >>args\n\nFiltering out the UID into a fixed string...\n\n> -\t\techo \"bootout gui/$uid $PLIST\" >>expect &&\n> -\t\techo \"bootstrap gui/$uid $PLIST\" >>expect || return 1\n> +\t\techo \"bootout gui/[UID] $PLIST\" >>expect &&\n> +\t\techo \"bootstrap gui/[UID] $PLIST\" >>expect || return 1\n>  \tdone &&\n>  \ttest_cmp expect args &&\n\n.. then checking for that fixed string. Clever!\n\nThanks!\n-Stolee\n"},{"id":"411018","messageId":"xmqqr1oato8m.fsf@gitster.c.googlers.com","threadId":"54729","inReplyTo":"87blffjkne.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 0/2] make macOS `git maintenance` test work on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-30T23:32:25Z","receivedAt":"2020-11-30T23:33:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> Also as another aside (but your patch is fine as it is), my suggestion\n> used Perl+Perl RX but you switched it to sed+BRE. Do we want to avoid\n> \"sed -E\"? I wondered that for something else the other day, we have\n> this:\n>\n>     t/check-non-portable-shell.pl:  /\\bsed\\s+-[^efn]\\s+/ and err 'sed option not portable (use only -n, -e, -f)';\n>\n> So maybe it means \"nothing but -nef, or maybe \"don't use -efn\". The ERE\n> (-E and -r) options aren't mentioned, and a naïve log search of of \"sed\n> -E\" and \"sed -r\" in t/ returns nothing.\n\nCorrect.  We currently do not use \"sed -E\", and the script says\n\"Let's not use it; it's not even in POSIX\" for things other than\n'n', 'e', and 'f'.\n\nhttps://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html\n\nThanks.\n"},{"id":"411030","messageId":"CAPig+cSK4y2P2PUVnXyaB5T0y-RBJOsb-npwM_8Wnm7oqRQLGQ@mail.gmail.com","threadId":"54729","inReplyTo":"e4647058-3bb6-7aa4-c245-95e3f4b3bc66@gmail.com","subject":"Re: [PATCH v2 2/2] t7900: make macOS-specific test work on Windows","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-01T03:03:29Z","receivedAt":"2020-12-01T03:04:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 30, 2020 at 8:12 AM Derrick Stolee <stolee@gmail.com> wrote:\n> On 11/29/2020 11:42 PM, Eric Sunshine wrote:\n> > +     # ensure $HOME can be compared against hook arguments on all platforms\n> > +     pfx=$(cd \"$HOME\" && pwd) &&\n> > +             PLIST=\"$pfx/Library/LaunchAgents/org.git-scm.git.$frequency.plist\" &&\n>\n> This pair of changes make sense to get around the $HOME issue\n> that caused me to surrender the effort.\n\nIn case it's not clear to other readers not familiar with Git on\nWindows (or who have not read t/README), the magic here is that `pwd`\nis overridden in t/test-lib.sh for MINGW to always return a\nWindows-style path.\n"},{"id":"411032","messageId":"CAPig+cT-axeG3UrxaQR3c+r7dMZkjmS19C3cHRVzu5KSc72BOQ@mail.gmail.com","threadId":"54729","inReplyTo":"87blffjkne.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 0/2] make macOS `git maintenance` test work on Windows","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-01T03:58:36Z","receivedAt":"2020-12-01T03:59:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 30, 2020 at 4:20 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> LGTM. FWIW I think your v1 is fine too, just meant to comment on the\n> basis of \"you could also do it like that\". Having a C program call\n> getuid() is fine, so is faking it. If you prefer the latter, cool.\n\nI do like the simplicity of the latter, and I wasn't super happy about\nintroducing a new test-tool subcommand just to make this one test\npass, especially since `test-tool getuid` is so different from most\nother subcommands which are typically added to give us access to some\ninternal element of Git otherwise not accessible to the test scripts.\n\n> I did wonder \"why not just call perl -e 'print $<' ?\"  first. But then\n> found (by reading the Perl source[1], didn't actually test this) that it\n> fakes up UID=0 for everything on Windows.\n\nI totally forgot about Perl's `$<`.\n\nUnder the Git for Windows SDK, Perl's `$<` returns a large positive\nnumber. I suspect this differs from what you saw in the Perl source\ncode because the Windows-specific code you looked at does not come\ninto play in this case. For Git for Windows SDK, Perl is almost\ncertainly instead built in Unix-like mode, linking against the MSYS2\nlibrary for its POSIX-like emulation, thus the Windows-specific Perl\ngoop is not needed.\n\n> I couldn't find any \"is root?\" in our tree that relies on Perl's $< in a\n> bad way (the couuple of occurances seem innocuous), we have some \"id -u\"\n> checks, but those also seem OK if it returned 0 on Windows (what does it\n> return?). Seems the worst we'd do there is unintentionally skip some\n> \"skip this as root\" tests.\n\nUnder Git for Windows SDK, `id -u` returns the same large positive\nnumber as returned by Perl's `$<`, which makes sense since `id` is\nalso likely linked against the MSYS2 library.\n\nAs for getuid() in Git itself, that always returns 1. I see now that's\nbecause 1 is hard-coded in Git's compat/mingw.h override of getuid().\nSo, an alternative would have been for the test to use `uid=1` on\nMINGW, but I like the current approach better of having the test be\nUID-agnostic.\n\n> It's also my impression that just using $(\"$PERL_PATH\" -e ...) is fine,\n> and at least to my reading the Perl RX is more readable with look-behind\n> assertions, but I'm biased by familiarity with it.\n\nThe `sed` version seems simpler and more straightforward to me,\nwhereas look-behind feels harder to reason about, but of course it's\nall subjective and not terribly important. Either would be fine.\n\n> Our PERL prereq & NO_PERL=YesPlease is just for \"this may require a\n> non-ancient Perl\" & \"don't install Perl for runtime stuff\"\n> respectively. Is that not the case and we'd like to avoid new perl\n> invocations where possible?\n>\n> I don't really care either way (or, if your switch in this case was just\n> a personal preference, also fine), but if we are trying to somewhat\n> discourage perl use (and maybe eventually get rid of it entirely) that\n> would be a useful t/README doc update.\n\nUsing `sed` was just a personal preference, partly because the `sed`\nexpression seems simpler to me, but mostly because Perl still feels\nheavyweight to me compared to `sed`. It also may be that I'm just\nold-school, preferring the small, sharp utilities (`sed`, `grep`,\n`sort`, etc.), and only turning to Perl (or one of its ilk) when the\ntask demands a more general-purpose tool.\n\nI haven't measured, but it's possible that Perl may indeed be\nheavyweight on Windows in terms of startup time.\n\n> I know Johannes (CC'd) has (this is from wetware memory) wanted to\n> (understandably) not need to bother with Perl as part of GFW, but I\n> can't remember if that's for a reason covered by NO_PERL=YesPlease,\n> i.e. packaging it up, or whether it's also to not wanting to provide a\n> perl for the test suite.\n\nI _think_ that was for the NO_PERL=YesPlease case, but I expect Dscho\ncan answer more concretely.\n"},{"id":"411066","messageId":"nycvar.QRO.7.76.6.2012011303210.54@tvgsbejvaqbjf.bet","threadId":"54729","inReplyTo":"CAPig+cT-axeG3UrxaQR3c+r7dMZkjmS19C3cHRVzu5KSc72BOQ@mail.gmail.com","subject":"Re: [PATCH v2 0/2] make macOS `git maintenance` test work on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-12-01T12:31:16Z","receivedAt":"2020-12-01T12:33:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric & Ævar,\n\nOn Mon, 30 Nov 2020, Eric Sunshine wrote:\n\n> On Mon, Nov 30, 2020 at 4:20 AM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>\n> > I did wonder \"why not just call perl -e 'print $<' ?\"  first. But then\n> > found (by reading the Perl source[1], didn't actually test this) that it\n> > fakes up UID=0 for everything on Windows.\n>\n> I totally forgot about Perl's `$<`.\n>\n> Under the Git for Windows SDK, Perl's `$<` returns a large positive\n> number. I suspect this differs from what you saw in the Perl source\n> code because the Windows-specific code you looked at does not come\n> into play in this case. For Git for Windows SDK, Perl is almost\n> certainly instead built in Unix-like mode, linking against the MSYS2\n> library for its POSIX-like emulation, thus the Windows-specific Perl\n> goop is not needed.\n\nCorrect.\n\n> > I couldn't find any \"is root?\" in our tree that relies on Perl's $< in a\n> > bad way (the couuple of occurances seem innocuous), we have some \"id -u\"\n> > checks, but those also seem OK if it returned 0 on Windows (what does it\n> > return?). Seems the worst we'd do there is unintentionally skip some\n> > \"skip this as root\" tests.\n>\n> Under Git for Windows SDK, `id -u` returns the same large positive\n> number as returned by Perl's `$<`, which makes sense since `id` is\n> also likely linked against the MSYS2 library.\n\nCorrect.\n\n> > I know Johannes (CC'd) has (this is from wetware memory) wanted to\n> > (understandably) not need to bother with Perl as part of GFW, but I\n> > can't remember if that's for a reason covered by NO_PERL=YesPlease,\n> > i.e. packaging it up, or whether it's also to not wanting to provide a\n> > perl for the test suite.\n>\n> I _think_ that was for the NO_PERL=YesPlease case, but I expect Dscho\n> can answer more concretely.\n\nI assume you're referring to NO_PERL being set in the CI builds on\nWindows?\n\nThere were two problems we tried to address via setting NO_PERL: MSYS2\nproblems on 32-bit Windows (where the `fork()` emulation would frequently\nrun into DLL base address issues due to the limited address space combined\nwith the requirement to keep the MSYS2 DLL at a _fixed_ memory location),\nand speed.\n\nWe no longer run the 32-bit Pipeline, saving a couple electrons. But speed\nis still a major concern with the automated test suite. Just have a look:\nhttps://github.com/git/git/actions/runs/393089617 shows the `linux-clang`\njob running for a little over 27 minutes, and that includes the build and\ntwo test suite runs (taking 12m08s and 12m41s, respectively). At the same\ntime, the Windows tests (_not_ including the build), split into 10\nparallel jobs, take between ~8-13.5 minutes to run (granted, there is a\n1.5 minute overhead in each split job to download and set up stuff). And\nthat's _skipping_ the test cases requiring Perl, or GPG, or Perforce,\netc...\n\nPerl is slow. Even on Linux. Running t3701 with the scripted vs the\nbuilt-in `git add -i` shows a stark contrast. See for yourself:\nhttps://github.com/git/git/runs/1478012705?check_suite_focus=true#step:4:994\nsays that the scripted version took just under 25 seconds, while\nhttps://github.com/git/git/runs/1478012705?check_suite_focus=true#step:4:1908\nsays that the built-in version took just over 8 seconds (this is the\n`linux-gcc` job that runs the test suite twice, 2nd time with\n`GIT_TEST_ADD_I_USE_BUILTIN=1`).\n\nAnd that's on Linux.\n\nOn Windows, not even running any other tests in parallel (i.e. unlike the\n`linux-gcc` job), the difference is 1m23s vs 39s on my beefy laptop.\nThat's how slow Perl is.\n\nImagine how much faster the entire test suite would run, for every\ndeveloper, if we didn't use Perl (nor Bash, for that matter). This is not\ntheoretical. There were, and are, instances of contributions which\nobviously did not run through the test suite before being submitted. I\nsuspect that it simply takes too long.\n\nCiao,\nDscho\n"}]}