Re: [PATCH/WIP v3 06/31] am: detect mbox patches

From: Paul Tan <pyokagan@gmail.com>
Date: Jun 25, 2015, 13:40 UTC
Message-ID: <CACRoPnQAGt_mLbG-EtJ+qTpD2OvNgi-MZfU=NoKCoSmhCOnBEw@mail.gmail.com>
In-Reply-To: <334feea4cee88d06a10a7363956d2bfe@www.dscho.org>

On Wed, Jun 24, 2015 at 11:10 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 38 quoted lines

> Hi Paul,
>
> On 2015-06-18 13:25, Paul Tan wrote:
>
>> diff --git a/builtin/am.c b/builtin/am.c
>> index e9a3687..7b97ea8 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -121,6 +121,96 @@ static void am_destroy(const struct am_state *state)
>>       strbuf_release(&sb);
>>  }
>>
>> +/*
>> + * Returns 1 if the file looks like a piece of email a-la RFC2822, 0 otherwise.
>> + * We check this by grabbing all the non-indented lines and seeing if they look
>> + * like they begin with valid header field names.
>> + */
>> +static int is_email(const char *filename)
>> +{
>> +     struct strbuf sb = STRBUF_INIT;
>> +     FILE *fp = xfopen(filename, "r");
>> +     int ret = 1;
>> +
>> +     while (!strbuf_getline(&sb, fp, '\n')) {
>> +             const char *x;
>> +
>> +             strbuf_rtrim(&sb);
>> +
>> +             if (!sb.len)
>> +                     break; /* End of header */
>> +
>> +             /* Ignore indented folded lines */
>> +             if (*sb.buf == '\t' || *sb.buf == ' ')
>> +                     continue;
>> +
>> +             /* It's a header if it matches the regexp "^[!-9;-~]+:" */
>
> Why not just compile a regex and use it here? We use regexes elsewhere anyway...

Ah, you're right. A regular expression would definitely be clearer. I've fixed it on my end.

Show 35 quoted lines

>> +/**
>> + * Attempts to detect the patch_format of the patches contained in `paths`,
>> + * returning the PATCH_FORMAT_* enum value. Returns PATCH_FORMAT_UNKNOWN if
>> + * detection fails.
>> + */
>> +static int detect_patch_format(struct string_list *paths)
>> +{
>> +     enum patch_format ret = PATCH_FORMAT_UNKNOWN;
>> +     struct strbuf l1 = STRBUF_INIT;
>> +     struct strbuf l2 = STRBUF_INIT;
>> +     struct strbuf l3 = STRBUF_INIT;
>> +     FILE *fp;
>> +
>> +     /*
>> +      * We default to mbox format if input is from stdin and for directories
>> +      */
>> +     if (!paths->nr || !strcmp(paths->items->string, "-") ||
>> +         is_directory(paths->items->string)) {
>> +             ret = PATCH_FORMAT_MBOX;
>> +             goto done;
>> +     }
>> +
>> +     /*
>> +      * Otherwise, check the first 3 lines of the first patch, starting
>> +      * from the first non-blank line, to try to detect its format.
>> +      */
>> +     fp = xfopen(paths->items->string, "r");
>> +     while (!strbuf_getline(&l1, fp, '\n')) {
>> +             strbuf_trim(&l1);
>> +             if (l1.len)
>> +                     break;
>> +     }
>> +     strbuf_getline(&l2, fp, '\n');
>
> We should test the return value of `strbuf_getline()`; if EOF was reached already, `strbuf_getwholeline()` does not touch the strbuf. I know, the strbuf is still initialized empty here, but it is too easy to forget when changing this code.

Ah OK. I'll vote for doing a strbuf_reset() just before the strbuf_getline() though.

Show 9 quoted lines

>> +     strbuf_trim(&l2);
>> +     strbuf_getline(&l3, fp, '\n');
>> +     strbuf_trim(&l3);
>> +     fclose(fp);
>> +
>> +     if (starts_with(l1.buf, "From ") || starts_with(l1.buf, "From: "))
>> +             ret = PATCH_FORMAT_MBOX;
>
> Hmm. We can test that earlier and return without reading from the file any further, I think.

The "reading 3 lines at the beginning" logic is meant to support a later patch where support for StGit and mercurial patches is added. That said, it's true that we don't need to read 3 lines in this patch, so I think I will remove it in this patch.

>> +     else if (l1.len && l2.len && l3.len && is_email(paths->items->string))
>> +             ret = PATCH_FORMAT_MBOX;
>
> Maybe we can do better than this by folding the `is_email() function into this here function, reusing the same strbuf to read the lines and keeping track of the email header lines we saw... I would really like to avoid opening the same file twice just to figure out whether it is in email format.

Okay, how about every time we call a strbuf_getline(), we save the line to a string_list as well? Like string_list_getline_crlf() below:

/**
 * Like strbuf_getline(), but supports both '\n' and "\r\n" as line
 * terminators.
 */
static int strbuf_getline_crlf(struct strbuf *sb, FILE *fp)
{
    if (strbuf_getwholeline(sb, fp, '\n'))
        return EOF;
    if (sb->buf[sb->len - 1] == '\n') {
        strbuf_setlen(sb, sb->len - 1);
        if (sb->len > 0 && sb->buf[sb->len - 1] == '\r')
            strbuf_setlen(sb, sb->len - 1);
    }
    return 0;
}

/**
 * Like strbuf_getline_crlf(), but appends the line to a string_list and
 * returns it as a string. Returns NULL on EOF.
 */
static const char *string_list_getline_crlf(struct string_list *list, FILE *fp)
{
    struct strbuf sb = STRBUF_INIT;
    struct string_list_item *item;

    if (strbuf_getline_crlf(&sb, fp))
        return NULL;
    item = string_list_append_nodup(list, strbuf_detach(&sb, NULL));
    return item->string;
}

So now, is_email() can have access to previously-read lines, and if it needs some more, it can read more as well:

static int is_email(struct string_list *lines, FILE *fp)
{
    const char *header_regex = "^[!-9;-~]+:";
    regex_t regex;
    int ret = 1;
    size_t i;

    if (regcomp(&regex, header_regex, REG_NOSUB | REG_EXTENDED))
        die("Invalid search pattern: %s", header_regex);

    for (i = 0; i < lines->nr || string_list_getline_crlf(lines, fp); i++) {
        const char *line = lines->items[i].string;

        if (!*line)
            break; /* End of header */

        /* Ignore indented folded lines */
        if (*line == '\t' || *line == ' ')
            continue;

        /* It's a header if it matches header_regex */
        if (regexec(&regex, line, 0, NULL, 0)) {
            ret = 0;
            goto done;
        }
    }

done:
    regfree(&regex);
    return ret;
}

Which solves the problem of opening the file 2 times. What do you think?

Regards, Paul