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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Paul Tan <pyokagan@xxxxxxxxx> 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.

By the way, the overall structure of the series look sensible.

> +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()?

> +	}
> +
> +	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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]