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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 2, 2017, 23:55 UTC
Message-ID: <xmqqmv59m7fu.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171002224052.GR19555@aiede.mtv.corp.google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 8 quoted lines

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

Show 7 quoted lines

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