Re: [PATCH v2 0/4] handle multiline in-body headers

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 19, 2016, 21:39 UTC
Message-ID: <xmqqr38fd0y1.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <cover.1474319086.git.jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 13 quoted lines

> This is an iteration of the patch set after the discussion in
>  <cover.1474047135.git.jonathantanmy@google.com>.
>
> Changes:
> o largely rewritten to follow Junio's suggested design (refactor of
>   check_header to separate out ">From" and "[PATCH]", and an
>   is_inbody_header function performing an airtight check on whether a
>   line is an in-body header)
> o simpler try_convert_to_utf8 API
> o one file of the expected output of t/t5100/*0015 is modified (instead
>   of the input)
> o t/t5100/*0018--no-inbody-headers test files added
> o example in commit message improved following Peff's suggestion

Overall it looks much nicer, but I still do not understand why we want to do try-convert-to-utf8. The _only_ caller of that helper function is in 4/4 where it tries to convert the line to see if a line is a scissors line.

It seems to me that the only thing doing so gives us is that you could later enhance the definition of what a scissors-line looks like by adding unicode characters [*1*], but I would strongly suggest not doing that kind of enhancement [*2*]. With the current definition of what a scissors-line looks like, it can be checked before you do the UTF-8 conversion, I think, which would mean that the helper is not strictly necessary.

So...

[Footnotes]

*1* E.g. ✂ encoded in non-UTF8 may have to be converted to UTF-8 first before being recognized as such.

*2* Encouraging people to use non-ASCII that older version of Git did not take for structural things like "what is a scissors line" merely for the looks is a bad trade-off; patch e-mails that use the enhanced definition will break for those with older version of Git, and the benefit of "prettier scissors" is not really worth it, I would think.