{"thread":{"id":"55764","subject":"[PATCH] t9001-send-email.sh: fix expected absolute paths on Windows","startedAt":"2021-05-24T19:38:13Z","lastAt":"2021-06-02T11:40:52Z","messageCount":20,"participants":["Johannes Sixt","Jonathan Nieder","Ævar Arnfjörð Bjarmason","Junio C Hamano","Robert Foss","Felipe Contreras","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"425452","messageId":"bb30fe2b-cd75-4782-24a6-08bb002a0367@kdbg.org","threadId":"55764","inReplyTo":null,"subject":"[PATCH] t9001-send-email.sh: fix expected absolute paths on Windows","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-05-24T19:38:09Z","receivedAt":"2021-05-24T19:38:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Git for Windows is a native Windows program that works with native\nabsolute paths in the drive letter style C:\\dir. The auxiliary\ninfrastructure is based on MSYS2, which uses POSIX style /C/dir.\n\nWhen we test for output of absolute paths produced by git.exe, we\nusally have to expect C:\\dir style paths. To produce such expected\npaths, we have to use $(pwd) in the test scripts; the alternative,\n$PWD, produces a POSIX style path. ($PWD is a shell variable, and the\nshell is bash, an MSYS2 program, and operates in the POSIX realm.)\n\nThere are two recently added tests that were written to expect C:\\dir\npaths. The output that is tested is produced by `git send-email`, but\nbehind the scenes, this is a Perl script, which also works in the\nPOSIX realm and produces /C/dir style output.\n\nIn the first test case that is changed here, replace $(pwd) by $PWD\nso that the expected path is constructed using /C/dir style.\n\nThe second test case sets core.hooksPath to an absolute path. Since\nthe test script talks to native git.exe, it is supposed to place a\nC:/dir style path into the configuration; therefore, keep $(pwd).\nWhen this configuration value is consumed by the Perl script, it is\ntransformed to /C/dir style by the MSYS2 layer and echoed back in\nthis form in the error message. Hence, do use $PWD for the expected\nvalue.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n When I say \"the configuration is transformed to /C/dir style\", I am\n actually hand-waving: I can observe that a transformation must\n happen somewhere, but I actually do not know where the conversion\n really happens. \"The MSYS2 layer\" is my best qualified guess.\n\n t/t9001-send-email.sh | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 65b3035371..68bebc505b 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -539,15 +539,14 @@ test_expect_success $PREREQ \"--validate respects relative core.hooksPath path\" '\n \ttest_path_is_file my-hooks.ran &&\n \tcat >expect <<-EOF &&\n \tfatal: longline.patch: rejected by sendemail-validate hook\n-\tfatal: command '\"'\"'$(pwd)/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n+\tfatal: command '\"'\"'$PWD/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n \twarning: no patches were sent\n \tEOF\n \ttest_cmp expect actual\n '\n \n test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n-\thooks_path=\"$(pwd)/my-hooks\" &&\n-\ttest_config core.hooksPath \"$hooks_path\" &&\n+\ttest_config core.hooksPath \"$(pwd)/my-hooks\" &&\n \ttest_when_finished \"rm my-hooks.ran\" &&\n \ttest_must_fail git send-email \\\n \t\t--from=\"Example <nobody@example.com>\" \\\n@@ -558,7 +557,7 @@ test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n \ttest_path_is_file my-hooks.ran &&\n \tcat >expect <<-EOF &&\n \tfatal: longline.patch: rejected by sendemail-validate hook\n-\tfatal: command '\"'\"'$hooks_path/sendemail-validate'\"'\"' died with exit code 1\n+\tfatal: command '\"'\"'$PWD/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n \twarning: no patches were sent\n \tEOF\n \ttest_cmp expect actual\n-- \n2.31.0.152.g120726e270\n\n"},{"id":"425456","messageId":"YKwL3t8bKIh6LPsE@google.com","threadId":"55764","inReplyTo":"bb30fe2b-cd75-4782-24a6-08bb002a0367@kdbg.org","subject":"Re: [PATCH] t9001-send-email.sh: fix expected absolute paths on Windows","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2021-05-24T20:26:06Z","receivedAt":"2021-05-24T20:26:11Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJohannes Sixt wrote:\n\n> Git for Windows is a native Windows program that works with native\n> absolute paths in the drive letter style C:\\dir. The auxiliary\n> infrastructure is based on MSYS2, which uses POSIX style /C/dir.\n[nice explanation snipped]\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n>  When I say \"the configuration is transformed to /C/dir style\", I am\n>  actually hand-waving: I can observe that a transformation must\n>  happen somewhere, but I actually do not know where the conversion\n>  really happens. \"The MSYS2 layer\" is my best qualified guess.\n\nThanks.  The explanation is appreciated --- it helps avoid the feeling\nof randomness involved.  Hopefully some day our test setup will allow\ndoing everything at the \"native Windows program\" level (well, I can\nhope).\n\n[...]\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -539,15 +539,14 @@ test_expect_success $PREREQ \"--validate respects relative core.hooksPath path\" '\n>  \ttest_path_is_file my-hooks.ran &&\n>  \tcat >expect <<-EOF &&\n>  \tfatal: longline.patch: rejected by sendemail-validate hook\n> -\tfatal: command '\"'\"'$(pwd)/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n> +\tfatal: command '\"'\"'$PWD/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n>  \twarning: no patches were sent\n>  \tEOF\n>  \ttest_cmp expect actual\n\nIdeally we wouldn't have to check the exact output at all.  Is there a\nreason we care about the absolute path being echoed on error?\n\n[...]\n>  test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n> -\thooks_path=\"$(pwd)/my-hooks\" &&\n> -\ttest_config core.hooksPath \"$hooks_path\" &&\n> +\ttest_config core.hooksPath \"$(pwd)/my-hooks\" &&\n>  \ttest_when_finished \"rm my-hooks.ran\" &&\n>  \ttest_must_fail git send-email \\\n>  \t\t--from=\"Example <nobody@example.com>\" \\\n> @@ -558,7 +557,7 @@ test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n>  \ttest_path_is_file my-hooks.ran &&\n>  \tcat >expect <<-EOF &&\n>  \tfatal: longline.patch: rejected by sendemail-validate hook\n> -\tfatal: command '\"'\"'$hooks_path/sendemail-validate'\"'\"' died with exit code 1\n> +\tfatal: command '\"'\"'$PWD/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n>  \twarning: no patches were sent\n>  \tEOF\n>  \ttest_cmp expect actual\n\nLikewise.\n\nThat said, the patch as is is\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n"},{"id":"425462","messageId":"87im37ojrn.fsf@evledraar.gmail.com","threadId":"55764","inReplyTo":"bb30fe2b-cd75-4782-24a6-08bb002a0367@kdbg.org","subject":"Re: [PATCH] t9001-send-email.sh: fix expected absolute paths on Windows","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-24T22:15:44Z","receivedAt":"2021-05-24T22:21:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, May 24 2021, Johannes Sixt wrote:\n\nAlso CC-ing Robert Foss <robert.foss@linaro.org>, I last touched this\ncode, but the fallout is ultimately from his c8243933c74\n(git-send-email: Respect core.hooksPath setting, 2021-03-23).\n\n> Git for Windows is a native Windows program that works with native\n> absolute paths in the drive letter style C:\\dir. The auxiliary\n> infrastructure is based on MSYS2, which uses POSIX style /C/dir.\n>\n> When we test for output of absolute paths produced by git.exe, we\n> usally have to expect C:\\dir style paths. To produce such expected\n> paths, we have to use $(pwd) in the test scripts; the alternative,\n> $PWD, produces a POSIX style path. ($PWD is a shell variable, and the\n> shell is bash, an MSYS2 program, and operates in the POSIX realm.)\n>\n> There are two recently added tests that were written to expect C:\\dir\n> paths. The output that is tested is produced by `git send-email`, but\n> behind the scenes, this is a Perl script, which also works in the\n> POSIX realm and produces /C/dir style output.\n>\n> In the first test case that is changed here, replace $(pwd) by $PWD\n> so that the expected path is constructed using /C/dir style.\n>\n> The second test case sets core.hooksPath to an absolute path. Since\n> the test script talks to native git.exe, it is supposed to place a\n> C:/dir style path into the configuration; therefore, keep $(pwd).\n> When this configuration value is consumed by the Perl script, it is\n> transformed to /C/dir style by the MSYS2 layer and echoed back in\n> this form in the error message. Hence, do use $PWD for the expected\n> value.\n>\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n>  When I say \"the configuration is transformed to /C/dir style\", I am\n>  actually hand-waving: I can observe that a transformation must\n>  happen somewhere, but I actually do not know where the conversion\n>  really happens. \"The MSYS2 layer\" is my best qualified guess.\n>\n>  t/t9001-send-email.sh | 7 +++----\n>  1 file changed, 3 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> index 65b3035371..68bebc505b 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -539,15 +539,14 @@ test_expect_success $PREREQ \"--validate respects relative core.hooksPath path\" '\n>  \ttest_path_is_file my-hooks.ran &&\n>  \tcat >expect <<-EOF &&\n>  \tfatal: longline.patch: rejected by sendemail-validate hook\n> -\tfatal: command '\"'\"'$(pwd)/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n> +\tfatal: command '\"'\"'$PWD/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n>  \twarning: no patches were sent\n>  \tEOF\n>  \ttest_cmp expect actual\n>  '\n>  \n>  test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n> -\thooks_path=\"$(pwd)/my-hooks\" &&\n> -\ttest_config core.hooksPath \"$hooks_path\" &&\n> +\ttest_config core.hooksPath \"$(pwd)/my-hooks\" &&\n>  \ttest_when_finished \"rm my-hooks.ran\" &&\n>  \ttest_must_fail git send-email \\\n>  \t\t--from=\"Example <nobody@example.com>\" \\\n> @@ -558,7 +557,7 @@ test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n>  \ttest_path_is_file my-hooks.ran &&\n>  \tcat >expect <<-EOF &&\n>  \tfatal: longline.patch: rejected by sendemail-validate hook\n> -\tfatal: command '\"'\"'$hooks_path/sendemail-validate'\"'\"' died with exit code 1\n> +\tfatal: command '\"'\"'$PWD/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n>  \twarning: no patches were sent\n>  \tEOF\n>  \ttest_cmp expect actual\n\nDoes this alternate patch[1] also fix the issue? I don't have a Windows\nsystem on which to test this, but it seems to me like it should.\n\nI.e. the issue seems to me to me that we have an absolute path from\n--git-path, and one of Cwd.pm's or File::Spec.pm's ideas of what that\nabsolute path should look like differs from ours.\n\nWe have a sprinkle of File::Spec->file_name_is_absolute($dir) in Git.pm\nfor some other stuff to deal with the same scenario, but I don't see why\nwe need the abs_path() here at all.\n\nEither we have a relative path from \"rev-parse --git-dir hooks\", or an\nabsolute one, in either case we feed it to Perl's\nsystem(\"some-relative-or-absolute-path\").\n\nI have a parallel series where I did some send-email changes by just\nextracting the relevant code from Git.pm, since there were objections to\nchanging the \"public API\". But in this case there's been no release with\nthis, so presumably it's fine to just change it.\n\n1.\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 73ebbf80cc6..df6280ebab5 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -629,8 +629,7 @@ sub hooks_path {\n \tmy ($self) = @_;\n \n \tmy $dir = $self->command_oneline('rev-parse', '--git-path', 'hooks');\n-\tmy $abs = abs_path($dir);\n-\treturn $abs;\n+\treturn $dir;\n }\n \n =item wc_path ()\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 65b30353719..9c518462c3e 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -539,7 +539,7 @@ test_expect_success $PREREQ \"--validate respects relative core.hooksPath path\" '\n \ttest_path_is_file my-hooks.ran &&\n \tcat >expect <<-EOF &&\n \tfatal: longline.patch: rejected by sendemail-validate hook\n-\tfatal: command '\"'\"'$(pwd)/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n+\tfatal: command '\"'\"'my-hooks/sendemail-validate'\"'\"' died with exit code 1\n \twarning: no patches were sent\n \tEOF\n \ttest_cmp expect actual\n"},{"id":"425466","messageId":"cover-0.2-00000000000-20210524T231047Z-avarab@gmail.com","threadId":"55764","inReplyTo":"bb30fe2b-cd75-4782-24a6-08bb002a0367@kdbg.org","subject":"[PATCH 0/2] send-email: pre-release fixes for v2.32.0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-24T23:14:23Z","receivedAt":"2021-05-24T23:14:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The 1/2 here fixes a bug I introduced with an error message going\nmissing.\n\nThe 2/2 hopefully replaces\n<bb30fe2b-cd75-4782-24a6-08bb002a0367@kdbg.org>[1], but I have not\ntested it on Windows. I think improving the error message is better\nstrategy here than working around the current Git.pm abs_path()\nbehavior.\n\nThis has a trivial conflict with my outstanding [2] series which is\neasily solved, we just need to take the side introduced in this topic\n(i.e. abs_path isn't needed anymore).\n\nAlso, I said I'd CC Robert Foss in [3] but didn't, finally doing that\nhere.\n\n1. http://lore.kernel.org/git/bb30fe2b-cd75-4782-24a6-08bb002a0367@kdbg.org\n2. https://lore.kernel.org/git/cover-00.13-00000000000-20210524T074932Z-avarab@gmail.com\n3. http://lore.kernel.org/git/87im37ojrn.fsf@evledraar.gmail.com\n\nÆvar Arnfjörð Bjarmason (2):\n  send-email: fix missing error message regression\n  send-email: don't needlessly abs_path() the core.hooksPath\n\n git-send-email.perl   | 12 +++++++++++-\n perl/Git.pm           |  3 +--\n t/t9001-send-email.sh | 25 ++++++++++++++++++++++---\n 3 files changed, 34 insertions(+), 6 deletions(-)\n\n-- \n2.32.0.rc1.385.g9db524b96f7\n\n"},{"id":"425467","messageId":"patch-1.2-df3a2b8562d-20210524T231047Z-avarab@gmail.com","threadId":"55764","inReplyTo":"cover-0.2-00000000000-20210524T231047Z-avarab@gmail.com","subject":"[PATCH 1/2] send-email: fix missing error message regression","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-24T23:14:24Z","receivedAt":"2021-05-24T23:14:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a regression with the \"the editor exited uncleanly, aborting\neverything\" error message going missing after my\nd21616c0394 (git-send-email: refactor duplicate $? checks into a\nfunction, 2021-04-06).\n\nI introduced a $msg variable, but did not actually use it. This caused\nus to miss the optional error message supplied by the \"do_edit\"\ncodepath. Fix that, and add tests to check that this works.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl   | 12 +++++++++++-\n t/t9001-send-email.sh | 23 +++++++++++++++++++++--\n 2 files changed, 32 insertions(+), 3 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 175da07d946..170f42fe210 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -219,8 +219,18 @@ sub system_or_msg {\n \tmy $exit_code = $? >> 8;\n \treturn unless $signalled or $exit_code;\n \n+\tmy @sprintf_args = ($args->[0], $exit_code);\n+\tif (defined $msg) {\n+\t\t# Quiet the 'redundant' warning category, except we\n+\t\t# need to support down to Perl 5.8, so we can't do a\n+\t\t# \"no warnings 'redundant'\", since that category was\n+\t\t# introduced in perl 5.22, and asking for it will die\n+\t\t# on older perls.\n+\t\tno warnings;\n+\t\treturn sprintf($msg, @sprintf_args);\n+\t}\n \treturn sprintf(__(\"fatal: command '%s' died with exit code %d\"),\n-\t\t       $args->[0], $exit_code);\n+\t\t       @sprintf_args);\n }\n \n sub system_or_die {\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 65b30353719..2acf389837c 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -644,14 +644,33 @@ test_expect_success $PREREQ 'In-Reply-To with --chain-reply-to' '\n \ttest_cmp expect actual\n '\n \n+test_set_editor \"$(pwd)/fake-editor\"\n+\n+test_expect_success $PREREQ 'setup erroring fake editor' '\n+\twrite_script fake-editor <<-\\EOF\n+\techo >&2 \"I am about to error\"\n+\texit 1\n+\tEOF\n+'\n+\n+test_expect_success $PREREQ 'fake editor dies with error' '\n+\tclean_fake_sendmail &&\n+\ttest_must_fail git send-email \\\n+\t\t--compose --subject foo \\\n+\t\t--from=\"Example <nobody@example.com>\" \\\n+\t\t--to=nobody@example.com \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\t$patches 2>err &&\n+\tgrep \"I am about to error\" err &&\n+\tgrep \"the editor exited uncleanly, aborting everything\" err\n+'\n+\n test_expect_success $PREREQ 'setup fake editor' '\n \twrite_script fake-editor <<-\\EOF\n \techo fake edit >>\"$1\"\n \tEOF\n '\n \n-test_set_editor \"$(pwd)/fake-editor\"\n-\n test_expect_success $PREREQ '--compose works' '\n \tclean_fake_sendmail &&\n \tgit send-email \\\n-- \n2.32.0.rc1.385.g9db524b96f7\n\n"},{"id":"425468","messageId":"patch-2.2-d097e7b0b81-20210524T231047Z-avarab@gmail.com","threadId":"55764","inReplyTo":"cover-0.2-00000000000-20210524T231047Z-avarab@gmail.com","subject":"[PATCH 2/2] send-email: don't needlessly abs_path() the core.hooksPath","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-24T23:14:25Z","receivedAt":"2021-05-24T23:14:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In c8243933c74 (git-send-email: Respect core.hooksPath setting,\n2021-03-23) we started supporting core.hooksPath in \"send-email\". It's\nbeen reported that on Windows[1] doing this by calling abs_path()\nresults in different canonicalizations of the absolute path.\n\nThis wasn't an issue in c8243933c74 itself, but was revealed by my\nea7811b37e0 (git-send-email: improve --validate error output,\n2021-04-06) when we started emitting the path to the hook, which was\npreviously only internal to git-send-email.perl.\n\nI think this change should let us have our cake and eat it too. We now\nemit a relative path for the common case where the hook is in the\n.git/hooks directory, but in the case it's an absolute path (there's\nanother test for that, not seen here) we'll prefix it with $(pwd).\n\nI hope that unlike the current implementation that $(pwd) v.s. $PWD\ndifference won't matter on Windows, since now the absolute path is the\none we get from rev-parse, not the one that's been passed through\nPerl's Cwd::abs_path().\n\n1. http://lore.kernel.org/git/bb30fe2b-cd75-4782-24a6-08bb002a0367@kdbg.org\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n perl/Git.pm           | 3 +--\n t/t9001-send-email.sh | 2 +-\n 2 files changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 73ebbf80cc6..df6280ebab5 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -629,8 +629,7 @@ sub hooks_path {\n \tmy ($self) = @_;\n \n \tmy $dir = $self->command_oneline('rev-parse', '--git-path', 'hooks');\n-\tmy $abs = abs_path($dir);\n-\treturn $abs;\n+\treturn $dir;\n }\n \n =item wc_path ()\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 2acf389837c..3b7540050ca 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -539,7 +539,7 @@ test_expect_success $PREREQ \"--validate respects relative core.hooksPath path\" '\n \ttest_path_is_file my-hooks.ran &&\n \tcat >expect <<-EOF &&\n \tfatal: longline.patch: rejected by sendemail-validate hook\n-\tfatal: command '\"'\"'$(pwd)/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n+\tfatal: command '\"'\"'my-hooks/sendemail-validate'\"'\"' died with exit code 1\n \twarning: no patches were sent\n \tEOF\n \ttest_cmp expect actual\n-- \n2.32.0.rc1.385.g9db524b96f7\n\n"},{"id":"425469","messageId":"87cztfoh91.fsf@evledraar.gmail.com","threadId":"55764","inReplyTo":"87im37ojrn.fsf@evledraar.gmail.com","subject":"Re: [PATCH] t9001-send-email.sh: fix expected absolute paths on Windows","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-24T23:15:08Z","receivedAt":"2021-05-24T23:16:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 25 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> On Mon, May 24 2021, Johannes Sixt wrote:\n>\n> Also CC-ing Robert Foss <robert.foss@linaro.org>, I last touched this\n> code, but the fallout is ultimately from his c8243933c74\n> (git-send-email: Respect core.hooksPath setting, 2021-03-23).\n>\n>> Git for Windows is a native Windows program that works with native\n>> absolute paths in the drive letter style C:\\dir. The auxiliary\n>> infrastructure is based on MSYS2, which uses POSIX style /C/dir.\n>>\n>> When we test for output of absolute paths produced by git.exe, we\n>> usally have to expect C:\\dir style paths. To produce such expected\n>> paths, we have to use $(pwd) in the test scripts; the alternative,\n>> $PWD, produces a POSIX style path. ($PWD is a shell variable, and the\n>> shell is bash, an MSYS2 program, and operates in the POSIX realm.)\n>>\n>> There are two recently added tests that were written to expect C:\\dir\n>> paths. The output that is tested is produced by `git send-email`, but\n>> behind the scenes, this is a Perl script, which also works in the\n>> POSIX realm and produces /C/dir style output.\n>>\n>> In the first test case that is changed here, replace $(pwd) by $PWD\n>> so that the expected path is constructed using /C/dir style.\n>>\n>> The second test case sets core.hooksPath to an absolute path. Since\n>> the test script talks to native git.exe, it is supposed to place a\n>> C:/dir style path into the configuration; therefore, keep $(pwd).\n>> When this configuration value is consumed by the Perl script, it is\n>> transformed to /C/dir style by the MSYS2 layer and echoed back in\n>> this form in the error message. Hence, do use $PWD for the expected\n>> value.\n>>\n>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n>> ---\n>>  When I say \"the configuration is transformed to /C/dir style\", I am\n>>  actually hand-waving: I can observe that a transformation must\n>>  happen somewhere, but I actually do not know where the conversion\n>>  really happens. \"The MSYS2 layer\" is my best qualified guess.\n>>\n>>  t/t9001-send-email.sh | 7 +++----\n>>  1 file changed, 3 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n>> index 65b3035371..68bebc505b 100755\n>> --- a/t/t9001-send-email.sh\n>> +++ b/t/t9001-send-email.sh\n>> @@ -539,15 +539,14 @@ test_expect_success $PREREQ \"--validate respects relative core.hooksPath path\" '\n>>  \ttest_path_is_file my-hooks.ran &&\n>>  \tcat >expect <<-EOF &&\n>>  \tfatal: longline.patch: rejected by sendemail-validate hook\n>> -\tfatal: command '\"'\"'$(pwd)/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n>> +\tfatal: command '\"'\"'$PWD/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n>>  \twarning: no patches were sent\n>>  \tEOF\n>>  \ttest_cmp expect actual\n>>  '\n>>  \n>>  test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n>> -\thooks_path=\"$(pwd)/my-hooks\" &&\n>> -\ttest_config core.hooksPath \"$hooks_path\" &&\n>> +\ttest_config core.hooksPath \"$(pwd)/my-hooks\" &&\n>>  \ttest_when_finished \"rm my-hooks.ran\" &&\n>>  \ttest_must_fail git send-email \\\n>>  \t\t--from=\"Example <nobody@example.com>\" \\\n>> @@ -558,7 +557,7 @@ test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n>>  \ttest_path_is_file my-hooks.ran &&\n>>  \tcat >expect <<-EOF &&\n>>  \tfatal: longline.patch: rejected by sendemail-validate hook\n>> -\tfatal: command '\"'\"'$hooks_path/sendemail-validate'\"'\"' died with exit code 1\n>> +\tfatal: command '\"'\"'$PWD/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n>>  \twarning: no patches were sent\n>>  \tEOF\n>>  \ttest_cmp expect actual\n>\n> Does this alternate patch[1] also fix the issue? I don't have a Windows\n> system on which to test this, but it seems to me like it should.\n\nI've submitted that (in this thread) as\nhttps://lore.kernel.org/git/cover-0.2-00000000000-20210524T231047Z-avarab@gmail.com/,\nbut as noted here I haven't tested it on Windows, so testing if it works\nwould be most welcome...\n\n> I.e. the issue seems to me to me that we have an absolute path from\n> --git-path, and one of Cwd.pm's or File::Spec.pm's ideas of what that\n> absolute path should look like differs from ours.\n>\n> We have a sprinkle of File::Spec->file_name_is_absolute($dir) in Git.pm\n> for some other stuff to deal with the same scenario, but I don't see why\n> we need the abs_path() here at all.\n>\n> Either we have a relative path from \"rev-parse --git-dir hooks\", or an\n> absolute one, in either case we feed it to Perl's\n> system(\"some-relative-or-absolute-path\").\n>\n> I have a parallel series where I did some send-email changes by just\n> extracting the relevant code from Git.pm, since there were objections to\n> changing the \"public API\". But in this case there's been no release with\n> this, so presumably it's fine to just change it.\n>\n> 1.\n>\n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index 73ebbf80cc6..df6280ebab5 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -629,8 +629,7 @@ sub hooks_path {\n>  \tmy ($self) = @_;\n>  \n>  \tmy $dir = $self->command_oneline('rev-parse', '--git-path', 'hooks');\n> -\tmy $abs = abs_path($dir);\n> -\treturn $abs;\n> +\treturn $dir;\n>  }\n>  \n>  =item wc_path ()\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> index 65b30353719..9c518462c3e 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -539,7 +539,7 @@ test_expect_success $PREREQ \"--validate respects relative core.hooksPath path\" '\n>  \ttest_path_is_file my-hooks.ran &&\n>  \tcat >expect <<-EOF &&\n>  \tfatal: longline.patch: rejected by sendemail-validate hook\n> -\tfatal: command '\"'\"'$(pwd)/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n> +\tfatal: command '\"'\"'my-hooks/sendemail-validate'\"'\"' died with exit code 1\n>  \twarning: no patches were sent\n>  \tEOF\n>  \ttest_cmp expect actual\n\n"},{"id":"425475","messageId":"xmqqh7iripzg.fsf@gitster.g","threadId":"55764","inReplyTo":"patch-2.2-d097e7b0b81-20210524T231047Z-avarab@gmail.com","subject":"Re: [PATCH 2/2] send-email: don't needlessly abs_path() the core.hooksPath","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-25T01:03:47Z","receivedAt":"2021-05-25T01:03:53Z","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> In c8243933c74 (git-send-email: Respect core.hooksPath setting,\n> 2021-03-23) we started supporting core.hooksPath in \"send-email\". It's\n> been reported that on Windows[1] doing this by calling abs_path()\n> results in different canonicalizations of the absolute path.\n\nI see the author of that patch CC'ed; the change in question\nexplains why we switched from \"the hooks directory immediately under\n$repo->repo_path()\" to \"ask 'rev-parse --git-path hooks'\", but it\ndoes not say why we call abs_path() on the result.  I guess that is\nbecause $repo->repo_path() has always been a result of applying the\nabs_path() function to something, so it was to safeguard the callers\nthat expect an absolute path coming back from hooks_path?\n\nAnd that makes this change dubious, especially as a band-aid for a\nbreakage immediately before the final release, doesn't it?  Are we\nconvinced that the callers are OK with seeing sometimes relative\npaths?  Certainly the cases the tests J6t fixed are not negatively\naffected, but is that sufficient?  To what directory is the\nconfiguration variable supposed to be relative to, and are we sure\nthat the user will always invoke \"git send-email\" from that\ndirectory?\n\nPuzzled.\n\n"},{"id":"425486","messageId":"CAG3jFyv716SC1wQUcQxCdU=J-W2hYo8=tA65VZ5jqmhpQ7G+=Q@mail.gmail.com","threadId":"55764","inReplyTo":"xmqqh7iripzg.fsf@gitster.g","subject":"Re: [PATCH 2/2] send-email: don't needlessly abs_path() the core.hooksPath","fromName":"Robert Foss","fromEmail":"robert.foss@linaro.org","sentAt":"2021-05-25T06:10:14Z","receivedAt":"2021-05-25T06:10:27Z","isPatch":true,"sender":{"key":"robert.foss@linaro.org","avatar":"https://avatars.githubusercontent.com/u/523260?v=4"},"body":"Hey\n\nOn Tue, 25 May 2021 at 03:03, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n> > In c8243933c74 (git-send-email: Respect core.hooksPath setting,\n> > 2021-03-23) we started supporting core.hooksPath in \"send-email\". It's\n> > been reported that on Windows[1] doing this by calling abs_path()\n> > results in different canonicalizations of the absolute path.\n>\n> I see the author of that patch CC'ed; the change in question\n> explains why we switched from \"the hooks directory immediately under\n> $repo->repo_path()\" to \"ask 'rev-parse --git-path hooks'\", but it\n> does not say why we call abs_path() on the result.  I guess that is\n> because $repo->repo_path() has always been a result of applying the\n> abs_path() function to something, so it was to safeguard the callers\n> that expect an absolute path coming back from hooks_path?\n\nI don't think I have a good reason why abs_path() was used, most\nlikely it was copied from some other snippet and kept for uniformity.\n\n>\n> And that makes this change dubious, especially as a band-aid for a\n> breakage immediately before the final release, doesn't it?  Are we\n> convinced that the callers are OK with seeing sometimes relative\n> paths?  Certainly the cases the tests J6t fixed are not negatively\n> affected, but is that sufficient?  To what directory is the\n> configuration variable supposed to be relative to, and are we sure\n> that the user will always invoke \"git send-email\" from that\n> directory?\n\nPreviously this functionality was entirely not working, so I don't\nthink we'll have any regressions. With that being said I'm unable to\ntest that it works well on windows.\n\nAs far as what the expected norms for paths are within git, I really\ndon't have any answers.\n\n>\n> Puzzled.\n>\n"},{"id":"425487","messageId":"874kerny0e.fsf@evledraar.gmail.com","threadId":"55764","inReplyTo":"xmqqh7iripzg.fsf@gitster.g","subject":"Re: [PATCH 2/2] send-email: don't needlessly abs_path() the core.hooksPath","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-25T05:57:37Z","receivedAt":"2021-05-25T06:11:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 25 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> In c8243933c74 (git-send-email: Respect core.hooksPath setting,\n>> 2021-03-23) we started supporting core.hooksPath in \"send-email\". It's\n>> been reported that on Windows[1] doing this by calling abs_path()\n>> results in different canonicalizations of the absolute path.\n>\n> I see the author of that patch CC'ed; the change in question\n> explains why we switched from \"the hooks directory immediately under\n> $repo->repo_path()\" to \"ask 'rev-parse --git-path hooks'\", but it\n> does not say why we call abs_path() on the result.  I guess that is\n> because $repo->repo_path() has always been a result of applying the\n> abs_path() function to something, so it was to safeguard the callers\n> that expect an absolute path coming back from hooks_path?\n>\n> And that makes this change dubious, especially as a band-aid for a\n> breakage immediately before the final release, doesn't it?  Are we\n> convinced that the callers are OK with seeing sometimes relative\n> paths?  Certainly the cases the tests J6t fixed are not negatively\n> affected, but is that sufficient?  To what directory is the\n> configuration variable supposed to be relative to, and are we sure\n> that the user will always invoke \"git send-email\" from that\n> directory?\n\nThe one caller is git-send-email.perl is fine with it, at least on *nix,\nthis fix still needs testing on Windows.\n\nThe repo_path() function was introduced in c8243933c74, so it's never\nbeen in a release, thus I think it's fine to alter its behavior.\n\nThe code here doesn't need to concern itself with what needs to be\nrelative to what, you run send-email in some working tree directory (or\ntop-level), and depending on core.hooksPath we'll either return a\nrelative path to the .git/hooks or an absolute one, the system()\ninvocation will accept either.\n"},{"id":"425488","messageId":"871r9vnxtb.fsf@evledraar.gmail.com","threadId":"55764","inReplyTo":"874kerny0e.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] send-email: don't needlessly abs_path() the core.hooksPath","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-25T06:13:52Z","receivedAt":"2021-05-25T06:15:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 25 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> On Tue, May 25 2021, Junio C Hamano wrote:\n>\n>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>>\n>>> In c8243933c74 (git-send-email: Respect core.hooksPath setting,\n>>> 2021-03-23) we started supporting core.hooksPath in \"send-email\". It's\n>>> been reported that on Windows[1] doing this by calling abs_path()\n>>> results in different canonicalizations of the absolute path.\n>>\n>> I see the author of that patch CC'ed; the change in question\n>> explains why we switched from \"the hooks directory immediately under\n>> $repo->repo_path()\" to \"ask 'rev-parse --git-path hooks'\", but it\n>> does not say why we call abs_path() on the result.  I guess that is\n>> because $repo->repo_path() has always been a result of applying the\n>> abs_path() function to something, so it was to safeguard the callers\n>> that expect an absolute path coming back from hooks_path?\n>>\n>> And that makes this change dubious, especially as a band-aid for a\n>> breakage immediately before the final release, doesn't it?  Are we\n>> convinced that the callers are OK with seeing sometimes relative\n>> paths?  Certainly the cases the tests J6t fixed are not negatively\n>> affected, but is that sufficient?  To what directory is the\n>> configuration variable supposed to be relative to, and are we sure\n>> that the user will always invoke \"git send-email\" from that\n>> directory?\n>\n> The one caller is git-send-email.perl is fine with it, at least on *nix,\n> this fix still needs testing on Windows.\n>\n> The repo_path() function was introduced in c8243933c74, so it's never\n> been in a release, thus I think it's fine to alter its behavior.\n>\n> The code here doesn't need to concern itself with what needs to be\n> relative to what, you run send-email in some working tree directory (or\n> top-level), and depending on core.hooksPath we'll either return a\n> relative path to the .git/hooks or an absolute one, the system()\n> invocation will accept either.\n\n...I think the one issue with my 2/2 is that it doesn't go far enough,\nwe should just remove the repo_path() from Git.pm and instead use its:\n\n    $self->command_oneline('rev-parse', '--git-path', 'hooks')\n\n...in the one user in git-send-email.perl, as discussed in other\nserieses Git.pm is \"public\", so stuff we stick in there we can't\nalter. In this case we're doing so before a release, and nobody wanted\nthis except git-send-email.perl.\n\nThat caller is likely to just go away if and when Emily's \"git hook run\"\nlands, so I think it would be best (but not strictly needed for the\npre-rc fix) to just remove that API.\n\nWhat do you / Robert Foss think (maybe he wanted this for something\nelse...)?\n"},{"id":"425489","messageId":"xmqq35ubgwpy.fsf@gitster.g","threadId":"55764","inReplyTo":"874kerny0e.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] send-email: don't needlessly abs_path() the core.hooksPath","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-25T06:21:13Z","receivedAt":"2021-05-25T06:21:26Z","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> The repo_path() function was introduced in c8243933c74, so it's never\n> been in a release, thus I think it's fine to alter its behavior.\n>\n> The code here doesn't need to concern itself with what needs to be\n> relative to what, you run send-email in some working tree directory (or\n> top-level), and depending on core.hooksPath we'll either return a\n> relative path to the .git/hooks or an absolute one, the system()\n> invocation will accept either.\n\nAll of that are convincing, but I'd rather not risk it.  I'll take\nyour 1/2 with J6t's fix, both of which are obvious and no brainer,\nbut I am willing to take this 2/2 as a post-release clean-up next\nmonght, though ;-)\n\nThanks.\n"},{"id":"425490","messageId":"CAG3jFyu8AzSJS2EZwYKD9YKiewLVQLeKZZYEvUobyV+a6_VsDw@mail.gmail.com","threadId":"55764","inReplyTo":"871r9vnxtb.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] send-email: don't needlessly abs_path() the core.hooksPath","fromName":"Robert Foss","fromEmail":"robert.foss@linaro.org","sentAt":"2021-05-25T06:21:35Z","receivedAt":"2021-05-25T06:21:49Z","isPatch":true,"sender":{"key":"robert.foss@linaro.org","avatar":"https://avatars.githubusercontent.com/u/523260?v=4"},"body":"On Tue, 25 May 2021 at 08:15, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n>\n> On Tue, May 25 2021, Ævar Arnfjörð Bjarmason wrote:\n>\n> > On Tue, May 25 2021, Junio C Hamano wrote:\n> >\n> >> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n> >>\n> >>> In c8243933c74 (git-send-email: Respect core.hooksPath setting,\n> >>> 2021-03-23) we started supporting core.hooksPath in \"send-email\". It's\n> >>> been reported that on Windows[1] doing this by calling abs_path()\n> >>> results in different canonicalizations of the absolute path.\n> >>\n> >> I see the author of that patch CC'ed; the change in question\n> >> explains why we switched from \"the hooks directory immediately under\n> >> $repo->repo_path()\" to \"ask 'rev-parse --git-path hooks'\", but it\n> >> does not say why we call abs_path() on the result.  I guess that is\n> >> because $repo->repo_path() has always been a result of applying the\n> >> abs_path() function to something, so it was to safeguard the callers\n> >> that expect an absolute path coming back from hooks_path?\n> >>\n> >> And that makes this change dubious, especially as a band-aid for a\n> >> breakage immediately before the final release, doesn't it?  Are we\n> >> convinced that the callers are OK with seeing sometimes relative\n> >> paths?  Certainly the cases the tests J6t fixed are not negatively\n> >> affected, but is that sufficient?  To what directory is the\n> >> configuration variable supposed to be relative to, and are we sure\n> >> that the user will always invoke \"git send-email\" from that\n> >> directory?\n> >\n> > The one caller is git-send-email.perl is fine with it, at least on *nix,\n> > this fix still needs testing on Windows.\n> >\n> > The repo_path() function was introduced in c8243933c74, so it's never\n> > been in a release, thus I think it's fine to alter its behavior.\n> >\n> > The code here doesn't need to concern itself with what needs to be\n> > relative to what, you run send-email in some working tree directory (or\n> > top-level), and depending on core.hooksPath we'll either return a\n> > relative path to the .git/hooks or an absolute one, the system()\n> > invocation will accept either.\n>\n> ...I think the one issue with my 2/2 is that it doesn't go far enough,\n> we should just remove the repo_path() from Git.pm and instead use its:\n>\n>     $self->command_oneline('rev-parse', '--git-path', 'hooks')\n\nThis looks good to me.\n\n>\n> ...in the one user in git-send-email.perl, as discussed in other\n> serieses Git.pm is \"public\", so stuff we stick in there we can't\n> alter. In this case we're doing so before a release, and nobody wanted\n> this except git-send-email.perl.\n>\n> That caller is likely to just go away if and when Emily's \"git hook run\"\n> lands, so I think it would be best (but not strictly needed for the\n> pre-rc fix) to just remove that API.\n\nThat sounds like a sane approach. If core.hooksPath is needed in more\nlocations in the future, it can be added then.\n\n>\n> What do you / Robert Foss think (maybe he wanted this for something\n> else...)?\n"},{"id":"425522","messageId":"87v977m2q6.fsf@evledraar.gmail.com","threadId":"55764","inReplyTo":"xmqq35ubgwpy.fsf@gitster.g","subject":"Re: [PATCH 2/2] send-email: don't needlessly abs_path() the core.hooksPath","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-25T12:09:35Z","receivedAt":"2021-05-25T12:12:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 25 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>> The repo_path() function was introduced in c8243933c74, so it's never\n>> been in a release, thus I think it's fine to alter its behavior.\n>>\n>> The code here doesn't need to concern itself with what needs to be\n>> relative to what, you run send-email in some working tree directory (or\n>> top-level), and depending on core.hooksPath we'll either return a\n>> relative path to the .git/hooks or an absolute one, the system()\n>> invocation will accept either.\n>\n> All of that are convincing, but I'd rather not risk it.  I'll take\n> your 1/2 with J6t's fix, both of which are obvious and no brainer,\n> but I am willing to take this 2/2 as a post-release clean-up next\n> monght, though ;-)\n\nFair enough. I get that we're in the rc phase, but FWIW I still\ndisagree.\n\nI do think supporting whatever edge case we're exposing by\nround-tripping through abs_path() and its subtly different behavior is\nworse than a change to make us rely on the well-understood rev-parse\nbehavior directly.\n\nEspecially since the fix is to something that didn't work to begin with\nbefore the v2.32.0 release is out (core.hooksPath + send-email), but\nperhaps that's more of an argument for reverting the topic during RC +\ntry to have it land in v2.33.0?\n\nAnd that + what seems to be the prevailing (IMO nonsensical) opinion on\nGit.pm's stability promise, meaning that once this new function is out\nin a release we'll be stuck supporting it makes me want to change this\npre-release.\n\nBut I'll leave it to you, if you are convinced and do want to take this\n2/2 after all I'll submit another trivial patch on top to remove the new\n(then unused) repo_path function, which we expect to go away anyway.\n"},{"id":"425533","messageId":"xmqq5yz6ehpk.fsf@gitster.g","threadId":"55764","inReplyTo":"87v977m2q6.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] send-email: don't needlessly abs_path() the core.hooksPath","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-25T19:28:23Z","receivedAt":"2021-05-25T19:28:29Z","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> But I'll leave it to you, if you are convinced and do want to take this\n> 2/2 after all I'll submit another trivial patch on top to remove the new\n> (then unused) repo_path function, which we expect to go away anyway.\n\nSounds good.\n"},{"id":"425544","messageId":"60ada2dac06de_2c7f620854@natae.notmuch","threadId":"55764","inReplyTo":"874kerny0e.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] send-email: don't needlessly abs_path() the core.hooksPath","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-05-26T01:22:34Z","receivedAt":"2021-05-26T01:22:44Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Tue, May 25 2021, Junio C Hamano wrote:\n> \n> > Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n> >\n> >> In c8243933c74 (git-send-email: Respect core.hooksPath setting,\n> >> 2021-03-23) we started supporting core.hooksPath in \"send-email\". It's\n> >> been reported that on Windows[1] doing this by calling abs_path()\n> >> results in different canonicalizations of the absolute path.\n> >\n> > I see the author of that patch CC'ed; the change in question\n> > explains why we switched from \"the hooks directory immediately under\n> > $repo->repo_path()\" to \"ask 'rev-parse --git-path hooks'\", but it\n> > does not say why we call abs_path() on the result.  I guess that is\n> > because $repo->repo_path() has always been a result of applying the\n> > abs_path() function to something, so it was to safeguard the callers\n> > that expect an absolute path coming back from hooks_path?\n> >\n> > And that makes this change dubious, especially as a band-aid for a\n> > breakage immediately before the final release, doesn't it?  Are we\n> > convinced that the callers are OK with seeing sometimes relative\n> > paths?  Certainly the cases the tests J6t fixed are not negatively\n> > affected, but is that sufficient?  To what directory is the\n> > configuration variable supposed to be relative to, and are we sure\n> > that the user will always invoke \"git send-email\" from that\n> > directory?\n> \n> The one caller is git-send-email.perl is fine with it, at least on *nix,\n> this fix still needs testing on Windows.\n\nWhile I understand the reluctance to test things on Windows, sometimes we\nwaste more time discussing about what couldn't potentially break there\nthan the time it takes to setup a VM and just make sure.\n\nAm I the only one that has setup a Windows VM just for tests?\n\nAlso, there's ways to trigger CI runs to test this stuff aren't there?\n\nSeems like a much less painful way to leapfrog this roadblock.\n\nCheers.\n\n-- \nFelipe Contreras"},{"id":"425564","messageId":"patch-1.2-ff01d4619ea-20210526T111558Z-avarab@gmail.com","threadId":"55764","inReplyTo":"cover-0.2-00000000000-20210526T111558Z-avarab@gmail.com","subject":"[PATCH v2 1/2] send-email: don't needlessly abs_path() the core.hooksPath","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-26T11:21:06Z","receivedAt":"2021-05-26T11:21:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In c8243933c74 (git-send-email: Respect core.hooksPath setting,\n2021-03-23) we started supporting core.hooksPath in \"send-email\". It's\nbeen reported that on Windows[1] doing this by calling abs_path()\nresults in different canonicalizations of the absolute path.\n\nThis wasn't an issue in c8243933c74 itself, but was revealed by my\nea7811b37e0 (git-send-email: improve --validate error output,\n2021-04-06) when we started emitting the path to the hook, which was\npreviously only internal to git-send-email.perl.\n\nThe just-landed 53753a37d09 (t9001-send-email.sh: fix expected\nabsolute paths on Windows, 2021-05-24) narrowly fixed this issue, but\nI believe we can do better here. We should not be relying on whatever\nchanges Perl's abs_path() makes to the path \"rev-parse --git-path\nhooks\" hands to us. Let's instead trust it, and hand it to Perl's\nsystem() in git-send-email.perl. It will handle either a relative or\nabsolute path.\n\nSo let's revert most of 53753a37d09 and just have \"hooks_path\" return\nwhat we get from \"rev-parse\" directly without modification. This has\nthe added benefit of making the error message friendlier in the common\ncase, we'll no longer print an absolute path for repository-local hook\nerrors.\n\n1. http://lore.kernel.org/git/bb30fe2b-cd75-4782-24a6-08bb002a0367@kdbg.org\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n perl/Git.pm           | 3 +--\n t/t9001-send-email.sh | 7 ++++---\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 73ebbf80cc6..df6280ebab5 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -629,8 +629,7 @@ sub hooks_path {\n \tmy ($self) = @_;\n \n \tmy $dir = $self->command_oneline('rev-parse', '--git-path', 'hooks');\n-\tmy $abs = abs_path($dir);\n-\treturn $abs;\n+\treturn $dir;\n }\n \n =item wc_path ()\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex aa603cf4d07..3b7540050ca 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -539,14 +539,15 @@ test_expect_success $PREREQ \"--validate respects relative core.hooksPath path\" '\n \ttest_path_is_file my-hooks.ran &&\n \tcat >expect <<-EOF &&\n \tfatal: longline.patch: rejected by sendemail-validate hook\n-\tfatal: command '\"'\"'$PWD/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n+\tfatal: command '\"'\"'my-hooks/sendemail-validate'\"'\"' died with exit code 1\n \twarning: no patches were sent\n \tEOF\n \ttest_cmp expect actual\n '\n \n test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n-\ttest_config core.hooksPath \"$(pwd)/my-hooks\" &&\n+\thooks_path=\"$(pwd)/my-hooks\" &&\n+\ttest_config core.hooksPath \"$hooks_path\" &&\n \ttest_when_finished \"rm my-hooks.ran\" &&\n \ttest_must_fail git send-email \\\n \t\t--from=\"Example <nobody@example.com>\" \\\n@@ -557,7 +558,7 @@ test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n \ttest_path_is_file my-hooks.ran &&\n \tcat >expect <<-EOF &&\n \tfatal: longline.patch: rejected by sendemail-validate hook\n-\tfatal: command '\"'\"'$PWD/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n+\tfatal: command '\"'\"'$hooks_path/sendemail-validate'\"'\"' died with exit code 1\n \twarning: no patches were sent\n \tEOF\n \ttest_cmp expect actual\n-- \n2.32.0.rc1.400.g0a5a93401d3\n\n"},{"id":"425565","messageId":"cover-0.2-00000000000-20210526T111558Z-avarab@gmail.com","threadId":"55764","inReplyTo":"xmqq5yz6ehpk.fsf@gitster.g","subject":"[PATCH v2 0/2] send-email: don't needlessly abs_path() the core.hooksPath","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-26T11:21:05Z","receivedAt":"2021-05-26T11:21:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, May 26 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>> But I'll leave it to you, if you are convinced and do want to take this\n>> 2/2 after all I'll submit another trivial patch on top to remove the new\n>> (then unused) repo_path function, which we expect to go away anyway.\n>\n> Sounds good.\n\nHere that is, now on top of the new \"master\" which has the\nt9001-send-email.sh changes you'd already merged down.\n\nÆvar Arnfjörð Bjarmason (2):\n  send-email: don't needlessly abs_path() the core.hooksPath\n  send-email: move \"hooks_path\" invocation to git-send-email.perl\n\n git-send-email.perl   |  3 ++-\n perl/Git.pm           | 13 -------------\n t/t9001-send-email.sh |  7 ++++---\n 3 files changed, 6 insertions(+), 17 deletions(-)\n\nRange-diff against v1:\n1:  df3a2b8562d < -:  ----------- send-email: fix missing error message regression\n2:  d097e7b0b81 ! 1:  ff01d4619ea send-email: don't needlessly abs_path() the core.hooksPath\n    @@ Commit message\n         2021-04-06) when we started emitting the path to the hook, which was\n         previously only internal to git-send-email.perl.\n     \n    -    I think this change should let us have our cake and eat it too. We now\n    -    emit a relative path for the common case where the hook is in the\n    -    .git/hooks directory, but in the case it's an absolute path (there's\n    -    another test for that, not seen here) we'll prefix it with $(pwd).\n    +    The just-landed 53753a37d09 (t9001-send-email.sh: fix expected\n    +    absolute paths on Windows, 2021-05-24) narrowly fixed this issue, but\n    +    I believe we can do better here. We should not be relying on whatever\n    +    changes Perl's abs_path() makes to the path \"rev-parse --git-path\n    +    hooks\" hands to us. Let's instead trust it, and hand it to Perl's\n    +    system() in git-send-email.perl. It will handle either a relative or\n    +    absolute path.\n     \n    -    I hope that unlike the current implementation that $(pwd) v.s. $PWD\n    -    difference won't matter on Windows, since now the absolute path is the\n    -    one we get from rev-parse, not the one that's been passed through\n    -    Perl's Cwd::abs_path().\n    +    So let's revert most of 53753a37d09 and just have \"hooks_path\" return\n    +    what we get from \"rev-parse\" directly without modification. This has\n    +    the added benefit of making the error message friendlier in the common\n    +    case, we'll no longer print an absolute path for repository-local hook\n    +    errors.\n     \n         1. http://lore.kernel.org/git/bb30fe2b-cd75-4782-24a6-08bb002a0367@kdbg.org\n     \n    @@ t/t9001-send-email.sh: test_expect_success $PREREQ \"--validate respects relative\n      \ttest_path_is_file my-hooks.ran &&\n      \tcat >expect <<-EOF &&\n      \tfatal: longline.patch: rejected by sendemail-validate hook\n    --\tfatal: command '\"'\"'$(pwd)/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n    +-\tfatal: command '\"'\"'$PWD/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n     +\tfatal: command '\"'\"'my-hooks/sendemail-validate'\"'\"' died with exit code 1\n      \twarning: no patches were sent\n      \tEOF\n      \ttest_cmp expect actual\n    + '\n    + \n    + test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n    +-\ttest_config core.hooksPath \"$(pwd)/my-hooks\" &&\n    ++\thooks_path=\"$(pwd)/my-hooks\" &&\n    ++\ttest_config core.hooksPath \"$hooks_path\" &&\n    + \ttest_when_finished \"rm my-hooks.ran\" &&\n    + \ttest_must_fail git send-email \\\n    + \t\t--from=\"Example <nobody@example.com>\" \\\n    +@@ t/t9001-send-email.sh: test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n    + \ttest_path_is_file my-hooks.ran &&\n    + \tcat >expect <<-EOF &&\n    + \tfatal: longline.patch: rejected by sendemail-validate hook\n    +-\tfatal: command '\"'\"'$PWD/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n    ++\tfatal: command '\"'\"'$hooks_path/sendemail-validate'\"'\"' died with exit code 1\n    + \twarning: no patches were sent\n    + \tEOF\n    + \ttest_cmp expect actual\n-:  ----------- > 2:  cda41c36772 send-email: move \"hooks_path\" invocation to git-send-email.perl\n-- \n2.32.0.rc1.400.g0a5a93401d3\n\n"},{"id":"425566","messageId":"patch-2.2-cda41c36772-20210526T111558Z-avarab@gmail.com","threadId":"55764","inReplyTo":"cover-0.2-00000000000-20210526T111558Z-avarab@gmail.com","subject":"[PATCH v2 2/2] send-email: move \"hooks_path\" invocation to git-send-email.perl","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-26T11:21:07Z","receivedAt":"2021-05-26T11:22:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the newly added \"hooks_path\" API in Git.pm to its only user in\ngit-send-email.perl. This was added in c8243933c74 (git-send-email:\nRespect core.hooksPath setting, 2021-03-23), meaning that it hasn't\nyet made it into a non-rc release of git.\n\nThe consensus with Git.pm is that we need to be considerate of\nout-of-tree users who treat it as a public documented interface. We\nshould therefore be less willing to add new functionality to it, least\nwe be stuck supporting it after our own uses for it disappear.\n\nIn this case the git-send-email.perl hook invocation will probably be\nreplaced by a future \"git hook run\" command, and in the commit\npreceding this one the \"hooks_path\" become nothing but a trivial\nwrapper for \"rev-parse --git-path hooks\" anyway (with no\nCwd::abs_path() call), so let's just inline this command in\ngit-send-email.perl itself.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl |  3 ++-\n perl/Git.pm         | 12 ------------\n 2 files changed, 2 insertions(+), 13 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 170f42fe210..25be2ebd2af 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1959,7 +1959,8 @@ sub validate_patch {\n \tmy ($fn, $xfer_encoding) = @_;\n \n \tif ($repo) {\n-\t\tmy $validate_hook = catfile($repo->hooks_path(),\n+\t\tmy $hooks_path = $repo->command_oneline('rev-parse', '--git-path', 'hooks');\n+\t\tmy $validate_hook = catfile($hooks_path,\n \t\t\t\t\t    'sendemail-validate');\n \t\tmy $hook_error;\n \t\tif (-x $validate_hook) {\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex df6280ebab5..02eacef0c2a 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -619,18 +619,6 @@ sub _prompt {\n \n sub repo_path { $_[0]->{opts}->{Repository} }\n \n-=item hooks_path ()\n-\n-Return path to the hooks directory. Must be called on a repository instance.\n-\n-=cut\n-\n-sub hooks_path {\n-\tmy ($self) = @_;\n-\n-\tmy $dir = $self->command_oneline('rev-parse', '--git-path', 'hooks');\n-\treturn $dir;\n-}\n \n =item wc_path ()\n \n-- \n2.32.0.rc1.400.g0a5a93401d3\n\n"},{"id":"426226","messageId":"nycvar.QRO.7.76.6.2106021330470.55@tvgsbejvaqbjf.bet","threadId":"55764","inReplyTo":"bb30fe2b-cd75-4782-24a6-08bb002a0367@kdbg.org","subject":"Re: [PATCH] t9001-send-email.sh: fix expected absolute paths on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-06-02T11:40:44Z","receivedAt":"2021-06-02T11:40:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Mon, 24 May 2021, Johannes Sixt wrote:\n\n> Git for Windows is a native Windows program that works with native\n> absolute paths in the drive letter style C:\\dir. The auxiliary\n> infrastructure is based on MSYS2, which uses POSIX style /C/dir.\n\nAs far as I remember, VMS is also POSIX, and it has a different path\nstyle. Therefore I would probably use the term \"Unix style\" here instead\nof \"POSIX style\".\n\nBut that has nothing to do with the validity of your point: it is still\na correct and important observation.\n\n> When we test for output of absolute paths produced by git.exe, we\n> usally have to expect C:\\dir style paths. To produce such expected\n> paths, we have to use $(pwd) in the test scripts; the alternative,\n> $PWD, produces a POSIX style path. ($PWD is a shell variable, and the\n> shell is bash, an MSYS2 program, and operates in the POSIX realm.)\n>\n> There are two recently added tests that were written to expect C:\\dir\n> paths. The output that is tested is produced by `git send-email`, but\n> behind the scenes, this is a Perl script, which also works in the\n> POSIX realm and produces /C/dir style output.\n>\n> In the first test case that is changed here, replace $(pwd) by $PWD\n> so that the expected path is constructed using /C/dir style.\n>\n> The second test case sets core.hooksPath to an absolute path. Since\n> the test script talks to native git.exe, it is supposed to place a\n> C:/dir style path into the configuration; therefore, keep $(pwd).\n> When this configuration value is consumed by the Perl script, it is\n> transformed to /C/dir style by the MSYS2 layer and echoed back in\n> this form in the error message. Hence, do use $PWD for the expected\n> value.\n>\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n>  When I say \"the configuration is transformed to /C/dir style\", I am\n>  actually hand-waving: I can observe that a transformation must\n>  happen somewhere, but I actually do not know where the conversion\n>  really happens. \"The MSYS2 layer\" is my best qualified guess.\n\nIndeed, it is the MSYS2 runtime that performs this conversion. Concretely,\nwhenever you call a non-MSYS2 program from within MSYS2, command-line\narguments that look like Unix paths are converted by replacing forward\nslashes with backslashes and by prefixing absolute paths with MSYS2' root\ndirectory (as a Windows style path, of course).\n\nHowever, in this instance, it is a different problem, I think.\n\nPerl cannot handle Windows style paths. At least _Git's_ Perl scripts\ncannot.\n\nFor example, the `PATH` variable is assumed to contain colon-separated\ndirectory paths in our scripts. But that is not true on Windows: the colon\nalready separates the drive letter from the rest of the path, and\ntherefore the separator used in `PATH` is a _semicolon_.\n\nTo help with this, the MSYS2 runtime converts the command-line arguments\nand environment variables that look like path lists (such as `PATH`) and\npaths (such as `SYSTEMROOT`) from Windows style to Unix style when it\ndetects that, say, MSYS2's Perl is started from a non-MSYS2 program such\nas `git.exe`.\n\nWhich means that the Perl code executed in Ævar's tests spits out Unix\nstyle paths.\n\nHappily for us, the MSYS2 Bash with which Git's test suite is expected to\nbe executed on Windows understands those Unix style paths very well! All\nwe need to do is to use them here, and that is what your patch does,\ntherefore:\n\nAcked-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThank you,\nDscho\n\n>\n>  t/t9001-send-email.sh | 7 +++----\n>  1 file changed, 3 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> index 65b3035371..68bebc505b 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -539,15 +539,14 @@ test_expect_success $PREREQ \"--validate respects relative core.hooksPath path\" '\n>  \ttest_path_is_file my-hooks.ran &&\n>  \tcat >expect <<-EOF &&\n>  \tfatal: longline.patch: rejected by sendemail-validate hook\n> -\tfatal: command '\"'\"'$(pwd)/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n> +\tfatal: command '\"'\"'$PWD/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n>  \twarning: no patches were sent\n>  \tEOF\n>  \ttest_cmp expect actual\n>  '\n>\n>  test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n> -\thooks_path=\"$(pwd)/my-hooks\" &&\n> -\ttest_config core.hooksPath \"$hooks_path\" &&\n> +\ttest_config core.hooksPath \"$(pwd)/my-hooks\" &&\n>  \ttest_when_finished \"rm my-hooks.ran\" &&\n>  \ttest_must_fail git send-email \\\n>  \t\t--from=\"Example <nobody@example.com>\" \\\n> @@ -558,7 +557,7 @@ test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n>  \ttest_path_is_file my-hooks.ran &&\n>  \tcat >expect <<-EOF &&\n>  \tfatal: longline.patch: rejected by sendemail-validate hook\n> -\tfatal: command '\"'\"'$hooks_path/sendemail-validate'\"'\"' died with exit code 1\n> +\tfatal: command '\"'\"'$PWD/my-hooks/sendemail-validate'\"'\"' died with exit code 1\n>  \twarning: no patches were sent\n>  \tEOF\n>  \ttest_cmp expect actual\n> --\n> 2.31.0.152.g120726e270\n>\n>\n"}]}