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

[Jeff King]

On Mon, Oct 02, 2017 at 09:12:58AM -0700, Taylor Blau wrote:

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

Yeah, sorry if I wasn't clear: definitely the commit message is fine and
is the place to go into the detail and rationale.

-Peff

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

[Taylor Blau]

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

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

[Jeff King]

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

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

[Taylor Blau]

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

Signed-off-by: Taylor Blau 
---
 ref-filter.c| 17 -
 t/t6300-for-each-ref.sh |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index bc591f4f3..22be4097a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -415,8 +415,23 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
REALLOC_ARRAY(used_atom, used_atom_cnt);
used_atom[at].name = xmemdupz(atom, ep - atom);
used_atom[at].type = valid_atom[i].cmp_type;
-   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;
+   }
+   }
memset(_atom[at].u, 0, sizeof(used_atom[at].u));
if (valid_atom[i].parser)
valid_atom[i].parser(format, _atom[at], arg);
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2274a4b73..edc1bd8ea 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -51,6 +51,7 @@ test_atom() {
 }
 
 test_atom head refname refs/heads/master
+test_atom head refname: refs/heads/master
 test_atom head refname:short master
 test_atom head refname:lstrip=1 heads/master
 test_atom head refname:lstrip=2 master
-- 
2.14.1.145.gb3622a4ee