Re: [PATCH 2/2] send-email: don't needlessly abs_path() the core.hooksPath
- From
Felipe Contreras <felipe.contreras@gmail.com>
- Date
- May 26, 2021, 01:22 UTC
- Message-ID
- <60ada2dac06de_2c7f620854@natae.notmuch>
- In-Reply-To
- <874kerny0e.fsf@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason wrote:
Show 28 quoted lines
> 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.
-- Felipe Contreras