diff mbox series

[v2,4/5] mailinfo: strip quoted CR on users' wish

Message ID 01d4a4853b8a0b6911a2d7773f836620566b4293.1620148732.git.congdanhqx@gmail.com (mailing list archive)
State Superseded
Headers show
Series Teach am/mailinfo to process quoted CR | expand

Commit Message

Đoàn Trần Công Danh May 4, 2021, 5:20 p.m. UTC
In previous changes, we've turned on warning for quoted CR in base64 or
quoted-printable email messages. Some projects sees those quoted CR a lot
and they know that it happens most of the time.

Those projects in question usually fall back to use other tools to handle
patches when receiving such patches.

Let's help those projects handle those patches by stripping those
excessive CR-s.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 Documentation/git-mailinfo.txt | 1 +
 mailinfo.c                     | 7 +++++++
 mailinfo.h                     | 1 +
 t/t5100-mailinfo.sh            | 4 ++++
 4 files changed, 13 insertions(+)

Comments

Junio C Hamano May 5, 2021, 4:27 a.m. UTC | #1
Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> Subject: Re: [PATCH v2 4/5] mailinfo: strip quoted CR on users' wish

Again, perhaps

    mailinfo: allow stripping quoted CR without warning

By the way, the previous one said "skip", but I do not think it was
skipping quoted CR, so its title was misleading.  Perhaps

  [3/5] mailinfo: allow squelching quoted CR warning

or something.

> In previous changes, we've turned on warning for quoted CR in base64 or
> quoted-printable email messages. Some projects sees those quoted CR a lot
> and they know that it happens most of the time.

    ... a lot, they know that it happens most of the time, and they
    know it always is harmless to behave as if these CRs are not
    there.

The last sentence is an important precondition for the use of this
new option to be safe.

> diff --git a/mailinfo.c b/mailinfo.c
> index fe7ffd01d0..68f4eba72a 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -998,6 +998,11 @@ static void handle_filter_flowed(struct mailinfo *mi, struct strbuf *line,
>  		    line->buf[len - 2] == '\r' &&
>  		    line->buf[len - 1] == '\n') {
>  			*have_quoted_cr = 1;
> +			if (mi->quoted_cr == quoted_cr_strip) {
> +				strbuf_setlen(line, len - 2);
> +				strbuf_addch(line, '\n');
> +				len--;

The last one is beating a dead variable immediately before this
function returns, even though it is good for consistency (i.e. there
is an invaliant throughout the function that len is the number of
bytes contained in line->buf[]).

I am not sure what to think about this.  I wish there weren't need
for a separate len variable, with the need for this extra invariant.
After all, strbuf already has such an invariant that is well
understood by readers of this code (i.e. line->buf[]'s end is at
line->len).  For now, until we get rid of "len" from this function,
let's leave the final decrement in to make it absolutely clear that
we are aware of this extra invariant.
diff mbox series

Patch

diff --git a/Documentation/git-mailinfo.txt b/Documentation/git-mailinfo.txt
index c776b27515..d700929a46 100644
--- a/Documentation/git-mailinfo.txt
+++ b/Documentation/git-mailinfo.txt
@@ -100,6 +100,7 @@  The valid actions are:
 *	`nowarn`: Git will do nothing with this action.
 *	`warn`: Git will issue a warning for each message if such CR-LF is
 	found.
+*	`strip`: Git will convert those CR-LF to LF.
 --
 +
 The default action could be set by configuration option `mailinfo.quotedCR`.
diff --git a/mailinfo.c b/mailinfo.c
index fe7ffd01d0..68f4eba72a 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -998,6 +998,11 @@  static void handle_filter_flowed(struct mailinfo *mi, struct strbuf *line,
 		    line->buf[len - 2] == '\r' &&
 		    line->buf[len - 1] == '\n') {
 			*have_quoted_cr = 1;
+			if (mi->quoted_cr == quoted_cr_strip) {
+				strbuf_setlen(line, len - 2);
+				strbuf_addch(line, '\n');
+				len--;
+			}
 		}
 		handle_filter(mi, line);
 		return;
@@ -1228,6 +1233,8 @@  enum quoted_cr_action mailinfo_parse_quoted_cr_action(const char *action)
 		return quoted_cr_nowarn;
 	else if (!strcmp(action, "warn"))
 		return quoted_cr_warn;
+	else if (!strcmp(action, "strip"))
+		return quoted_cr_strip;
 	return quoted_cr_invalid_action;
 }
 
diff --git a/mailinfo.h b/mailinfo.h
index 1bcef5a6f3..e0e094c311 100644
--- a/mailinfo.h
+++ b/mailinfo.h
@@ -8,6 +8,7 @@ 
 enum quoted_cr_action {
 	quoted_cr_nowarn,
 	quoted_cr_warn,
+	quoted_cr_strip,
 	quoted_cr_invalid_action
 };
 
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 57b8fc8104..7559c922c6 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -242,6 +242,10 @@  test_expect_success 'mailinfo handle CR in base64 encoded email' '
 	check_quoted_cr_mail &&
 	grep "quoted CR detected" quoted-cr-err &&
 	check_quoted_cr_mail --quoted-cr=nowarn &&
+	test_must_be_empty quoted-cr-err &&
+	sed "s/%%//" "$DATA/quoted-cr-msg" >expect-cr-msg &&
+	sed "s/%%//" "$DATA/quoted-cr-patch" >expect-cr-patch &&
+	check_quoted_cr_mail --quoted-cr=strip &&
 	test_must_be_empty quoted-cr-err
 '