Re: [PATCH 1/3] pretty: support "mboxrd" output format

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 4, 2016, 00:19 UTC
Message-ID: <CAPig+cQ7y7J02+PVtdJywGEudOGXsy1jgadvVwBg5creiUv6zA@mail.gmail.com>
In-Reply-To: <CAPig+cTQkEb6K95aeJ9vNyJ=S+8x+JnCQAYSEzydXKzbq-+Yyg@mail.gmail.com>

On Fri, Jun 3, 2016 at 8:02 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 19 quoted lines

> On Fri, Jun 3, 2016 at 7:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>>>>         static int is_mboxrd_from(const char *line) {
>>>>                 return starts_with(line + strspn(line, ">"), "From ");
>>>>         }
>>>>
>>>> is sufficiently high-level that no longer is scary, hopefully?
>>>
>>> That's nice and concise but unfortunately not useful for this case
>>> where we must respect the logical end-of-line (within the larger
>>> buffer), represented by line+linelen.
>>
>> Hmph, none of ">", "F", "r", "o", "m", or " " is "\n"; would eol
>> still be an issue?
>
> Ah right. Sorry for being slow.
>
> Your concise version of is_mboxrd_from() is a nice alternative, as well.

My only minor reservation is that it your concise version is still subtle with regard to not taking 'linelen' into account. At first glance, it looks like a bug that it doesn't consider the logical end-of-line, and someone reading the code has to put in extra effort to convince himself that the code works as intended.

For that reason, I have a bit of a preference for a version of is_mboxrd_from() which does take 'linelen' into account explicitly.