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

From: Jeff King <peff@peff.net>
Date: May 27, 2015, 22:13 UTC
Message-ID: <20150527221356.GE23259@peff.net>
In-Reply-To: <xmqqtwuxkaxh.fsf@gitster.dls.corp.google.com>

On Wed, May 27, 2015 at 01:44:26PM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> 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