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

From: Eric Wong <e@80x24.org>
Date: Jun 2, 2016, 07:51 UTC
Message-ID: <20160602075125.GA19551@dcvr.yhbt.net>
In-Reply-To: <20160531182932.GA27021@dcvr.yhbt.net>

Eric Wong <e@80x24.org> wrote:

Show 24 quoted lines

> 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);
}

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
* extra parentheses
* removal of "t" variable (t for "terminal/termination"?)

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.