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

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

Paul Tan [off-list ref] writes:

quoted

@@ -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