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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 3, 2016, 22:22 UTC
Message-ID: <CAPig+cTLVXJBn00aco0vC9oFvZuchTgtNXtGGjpLPx1LwRZz4Q@mail.gmail.com>
In-Reply-To: <20160602075125.GA19551@dcvr.yhbt.net>

[cc:+junio]

On Thu, Jun 2, 2016 at 3:51 AM, Eric Wong <e@80x24.org> wrote:

Show 38 quoted lines

> Eric Wong <e@80x24.org> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> wrote:
>> > On Tue, May 31, 2016 at 3:45 AM, Eric Wong <e@80x24.org> wrote:
>> > > Eric Sunshine <sunshine@sunshineco.com> wrote:
>> > >> I wonder if hand-coding, rather than using a regex, could be an improvement:
>> > >>
>> > >>     static int is_mboxrd_from(const char *s, size_t n)
>> > >>     {
>> > >>         size_t f = strlen("From ");
>> > >>         const char *t = s + n;
>> > >>
>> > >>         while (s < t && *s == '>')
>> > >>             s++;
>> > >>         return t - s >= f && !memcmp(s, "From ", f);
>> > >>     }
>> > >>
>> > >> or something.
>> > >
>> > > Yikes.  I mostly work in high-level languages and do my best to
>> > > avoid string parsing in C; so that scares me.  A lot.
>> >
>> > The hand-coded is_mboxrd_from() above is pretty much idiomatic C and
>> > (I think) typical of how such a function would be coded in Git itself,
>> > so it looks normal and easy to grok to me (but, of course, I'm
>> > probably biased since I wrote it).
>
> For reference, here is the gfrom function from qmail (gfrom.c,
> source package netqmail-1.06 in Debian, reformatted git style)
>
> int gfrom(char *s, int len)
> {
>         while ((len > 0) && (*s == '>')) {
>                 ++s;
>                 --len;
>         }
>
>         return (len >= 5) && !str_diffn(s, "From ", 5);
> }

Seems less idiomatic and less like what we might see elsewhere in the Git codebase, but that's subjective. Functionally, it appears correct.

Show 5 quoted lines

> Similar to yours, but a several small things improves
> readability for me:
>
> * the avoidance of subtraction from the "return" conditional
> * s/n/len/ variable name

Idiomatic C code favors concise names such as 'i', 'j', or 'n', for instance, but I don't care strongly.

> * extra parentheses

Unnecessary syntactic noise (consuming reviewer brain cycles).

> * removal of "t" variable (t for "terminal/termination"?)

Heh, no, just the next letter after 's'. Again, just an idiom, as 'i', 'j', 'k' are often used for integers, 's' and 't' are common for strings.

Show 5 quoted lines

> str_diffn is memcmp-like, I assume.  My eyes glazed over
> when I saw that function implemented in str_diffn.c, too.
>
> Just thinking out loud, with sufficient tests I could go with
> either.  Will reroll when/if I get the chance tomorrow.

As mentioned above, it's all subjective and, of course, I have a bias toward the example I provided, but don't otherwise feel strongly about it. I do, however, like the idea of using a simple hand-coded matching function over the regex (but no so much that I would complain about it). Use whatever you and Junio feel is appropriate.