From: Jeff King Date: Wed, 27 May 2015 22:13:56 GMT Subject: Re: [PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo Message-ID: <20150527221356.GE23259@peff.net> In-Reply-To: On Wed, May 27, 2015 at 01:44:26PM -0700, Junio C Hamano wrote: > Paul Tan 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