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

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

[Taylor Blau]

Hi,

Attached is a one-long patch series to un-distinguish between atoms
without sub-arguments ("%(refname)") and atoms with empty sub-argument
lists ("%(refname:)").

This addresses a user-experience issue that Peff points out:

> Doh, that string_list behavior is what I was missing in my earlier
> comments. I agree this is probably the best way of doing it. I'm tempted
> to say that parse_ref_filter_atom() should do a similar thing. Right now
> we've got:
>
>   $ git for-each-ref --format='%(refname)' | wc
>      2206    2206   79929
>   $ git for-each-ref --format='%(refname:short)' | wc
>      2206    2206   53622
>   $ git for-each-ref --format='%(refname:)' | wc
>   fatal: unrecognized %(refname:) argument:
>       0       0       0

By treating %(refname) and %(refname:) as the same thing. Peff has
convinced me that these _are_ indeed the same thing, as the first is a
%(refname) atom without any sub-arguments, and the later is a %(refname)
%atom with empty sub-arguments.

The reasoning is highlighted in the comment this patch adds, which makes
more ergonomic the use of string_list_split in atom parser
implementations.

Thank you in advance :-).


--
- Taylor

[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 <me@ttaylorr.com>
---
 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(&used_atom[at].u, 0, sizeof(used_atom[at].u));
 	if (valid_atom[i].parser)
 		valid_atom[i].parser(format, &used_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

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

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

[PATCH v2] 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 <me@ttaylorr.com>
---
 ref-filter.c            | 10 +++++++++-
 t/t6300-for-each-ref.sh |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index bc591f4f3..f3e53d444 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -415,8 +415,16 @@ 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) {
+			/*
+			 * Treat empty sub-arguments list as NULL (i.e.,
+			 * "%(atom:)" is equivalent to "%(atom)").
+			 */
+			arg = NULL;
+		}
+	}
 	memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));
 	if (valid_atom[i].parser)
 		valid_atom[i].parser(format, &used_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

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

[Jeff King]

On Mon, Oct 02, 2017 at 09:10:34AM -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)".
> 
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  ref-filter.c            | 10 +++++++++-
>  t/t6300-for-each-ref.sh |  1 +
>  2 files changed, 10 insertions(+), 1 deletion(-)

This looks good to me, thanks.

-Peff

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

[Jonathan Nieder]

Hi,

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)".
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  ref-filter.c            | 10 +++++++++-
>  t/t6300-for-each-ref.sh |  1 +
>  2 files changed, 10 insertions(+), 1 deletion(-)

The above does a nice job of explaining

 - what this change is going to do
 - how it's good for the internal code structure / maintainability

What it doesn't tell me about is why the user-facing effect won't
cause problems.  Is there no atom where %(atom:) was previously
accepted and did something meaningful that this may break?

Looking at the manpage and code, I don't see any, so for what it's
worth, this is

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

but for next time, please remember to discuss regression risk in
the commit message, too.

Thanks,
Jonathan

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The above does a nice job of explaining
>
>  - what this change is going to do
>  - how it's good for the internal code structure / maintainability
>
> What it doesn't tell me about is why the user-facing effect won't
> cause problems.  Is there no atom where %(atom:) was previously
> accepted and did something meaningful that this may break?

That is, was there any situation where %(atom) and %(atom:) did two
differnt things and their differences made sense?

> Looking at the manpage and code, I don't see any, so for what it's
> worth, this is
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> but for next time, please remember to discuss regression risk in
> the commit message, too.

Yes, I agree that it is necessary to make sure somebody looked at
the issue _and_ record the fact that it happened.  Thanks for doing
that already ;-)

I also took a look at the code and currently we seem to abort,
either with "unrecognised arg" (e.g. "refname:") or "does not take
args" (e.g. "body"), so we should be OK, I'd think.

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

[Taylor Blau]

