Re: [PATCH v2 2/3] mailinfo.c::convert_to_utf8: reuse strlen info

[Danh Doan <congdanhqx@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.

> 
> > --- 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.

> > +       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.

> > +
> >  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