Re: [PATCH v2 2/3] mailinfo.c::convert_to_utf8: reuse strlen info
- From
Danh Doan <congdanhqx@gmail.com>
- Date
- Apr 19, 2020, 14:16 UTC
- Message-ID
- <20200419141637.GA28207@danh.dev>
- In-Reply-To
- <CAN0heSqi8KvbfSD80UA9s_aj9dGYTvbeW4GXVY59014_Sm-HmQ@mail.gmail.com>
On 2020-04-19 14:29:06+0200, Martin Ågren <martin.agren@gmail.com> wrote:
> I think the subject could be adapted though? Now it's not about "reusing > info" anymore, it's about using *other*, *better* info. Maybe > > mailinfo.c: avoid strlen on strings that might contain NUL
This is better than mine. I couldn't think about any other alternative.
Show 59 quoted lines
>
> > --- a/mailinfo.c
> > +++ b/mailinfo.c
> > @@ -447,19 +447,21 @@ static int convert_to_utf8(struct mailinfo *mi,
> > struct strbuf *line, const char *charset)
> > {
> > char *out;
> > + size_t out_len;
> >
> > if (!mi->metainfo_charset || !charset || !*charset)
> > return 0;
> >
> > if (same_encoding(mi->metainfo_charset, charset))
> > return 0;
> > - out = reencode_string(line->buf, mi->metainfo_charset, charset);
> > + out = reencode_string_len(line->buf, line->len,
> > + mi->metainfo_charset, charset, &out_len);
> > if (!out) {
> > mi->input_error = -1;
> > return error("cannot convert from %s to %s",
> > charset, mi->metainfo_charset);
> > }
> > - strbuf_attach(line, out, strlen(out), strlen(out));
> > + strbuf_attach(line, out, out_len, out_len);
> > return 0;
> > }
>
> Same diff as before, ok.
>
> > +write_nul_patch() {
> > + space=' '
> > + qNUL=
> > + case "$1" in
> > + subject) qNUL='=00' ;;
> > + esac
>
> Here it's case/esac...
>
> > + cat <<-EOF
> > + From ec7364544f690c560304f5a5de9428ea3b978b26 Mon Sep 17 00:00:00 2001
> > + From: A U Thor <author@example.com>
> > + Date: Sun, 19 Apr 2020 13:42:07 +0700
> > + Subject: [PATCH] =?ISO-8859-1?q?=C4=CB${qNUL}=D1=CF=D6?=
> > + MIME-Version: 1.0
> > + Content-Type: text/plain; charset=ISO-8859-1
> > + Content-Transfer-Encoding: 8bit
> > +
> > + EOF
> > + if test "$1" = body
> > + then
> > + printf "%s\0%s\n" abc def
> > + fi
>
> Here it's if/fi. Looks a bit inconsistent.
>
> I suppose you tried to have a case for "body" above but couldn't get it
> to work? Somehow, it would seem more consistent to have a qSubject and
> qBody and handle them the same way, but maybe that's not possible?
> Maybe using q_to_nul to create qBody containing a NUL?Those patch was written in different time, with different thought in mind.
I need to send a re-roll to update the subject and moving oneline in test code to later patch.
I'll update this hunk with that re-roll. Your new series _won't_ be affected, though.
Show 11 quoted lines
> > + cat <<-\EOF > > + --- > > + diff --git a/afile b/afile > > + new file mode 100644 > > + index 0000000000..e69de29bb2 > > + --$space > > + 2.26.1 > > + EOF > > +} > > I think this helper function should use &&-chaining.
Correct.
Show 26 quoted lines
> > + > > test_expect_success setup ' > > # Note the missing "+++" line: > > cat >bad-patch.diff <<-\EOF && > > @@ -32,4 +62,18 @@ test_expect_success 'try to apply corrupted patch' ' > > test_i18ncmp expected actual > > ' > > > > +test_expect_success "NUL in commit message's body" ' > > + test_when_finished "git am --abort" && > > + write_nul_patch body >body.patch && > > + test_must_fail git am body.patch 2>err && > > + grep "a NUL byte in commit log message not allowed" err > > +' > > Makes sense. > > > + > > +test_expect_failure "NUL in commit message's header" ' > > + test_when_finished "git am --abort" && > > + write_nul_patch subject >subject.patch && > > + test_must_fail git am subject.patch 2>err && > > + grep "a NUL byte in Subject is not allowed" err > > +' > > Interesting. :-)
Going through the mail now make me think about moving the last grep to next patch in order to clearly indicate that we expect "git-am" is failling but it's passed instead.
And that message isn't introduced until the very next change.
-- Danh