Re: [PATCH] ref-filter.c: pass empty-string as NULL to atom parsers

[Jeff King <peff@peff.net>]

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).

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).

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. :)

> -	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 */

-Peff