Re: [PATCH v2] trailer: allow spaces in tokens

From: Christian Couder <christian.couder@gmail.com>
Date: Aug 19, 2022, 10:29 UTC
Message-ID: <CAP8UFD2kMXHxvg1tKVNLhY0Gweq2YrD7+tHmZXKwguYzRQ1Qpg@mail.gmail.com>
In-Reply-To: <xmqqwnb4n98f.fsf@gitster.g>

On Fri, Aug 19, 2022 at 6:33 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 5 quoted lines

>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > In short, you do not support Max's patch that allows arbitrary
> > number of white spaces anywhere but at the beginning of line,

I haven't closely followed the patches that might have tweaked the original default behavior regarding whitespaces in tokens, but I think in general it's not a good idea to change the default behavior for no very good reason, because it could cause regressions in the way others already use `git interpret-trailers`. It could have been Ok to accept that during a few years after the original implementation was merged in 2014, but these days people and tools (like GitLab for example) rely on trailers and `git interpret-trailers` more and more.

So in general I think it's better to have a more cautious approach now, like we usually have for other commands, and only allow new options to tweak the default behavior. If one such option appears to be very widely used, then we might decide later that it should become the default behavior.

I understand that having commands with many options might not be considered by some of us as good design, but if we don't like that, then we could perhaps just have only one option, maybe called something like "trailer.tokenFormat", or just "trailer.format", that would contain a regex that every token, or trailer, should match.

It's a complex subject because there are different and competing viewpoints. On one hand it can be annoying to have some trailers ignored just because they don't match the default format in a very small way. (And rewriting trailers could mean rewriting published history which is not an easy thing to do.) On the other hand if we develop something like "trailer.tokenFormat" that allows everything, then we might consider that we should instead encourage people to do the right thing and to use as much as possible regular trailers with a strict format. So allowing new options to only tweak things in a small way might be the right thing to do after all.

Show 9 quoted lines

> > but see a room for unrelated improvement from the current code,
> > namely, to allow exactly one optional space, immediately before
> > the separator and nowhere else.
>
> Ah, no, sorry, I misread the situation.  It's not a room for
> improvement.  It is very close to what the current code already
> does, i.e. to allow optional spaces immediately before the
> separator.  The difference is that the current code allows arbitrary
> number of optional spaces, not zero or exactly one.

Yeah, I think it makes sense to not change this default behavior, but maybe, if people don't like it for some reason, allow options to tweak it.

> So the only thing we need to do is to update the documentation that
> Max misread as allowing spaces at arbitrary place to reflect what
> the code has been doing, i.e. an optional run of spaces is allowed
> between the "token" and the "separator"?

Yeah, I also think that the documentation should be very clear (and accurate of course) about where whitespaces and how many of them are allowed by default, and what can possibly be tweaked by options.

> Anybody wants to do the honors to produce such a patch, then?

Ok, I will give it a try soon.