Hi Robin
On 03/04/2023 15:32, Robin Jarry wrote:
Hi Phillip,
Phillip Wood, Apr 03, 2023 at 16:09:
+ <patch-file> LF
Usually git commands that produce or consume paths either use quoted
paths terminated by LF or unquoted paths terminated by NUL. That way
there is no ambiguity when a path contains LF.
I had never imagined that some twisted mind would insert LF in path
names but since nothing will forbid it, I agree that it is
a possibility.
I'm not sure what you mean by quoted paths, you mean adding literal
double quotes before printing them to the hook stdin? That means the
hook needs to handle de-quoting after reading, right?
I meant quoted in the same way that diff and ls-files etc quote paths
that contain control characters - see quote_c_style() in quote.c if
you're interested in the details. It looks like Git.pm can unquote paths
but has no code to quote them. It is probably easiest to use NUL
termination here - bash and zsh can read NUL terminated lines and so can
any scripting language.
diff --git a/git-send-email.perl b/git-send-email.perl
index 07f2a0cbeaad..bec4d0f4ab47 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -800,6 +800,7 @@ sub is_format_patch_arg {
validate_patch($f, $target_xfer_encoding);
}
}
+ validate_patch_series(@files)
This happens fairly early, before the user has had a chance to edit the
patches and before we have added all the recipient and in-reply-to
headers to the patch files. Would it be more useful to validate what
will actually be sent?
I agree that it would be better. I added the check here to be in line
with the existing sendemail-validate hook. I could move it after edition
and header finalization but then we would need to move
sendemail-validate as well for consistency. What do you think?
That would be my inclination but I'm only an occasional send-email user.
The downside is that the user may edit a patch only for it to be
rejected but we could offer for them to edit it again rather than just
throwing their work away.
+ # The hook needs a correct cwd and GIT_DIR.
+ require Cwd;
+ my $cwd_save = Cwd::getcwd();
+ chdir($repo->wc_path() or $repo->repo_path()) or die("chdir: $!");
+ local $ENV{"GIT_DIR"} = $repo->repo_path();
This looks like it is copied from the existing code but why do we need
to do this? I'm struggling to come up with a scenario where "git
send-email" can find the repository but the hook cannot.
Again, for consistency I assumed it would be best to keep the code
similar in both hooks. If you think it is safe to skip that check, I'll
remove it gladly.
I suspect it is safe, hopefully someone will speak up if I'm mistaken. A
while ago rebase stopped setting these variables when running an "exec"
command as they were causing problems (see 434e0636db (sequencer: do not
export GIT_DIR and GIT_WORK_TREE for 'exec', 2021-12-04))
+ # cannot use git hook run, it closes stdin before forking the hook
+ open(my $stdin, "|-", $validate_hook) or die("fork: $!");
This passes $validate_hook to the shell to execute which is not what we
want as it will split the hook path on whitespace etc. I think it would
be better to use "git hook run --to-stdin" (see 0414b3891c (hook:
support a --to-stdin=<path> option, 2023-02-08))
Ah that's a nice addition. I'll add that in v2.
That's great, Best Wishes
Phillip
Thanks for reviewing!
