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

[Jeff King <peff@peff.net>]

On Wed, May 27, 2015 at 01:44:26PM -0700, Junio C Hamano 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.

Just my 2 cents, but I think that grouping and overhead comments can
often make things more obvious. For example:

  struct am_state {
	/* state directory path */
	struct strbuf dir;

	/*
	 * current and last patch numbers; are these 1-indexed
	 * or 0-indexed?
	 */
	int cur;
	int last;

	struct strbuf author_name;
	struct strbuf author_email;
	struct strbuf author_date;
	struct strbuf msg;

	/* number of digits in patch filename */
	int prec;
  }

Note that by grouping "cur" and "last", we can discuss them as a group,
and the overhead comment gives us room to mention their shared
properties (my indexing question is a real question, btw, whose answer I
think would be useful to mention in a comment).

Likewise, by grouping the "msg" strbuf with the author information, it
becomes much more clear that they are all about the commit metadata
(though I would not be opposed to a comment above the whole block,
either).

-Peff