On Mon, Oct 02, 2017 at 02:43:35AM -0400, Jeff King wrote:
> On Sun, Oct 01, 2017 at 10:53:11PM -0700, Taylor Blau wrote:
>
> > Peff points out that different atom parsers handle the empty
> > "sub-argument" list differently. An example of this is the format
> > "%(refname:)".
> >
> > Since callers often use `string_list_split` (which splits the empty
> > string with any delimiter as a 1-ary string_list containing the empty
> > string), this makes handling empty sub-argument strings non-ergonomic.
> >
> > Let's fix this by assuming that atom parser implementations don't care
> > about distinguishing between the empty string "%(refname:)" and no
> > sub-arguments "%(refname)".
>
> This looks good to me (both the explanation and the function of the
> code).
Thanks :-).
> But let me assume for a moment that your "please let me know" from the
> earlier series is still in effect, and you wish to be showered with
> pedantry and subjective advice. ;)
>
> I see a lot of newer contributors sending single patches as a 1-patch
> series with a cover letter. As a reviewer, I think this is mostly just a
> hassle. The cover letter ends up mostly repeating the same content from
> the single commit, so readers end up having to go over it twice (and you
> ended up having to write it twice).
>
> Sometimes there _is_ useful information to be conveyed that doesn't
> belong in the commit message, but that can easily go after the "---" (or
> before a "-- >8 --" if you really feel it should be read before the
> commit message.
>
> In general, if you find yourself writing a really long cover letter, and
> especially one that isn't mostly "meta" information (like where this
> should be applied, or what's changed since the last version), you should
> consider whether that information ought to go into the commit message
> instead. The one exception is if you _do_ have a long series and you
> need to sketch out the approach to help the reader see the big picture
> (in which case your cover letter should be summarizing what's already in
> the commit messages).
Thank you for this advice. I was worried when writing my cover letter
last night that it would be considered repetitive, but I wasn't sure how
much brevity/detail would be desired in a patch series of this length.
I'll keep this in mind for the future.
> And before anybody digs in the list to find my novel-length cover
> letters to throw back in my face, I know that I'm very guilty of this.
> I'm trying to get better at it, and passing it on so you can learn from
> my mistakes. :)
I appreciate your humility ;-).
> > - if (arg)
> > + if (arg) {
> > arg = used_atom[at].name + (arg - atom) + 1;
> > + if (!*arg) {
> > + /*
> > + * string_list_split is often use by atom parsers to
> > + * split multiple sub-arguments for inspection.
> > + *
> > + * Given a string that does not contain a delimiter
> > + * (example: ""), string_list_split returns a 1-ary
> > + * string_list that requires adding special cases to
> > + * atom parsers.
> > + *
> > + * Thus, treat the empty argument string as NULL.
> > + */
> > + arg = NULL;
> > + }
> > + }
>
> I know this is getting _really_ subjective, but IMHO this is a lot more
> reasoning than the comment needs. The commit message goes into the
> details of the "why", but here I'd have just written something like:
>
> /* treat "%(foo:)" the same as "%(foo)"; i.e., no arguments */
I sent an updated v2 of this "series" (without a cover-letter) that
shortens this comment to more or less what you suggested. I've kept the
commit message longer, since I think that that information is useful
within "git blame".
--
- Taylor
