| Message ID | patch-2.2-d097e7b0b81-20210524T231047Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | send-email: pre-release fixes for v2.32.0 | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > In c8243933c74 (git-send-email: Respect core.hooksPath setting, > 2021-03-23) we started supporting core.hooksPath in "send-email". It's > been reported that on Windows[1] doing this by calling abs_path() > results in different canonicalizations of the absolute path. I see the author of that patch CC'ed; the change in question explains why we switched from "the hooks directory immediately under $repo->repo_path()" to "ask 'rev-parse --git-path hooks'", but it does not say why we call abs_path() on the result. I guess that is because $repo->repo_path() has always been a result of applying the abs_path() function to something, so it was to safeguard the callers that expect an absolute path coming back from hooks_path? And that makes this change dubious, especially as a band-aid for a breakage immediately before the final release, doesn't it? Are we convinced that the callers are OK with seeing sometimes relative paths? Certainly the cases the tests J6t fixed are not negatively affected, but is that sufficient? To what directory is the configuration variable supposed to be relative to, and are we sure that the user will always invoke "git send-email" from that directory? Puzzled.
On Tue, May 25 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> In c8243933c74 (git-send-email: Respect core.hooksPath setting, >> 2021-03-23) we started supporting core.hooksPath in "send-email". It's >> been reported that on Windows[1] doing this by calling abs_path() >> results in different canonicalizations of the absolute path. > > I see the author of that patch CC'ed; the change in question > explains why we switched from "the hooks directory immediately under > $repo->repo_path()" to "ask 'rev-parse --git-path hooks'", but it > does not say why we call abs_path() on the result. I guess that is > because $repo->repo_path() has always been a result of applying the > abs_path() function to something, so it was to safeguard the callers > that expect an absolute path coming back from hooks_path? > > And that makes this change dubious, especially as a band-aid for a > breakage immediately before the final release, doesn't it? Are we > convinced that the callers are OK with seeing sometimes relative > paths? Certainly the cases the tests J6t fixed are not negatively > affected, but is that sufficient? To what directory is the > configuration variable supposed to be relative to, and are we sure > that the user will always invoke "git send-email" from that > directory? The one caller is git-send-email.perl is fine with it, at least on *nix, this fix still needs testing on Windows. The repo_path() function was introduced in c8243933c74, so it's never been in a release, thus I think it's fine to alter its behavior. The code here doesn't need to concern itself with what needs to be relative to what, you run send-email in some working tree directory (or top-level), and depending on core.hooksPath we'll either return a relative path to the .git/hooks or an absolute one, the system() invocation will accept either.
Hey On Tue, 25 May 2021 at 03:03, Junio C Hamano <gitster@pobox.com> wrote: > > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > > In c8243933c74 (git-send-email: Respect core.hooksPath setting, > > 2021-03-23) we started supporting core.hooksPath in "send-email". It's > > been reported that on Windows[1] doing this by calling abs_path() > > results in different canonicalizations of the absolute path. > > I see the author of that patch CC'ed; the change in question > explains why we switched from "the hooks directory immediately under > $repo->repo_path()" to "ask 'rev-parse --git-path hooks'", but it > does not say why we call abs_path() on the result. I guess that is > because $repo->repo_path() has always been a result of applying the > abs_path() function to something, so it was to safeguard the callers > that expect an absolute path coming back from hooks_path? I don't think I have a good reason why abs_path() was used, most likely it was copied from some other snippet and kept for uniformity. > > And that makes this change dubious, especially as a band-aid for a > breakage immediately before the final release, doesn't it? Are we > convinced that the callers are OK with seeing sometimes relative > paths? Certainly the cases the tests J6t fixed are not negatively > affected, but is that sufficient? To what directory is the > configuration variable supposed to be relative to, and are we sure > that the user will always invoke "git send-email" from that > directory? Previously this functionality was entirely not working, so I don't think we'll have any regressions. With that being said I'm unable to test that it works well on windows. As far as what the expected norms for paths are within git, I really don't have any answers. > > Puzzled. >
On Tue, May 25 2021, Ævar Arnfjörð Bjarmason wrote: > On Tue, May 25 2021, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>> In c8243933c74 (git-send-email: Respect core.hooksPath setting, >>> 2021-03-23) we started supporting core.hooksPath in "send-email". It's >>> been reported that on Windows[1] doing this by calling abs_path() >>> results in different canonicalizations of the absolute path. >> >> I see the author of that patch CC'ed; the change in question >> explains why we switched from "the hooks directory immediately under >> $repo->repo_path()" to "ask 'rev-parse --git-path hooks'", but it >> does not say why we call abs_path() on the result. I guess that is >> because $repo->repo_path() has always been a result of applying the >> abs_path() function to something, so it was to safeguard the callers >> that expect an absolute path coming back from hooks_path? >> >> And that makes this change dubious, especially as a band-aid for a >> breakage immediately before the final release, doesn't it? Are we >> convinced that the callers are OK with seeing sometimes relative >> paths? Certainly the cases the tests J6t fixed are not negatively >> affected, but is that sufficient? To what directory is the >> configuration variable supposed to be relative to, and are we sure >> that the user will always invoke "git send-email" from that >> directory? > > The one caller is git-send-email.perl is fine with it, at least on *nix, > this fix still needs testing on Windows. > > The repo_path() function was introduced in c8243933c74, so it's never > been in a release, thus I think it's fine to alter its behavior. > > The code here doesn't need to concern itself with what needs to be > relative to what, you run send-email in some working tree directory (or > top-level), and depending on core.hooksPath we'll either return a > relative path to the .git/hooks or an absolute one, the system() > invocation will accept either. ...I think the one issue with my 2/2 is that it doesn't go far enough, we should just remove the repo_path() from Git.pm and instead use its: $self->command_oneline('rev-parse', '--git-path', 'hooks') ...in the one user in git-send-email.perl, as discussed in other serieses Git.pm is "public", so stuff we stick in there we can't alter. In this case we're doing so before a release, and nobody wanted this except git-send-email.perl. That caller is likely to just go away if and when Emily's "git hook run" lands, so I think it would be best (but not strictly needed for the pre-rc fix) to just remove that API. What do you / Robert Foss think (maybe he wanted this for something else...)?
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > The repo_path() function was introduced in c8243933c74, so it's never > been in a release, thus I think it's fine to alter its behavior. > > The code here doesn't need to concern itself with what needs to be > relative to what, you run send-email in some working tree directory (or > top-level), and depending on core.hooksPath we'll either return a > relative path to the .git/hooks or an absolute one, the system() > invocation will accept either. All of that are convincing, but I'd rather not risk it. I'll take your 1/2 with J6t's fix, both of which are obvious and no brainer, but I am willing to take this 2/2 as a post-release clean-up next monght, though ;-) Thanks.
On Tue, 25 May 2021 at 08:15, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Tue, May 25 2021, Ævar Arnfjörð Bjarmason wrote: > > > On Tue, May 25 2021, Junio C Hamano wrote: > > > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> > >>> In c8243933c74 (git-send-email: Respect core.hooksPath setting, > >>> 2021-03-23) we started supporting core.hooksPath in "send-email". It's > >>> been reported that on Windows[1] doing this by calling abs_path() > >>> results in different canonicalizations of the absolute path. > >> > >> I see the author of that patch CC'ed; the change in question > >> explains why we switched from "the hooks directory immediately under > >> $repo->repo_path()" to "ask 'rev-parse --git-path hooks'", but it > >> does not say why we call abs_path() on the result. I guess that is > >> because $repo->repo_path() has always been a result of applying the > >> abs_path() function to something, so it was to safeguard the callers > >> that expect an absolute path coming back from hooks_path? > >> > >> And that makes this change dubious, especially as a band-aid for a > >> breakage immediately before the final release, doesn't it? Are we > >> convinced that the callers are OK with seeing sometimes relative > >> paths? Certainly the cases the tests J6t fixed are not negatively > >> affected, but is that sufficient? To what directory is the > >> configuration variable supposed to be relative to, and are we sure > >> that the user will always invoke "git send-email" from that > >> directory? > > > > The one caller is git-send-email.perl is fine with it, at least on *nix, > > this fix still needs testing on Windows. > > > > The repo_path() function was introduced in c8243933c74, so it's never > > been in a release, thus I think it's fine to alter its behavior. > > > > The code here doesn't need to concern itself with what needs to be > > relative to what, you run send-email in some working tree directory (or > > top-level), and depending on core.hooksPath we'll either return a > > relative path to the .git/hooks or an absolute one, the system() > > invocation will accept either. > > ...I think the one issue with my 2/2 is that it doesn't go far enough, > we should just remove the repo_path() from Git.pm and instead use its: > > $self->command_oneline('rev-parse', '--git-path', 'hooks') This looks good to me. > > ...in the one user in git-send-email.perl, as discussed in other > serieses Git.pm is "public", so stuff we stick in there we can't > alter. In this case we're doing so before a release, and nobody wanted > this except git-send-email.perl. > > That caller is likely to just go away if and when Emily's "git hook run" > lands, so I think it would be best (but not strictly needed for the > pre-rc fix) to just remove that API. That sounds like a sane approach. If core.hooksPath is needed in more locations in the future, it can be added then. > > What do you / Robert Foss think (maybe he wanted this for something > else...)?
On Tue, May 25 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> The repo_path() function was introduced in c8243933c74, so it's never >> been in a release, thus I think it's fine to alter its behavior. >> >> The code here doesn't need to concern itself with what needs to be >> relative to what, you run send-email in some working tree directory (or >> top-level), and depending on core.hooksPath we'll either return a >> relative path to the .git/hooks or an absolute one, the system() >> invocation will accept either. > > All of that are convincing, but I'd rather not risk it. I'll take > your 1/2 with J6t's fix, both of which are obvious and no brainer, > but I am willing to take this 2/2 as a post-release clean-up next > monght, though ;-) Fair enough. I get that we're in the rc phase, but FWIW I still disagree. I do think supporting whatever edge case we're exposing by round-tripping through abs_path() and its subtly different behavior is worse than a change to make us rely on the well-understood rev-parse behavior directly. Especially since the fix is to something that didn't work to begin with before the v2.32.0 release is out (core.hooksPath + send-email), but perhaps that's more of an argument for reverting the topic during RC + try to have it land in v2.33.0? And that + what seems to be the prevailing (IMO nonsensical) opinion on Git.pm's stability promise, meaning that once this new function is out in a release we'll be stuck supporting it makes me want to change this pre-release. But I'll leave it to you, if you are convinced and do want to take this 2/2 after all I'll submit another trivial patch on top to remove the new (then unused) repo_path function, which we expect to go away anyway.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > But I'll leave it to you, if you are convinced and do want to take this > 2/2 after all I'll submit another trivial patch on top to remove the new > (then unused) repo_path function, which we expect to go away anyway. Sounds good.
Ævar Arnfjörð Bjarmason wrote: > On Tue, May 25 2021, Junio C Hamano wrote: > > > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > > >> In c8243933c74 (git-send-email: Respect core.hooksPath setting, > >> 2021-03-23) we started supporting core.hooksPath in "send-email". It's > >> been reported that on Windows[1] doing this by calling abs_path() > >> results in different canonicalizations of the absolute path. > > > > I see the author of that patch CC'ed; the change in question > > explains why we switched from "the hooks directory immediately under > > $repo->repo_path()" to "ask 'rev-parse --git-path hooks'", but it > > does not say why we call abs_path() on the result. I guess that is > > because $repo->repo_path() has always been a result of applying the > > abs_path() function to something, so it was to safeguard the callers > > that expect an absolute path coming back from hooks_path? > > > > And that makes this change dubious, especially as a band-aid for a > > breakage immediately before the final release, doesn't it? Are we > > convinced that the callers are OK with seeing sometimes relative > > paths? Certainly the cases the tests J6t fixed are not negatively > > affected, but is that sufficient? To what directory is the > > configuration variable supposed to be relative to, and are we sure > > that the user will always invoke "git send-email" from that > > directory? > > The one caller is git-send-email.perl is fine with it, at least on *nix, > this fix still needs testing on Windows. While I understand the reluctance to test things on Windows, sometimes we waste more time discussing about what couldn't potentially break there than the time it takes to setup a VM and just make sure. Am I the only one that has setup a Windows VM just for tests? Also, there's ways to trigger CI runs to test this stuff aren't there? Seems like a much less painful way to leapfrog this roadblock. Cheers.
diff --git a/perl/Git.pm b/perl/Git.pm index 73ebbf80cc6..df6280ebab5 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -629,8 +629,7 @@ sub hooks_path { my ($self) = @_; my $dir = $self->command_oneline('rev-parse', '--git-path', 'hooks'); - my $abs = abs_path($dir); - return $abs; + return $dir; } =item wc_path () diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 2acf389837c..3b7540050ca 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -539,7 +539,7 @@ test_expect_success $PREREQ "--validate respects relative core.hooksPath path" ' test_path_is_file my-hooks.ran && cat >expect <<-EOF && fatal: longline.patch: rejected by sendemail-validate hook - fatal: command '"'"'$(pwd)/my-hooks/sendemail-validate'"'"' died with exit code 1 + fatal: command '"'"'my-hooks/sendemail-validate'"'"' died with exit code 1 warning: no patches were sent EOF test_cmp expect actual
In c8243933c74 (git-send-email: Respect core.hooksPath setting, 2021-03-23) we started supporting core.hooksPath in "send-email". It's been reported that on Windows[1] doing this by calling abs_path() results in different canonicalizations of the absolute path. This wasn't an issue in c8243933c74 itself, but was revealed by my ea7811b37e0 (git-send-email: improve --validate error output, 2021-04-06) when we started emitting the path to the hook, which was previously only internal to git-send-email.perl. I think this change should let us have our cake and eat it too. We now emit a relative path for the common case where the hook is in the .git/hooks directory, but in the case it's an absolute path (there's another test for that, not seen here) we'll prefix it with $(pwd). I hope that unlike the current implementation that $(pwd) v.s. $PWD difference won't matter on Windows, since now the absolute path is the one we get from rev-parse, not the one that's been passed through Perl's Cwd::abs_path(). 1. http://lore.kernel.org/git/bb30fe2b-cd75-4782-24a6-08bb002a0367@kdbg.org Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- perl/Git.pm | 3 +-- t/t9001-send-email.sh | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-)