Re: [PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo

[Paul Tan <pyokagan@gmail.com>]

On Thu, May 28, 2015 at 4:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>
>> @@ -17,6 +34,10 @@ struct am_state {
>>       struct strbuf dir;            /* state directory path */
>>       int cur;                      /* current patch number */
>>       int last;                     /* last patch number */
>> +     struct strbuf msg;            /* commit message */
>> +     struct strbuf author_name;    /* commit author's name */
>> +     struct strbuf author_email;   /* commit author's email */
>> +     struct strbuf author_date;    /* commit author's date */
>>       int prec;                     /* number of digits in patch filename */
>>  };
>
> I always get suspicious when structure fields are overly commented,
> wondering if it is a sign of naming fields poorly.  All of the above
> fields look quite self-explanatory and I am not sure if it is worth
> having these comments, spending efforts to type SP many times to
> align them and all.

Okay, I'll take Jeff's suggestion to organize them into blocks.

>> +static int read_author_script(struct am_state *state)
>> +{
>> +     char *value;
>> +     struct strbuf sb = STRBUF_INIT;
>> +     const char *filename = am_path(state, "author-script");
>> +     FILE *fp = fopen(filename, "r");
>> +     if (!fp) {
>> +             if (errno == ENOENT)
>> +                     return 0;
>> +             die(_("could not open '%s' for reading"), filename);
>
> Hmph, do we want to report with die_errno()?

Yes, we do.

>> +     }
>> +
>> +     if (strbuf_getline(&sb, fp, '\n'))
>> +             return -1;
>> +     if (!skip_prefix(sb.buf, "GIT_AUTHOR_NAME=", (const char**) &value))
>
> This cast is unfortunate; can't "value" be of "const char *" type?

We can't, because sq_dequote() modifies the string directly. I would
think casting from non-const to const is safer than the other way
around.

Thanks,
Paul