| Message ID | patch-1.1-cc0ba73974a-20210323T173032Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c8243933c74e0bebcc617f750ae3990ecca47759 |
| Headers | show |
| Series | [v3] git-send-email: Respect core.hooksPath setting | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > From: Robert Foss <robert.foss@linaro.org> > > get-send-email currently makes the assumption that the > 'sendemail-validate' hook exists inside of the repository. > > Since the introduction of 'core.hooksPath' configuration option in > 867ad08a261 (hooks: allow customizing where the hook directory is, > 2016-05-04), this is no longer true. > > Instead of assuming a hardcoded repo relative path, query > git for the actual path of the hooks directory. > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > > Here's a v3 that fixes various issues with Robert's v2. Range-diff & > updated patch below. > > The advice I had in the v1 feedback about GetHooksPath was bad, just > having it be a new accessor is better. It's not like anyone is calling > this in a loop. How urgent is this "fix". I am wondering if Emily's "git hook" automatically fix this for us when it comes.
On Tue, Mar 23 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> From: Robert Foss <robert.foss@linaro.org> >> >> get-send-email currently makes the assumption that the >> 'sendemail-validate' hook exists inside of the repository. >> >> Since the introduction of 'core.hooksPath' configuration option in >> 867ad08a261 (hooks: allow customizing where the hook directory is, >> 2016-05-04), this is no longer true. >> >> Instead of assuming a hardcoded repo relative path, query >> git for the actual path of the hooks directory. >> >> Signed-off-by: Robert Foss <robert.foss@linaro.org> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> >> Here's a v3 that fixes various issues with Robert's v2. Range-diff & >> updated patch below. >> >> The advice I had in the v1 feedback about GetHooksPath was bad, just >> having it be a new accessor is better. It's not like anyone is calling >> this in a loop. > > How urgent is this "fix". I am wondering if Emily's "git hook" > automatically fix this for us when it comes. We've had iterations of that topic for almost a year now (since 2019 counting RFC discussions). While I'd like to see it land I'm skeptical of parts of that approach[1] and expect we'll have more re-rolls of it, and in any case the conflict in send-email[2] will be trivial to resolve. So I think it makes sense to queue up this narrow fix and not have this wait on the larger topic. 1. https://lore.kernel.org/git/87mtv8fww3.fsf@evledraar.gmail.com/ 2. https://lore.kernel.org/git/20210311021037.3001235-36-emilyshaffer@google.com/
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > While I'd like to see it land I'm skeptical of parts of that approach[1] > and expect we'll have more re-rolls of it, and in any case the conflict > in send-email[2] will be trivial to resolve. > So I think it makes sense > to queue up this narrow fix and not have this wait on the larger topic. Well, that is what I am doing for now, queue up this narrow change, while resolving the conflict to use Emily's topic. Which means that the new code this topic adds is never exercised while the other topic is still cooking in tree. > 1. https://lore.kernel.org/git/87mtv8fww3.fsf@evledraar.gmail.com/ > 2. https://lore.kernel.org/git/20210311021037.3001235-36-emilyshaffer@google.com/
diff --git a/git-send-email.perl b/git-send-email.perl index 1f425c08091..f5bbf1647e3 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1942,7 +1942,7 @@ sub validate_patch { my ($fn, $xfer_encoding) = @_; if ($repo) { - my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'), + my $validate_hook = catfile($repo->hooks_path(), 'sendemail-validate'); my $hook_error; if (-x $validate_hook) { diff --git a/perl/Git.pm b/perl/Git.pm index 02eacef0c2a..73ebbf80cc6 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -619,6 +619,19 @@ sub _prompt { sub repo_path { $_[0]->{opts}->{Repository} } +=item hooks_path () + +Return path to the hooks directory. Must be called on a repository instance. + +=cut + +sub hooks_path { + my ($self) = @_; + + my $dir = $self->command_oneline('rev-parse', '--git-path', 'hooks'); + my $abs = abs_path($dir); + return $abs; +} =item wc_path () diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 4eee9c3dcb5..1a1caf8f2ed 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -513,6 +513,38 @@ do done +test_expect_success $PREREQ "--validate respects relative core.hooksPath path" ' + clean_fake_sendmail && + mkdir my-hooks && + test_when_finished "rm my-hooks.ran" && + write_script my-hooks/sendemail-validate <<-\EOF && + >my-hooks.ran + exit 1 + EOF + test_config core.hooksPath "my-hooks" && + test_must_fail git send-email \ + --from="Example <nobody@example.com>" \ + --to=nobody@example.com \ + --smtp-server="$(pwd)/fake.sendmail" \ + --validate \ + longline.patch 2>err && + test_path_is_file my-hooks.ran && + grep "rejected by sendemail-validate" err +' + +test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" ' + test_config core.hooksPath "$(pwd)/my-hooks" && + test_when_finished "rm my-hooks.ran" && + test_must_fail git send-email \ + --from="Example <nobody@example.com>" \ + --to=nobody@example.com \ + --smtp-server="$(pwd)/fake.sendmail" \ + --validate \ + longline.patch 2>err && + test_path_is_file my-hooks.ran && + grep "rejected by sendemail-validate" err +' + for enc in 7bit 8bit quoted-printable base64 do test_expect_success $PREREQ "--transfer-encoding=$enc produces correct header" '