On Tue, Oct 03, 2017 at 08:55:01AM +0900, Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
> > The above does a nice job of explaining
> >
> >  - what this change is going to do
> >  - how it's good for the internal code structure / maintainability
> >
> > What it doesn't tell me about is why the user-facing effect won't
> > cause problems.  Is there no atom where %(atom:) was previously
> > accepted and did something meaningful that this may break?
>
> That is, was there any situation where %(atom) and %(atom:) did two
> differnt things and their differences made sense?
>
> > Looking at the manpage and code, I don't see any, so for what it's
> > worth, this is
> >
> > Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> >
> > but for next time, please remember to discuss regression risk in
> > the commit message, too.
>
> Yes, I agree that it is necessary to make sure somebody looked at
> the issue _and_ record the fact that it happened.  Thanks for doing
> that already ;-)
>
> I also took a look at the code and currently we seem to abort,
> either with "unrecognised arg" (e.g. "refname:") or "does not take
> args" (e.g. "body"), so we should be OK, I'd think.

Thank you both for the helpful pointers. I will make sure to include a
more thorough review of potential breakage in existing code given a
particular change.

With respect to this particular patch, I agree with Jonathan and Junio
that there are no places in ref-filter.c that would be affected by this
change, and that it should be able to be applied cleanly without
breakage.

--
- Taylor

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> On Tue, Oct 03, 2017 at 08:55:01AM +0900, Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>> > The above does a nice job of explaining
>> >
>> >  - what this change is going to do
>> >  - how it's good for the internal code structure / maintainability
>> >
>> > What it doesn't tell me about is why the user-facing effect won't
>> > cause problems.  Is there no atom where %(atom:) was previously
>> > accepted and did something meaningful that this may break?

This loose end needs to be tied.  

I replaced "let's assume that doing this change is OK" from the
original log message with something a bit stronger, as with this
change, we are saying that it is forbidden to treat %(atom) and
%(atom:) differently.  I also recorded the result of due-diligence
survey of the current code to suggest that the change would be OK
for current users.

-- >8 --
From: Taylor Blau <me@ttaylorr.com>
Date: Mon, 2 Oct 2017 09:10:34 -0700
Subject: [PATCH] ref-filter.c: pass empty-string as NULL to atom parsers

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 declaring that atom parser implementations must
not care about distinguishing between the empty string "%(refname:)"
and no sub-arguments "%(refname)".  Current code aborts, either with
"unrecognised arg" (e.g. "refname:") or "does not take args"
(e.g. "body:") as an error message.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
Reviewed-by: Jeff King <peff@peff.net>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ref-filter.c            | 10 +++++++++-
 t/t6300-for-each-ref.sh |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index bc591f4f3d..f3e53d4448 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -415,8 +415,16 @@ 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) {
+			/*
+			 * Treat empty sub-arguments list as NULL (i.e.,
+			 * "%(atom:)" is equivalent to "%(atom)").
+			 */
+			arg = NULL;
+		}
+	}
 	memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));
 	if (valid_atom[i].parser)
 		valid_atom[i].parser(format, &used_atom[at], arg);
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2274a4b733..edc1bd8eab 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.2-921-g20a440a8ba

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

[Jonathan Nieder]

Junio C Hamano wrote:

> From: Taylor Blau <me@ttaylorr.com>
> Date: Mon, 2 Oct 2017 09:10:34 -0700
> Subject: [PATCH] ref-filter.c: pass empty-string as NULL to atom parsers
>
> 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 declaring that atom parser implementations must
> not care about distinguishing between the empty string "%(refname:)"
> and no sub-arguments "%(refname)".  Current code aborts, either with
> "unrecognised arg" (e.g. "refname:") or "does not take args"
> (e.g. "body:") as an error message.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> Reviewed-by: Jeff King <peff@peff.net>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  ref-filter.c            | 10 +++++++++-
>  t/t6300-for-each-ref.sh |  1 +
>  2 files changed, 10 insertions(+), 1 deletion(-)

Thanks for taking care of it.  This is indeed still
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>