On Thu, May 28, 2015 at 4:44 AM, Junio C Hamano <[email protected]> wrote:
> Paul Tan <[email protected]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
