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

[Christian Couder <christian.couder@gmail.com>]

On Fri, Aug 19, 2022 at 6:33 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

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