Re: [PATCH v2 3/5] mailinfo: skip quoted CR on user's wish
From: Junio C Hamano <hidden>
Date: 2021-05-05 04:12:18
Đoàn Trần Công Danh [off-list ref] writes:
Subject: Re: [PATCH v2 3/5] mailinfo: skip quoted CR on user's wish
Nothing wrong per-se, but "on user's wish" feel somewhat bizarre.
Perhaps
mailinfo: allow skipping quoted CR
or something along that line?
quoted hunk
In previous change, we've turned on warning for quoted CR in base64 encoded email. Despite those warnings are usually helpful for our users, they may expect quoted CR in their emails. Let's give them an option to turn off the warning completely. Signed-off-by: Đoàn Trần Công Danh <redacted> --- Documentation/git-mailinfo.txt | 18 +++++++++++++++++- builtin/mailinfo.c | 8 ++++++-- mailinfo.c | 21 ++++++++++++++++++++- mailinfo.h | 8 ++++++++ t/t5100-mailinfo.sh | 6 ++++-- 5 files changed, 55 insertions(+), 6 deletions(-)diff --git a/Documentation/git-mailinfo.txt b/Documentation/git-mailinfo.txt index d343f040f5..c776b27515 100644 --- a/Documentation/git-mailinfo.txt +++ b/Documentation/git-mailinfo.txt@@ -9,7 +9,7 @@ git-mailinfo - Extracts patch and authorship from a single e-mail message SYNOPSIS -------- [verse] -'git mailinfo' [-k|-b] [-u | --encoding=<encoding> | -n] [--[no-]scissors] <msg> <patch> +'git mailinfo' [-k|-b] [-u | --encoding=<encoding> | -n] [--[no-]scissors] [--quoted-cr=<action>] <msg> <patch>
This line is getting really crowded. Perhaps it is time to do 'git mailinfo' [<options>] <msg> <patch> like other Git subcommands with too many options? Certainly it can be done after the dust settles from this entire series as a follow up clean-up patch.
quoted hunk
@@ -89,6 +89,22 @@ This can be enabled by default with the configuration option mailinfo.scissors. --no-scissors:: Ignore scissors lines. Useful for overriding mailinfo.scissors settings. +--quoted-cr=<action>:: + Action when processes email messages sent with base64 or + quoted-printable encoding, and the decoded lines end with CR-LF
s/with CR-LF/with a CRLF/
+ instead of a simple LF. ++ +The valid actions are: ++ +-- +* `nowarn`: Git will do nothing with this action.
s/with this action./when such a CRLF is seen./ perhaps?
+* `warn`: Git will issue a warning for each message if such CR-LF is
s/such CR-LF/such a CRLF/
quoted hunk
diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index b309badce5..1d600263cb 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c@@ -9,7 +9,7 @@ #include "mailinfo.h" static const char mailinfo_usage[] = - "git mailinfo [-k | -b] [-m | --message-id] [-u | --encoding=<encoding> | -n] [--scissors | --no-scissors] <msg> <patch> < mail >info"; + "git mailinfo [-k | -b] [-m | --message-id] [-u | --encoding=<encoding> | -n] [--scissors | --no-scissors] [--quoted-cr=<action>] <msg> <patch> < mail >info";
It is surprising that we haven't switched this to parse_options(). It of course is outside the scope of this series, but from a cursory look of its option parsing loop, it looks like a trivial improvement to make.
quoted hunk
@@ -43,7 +43,11 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix) mi.use_scissors = 0; else if (!strcmp(argv[1], "--no-inbody-headers")) mi.use_inbody_headers = 0; - else + else if (skip_prefix(argv[1], "--quoted-cr=", &str)) { + mi.quoted_cr = mailinfo_parse_quoted_cr_action(str); + if (mi.quoted_cr == quoted_cr_invalid_action) + usage(mailinfo_usage);
This is not all that helpful, given that mailinfo_usage[] only says
<action> without saying what the supported values are, and the
message does not make it clear it was issued while looking at the
--quoted-cr option.
At least, something like
if (mi.quoted_cr == quoted_cr_invalid_action)
die("--quoted-cr=%s: invalid action", str);
would be more palatable, but I wonder if mailinfo_parse_quoted_cr_action()
should have an option to die with the list of actions it knows about
in a message.
quoted hunk
diff --git a/mailinfo.c b/mailinfo.c index 713567f84b..fe7ffd01d0 100644 --- a/mailinfo.c +++ b/mailinfo.c@@ -1040,7 +1040,8 @@ static void handle_filter_flowed(struct mailinfo *mi, struct strbuf *line, static void summarize_quoted_cr(struct mailinfo *mi, int have_quoted_cr) { - if (have_quoted_cr) + if (have_quoted_cr + && mi->quoted_cr == quoted_cr_warn)
Existing code in this file prefers to split a multi-line statement after sequence point like &&, ||, etc., not before.
quoted hunk
warning("quoted CR detected"); }@@ -1221,9 +1222,19 @@ int mailinfo(struct mailinfo *mi, const char *msg, const char *patch) return mi->input_error; } +enum quoted_cr_action mailinfo_parse_quoted_cr_action(const char *action) +{ + if (!strcmp(action, "nowarn")) + return quoted_cr_nowarn; + else if (!strcmp(action, "warn")) + return quoted_cr_warn; + return quoted_cr_invalid_action; +}
OK.
quoted hunk
static int git_mailinfo_config(const char *var, const char *value, void *mi_) { struct mailinfo *mi = mi_; + const char *str; if (!starts_with(var, "mailinfo.")) return git_default_config(var, value, NULL);@@ -1231,6 +1242,13 @@ static int git_mailinfo_config(const char *var, const char *value, void *mi_) mi->use_scissors = git_config_bool(var, value); return 0; } + if (!strcmp(var, "mailinfo.quotedcr")) { + git_config_string(&str, var, value); + mi->quoted_cr = mailinfo_parse_quoted_cr_action(str); + if (mi->quoted_cr == quoted_cr_invalid_action) + die(_("bad action '%s' for '%s'"), str, var); + free((void *)str); + }
Here, it is more reasonable. It still does not say what actions are accepted, but at least the user learns where our displeasure comes from.
/* perhaps others here */ return 0; }