Re: Questions about trailer configuration semantics

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

[Adding Peff and Jonathan in Cc as they know also about this area of the code]

On Mon, Jul 27, 2020 at 7:18 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> [Redirecting it to the resident expert of the trailers]

Thanks!

> Anders Waldenborg <anders@0x63.nu> writes:
>
> > I noticed some undocumented and (at least to me) surprising behavior in
> > trailers.c.
> >
> > When configuring a value in trailer.<token>.key it causes the trailer to
> > be normalized to that in "git interpret-trailers --parse".
> > E.g:
> >  $ printf '\naCKed: Zz\n' | \
> >    git -c 'trailer.Acked.key=Acked' interpret-trailers --parse
> >  will emit: "Acked: Zz"

Yeah, I think that's nice, as it can make sure that the key appears in
the same way. It's true that it would be better if it would be
documented.

> > but only if "key" is used, other config options doesn't cause it to be
> > normalized.
> > E.g:
> >  $ printf '\naCKed: Zz\n' | \
> >    git -c 'trailer.Acked.ifmissing=doNothing' interpret-trailers --parse
> >  will emit: "aCKed: Zz" (still lowercase a and uppercase CK)

Yeah, in this case we are not sure if "Acked" or "aCKed" is the right
way to spell it.

> > Then there is the replacement by config "trailer.fix.key=Fixes" which
> > expands "fix" to "Fixes". This happens when using "--trailer 'fix = 123'"
> > which seems to be expected and useful behavior (albeit a bit unclear in
> > documentation). But it also happens when parsing incoming trailers, e.g
> > with that config
> >  $ printf "\nFix: 1\n" | git interpret-trailers --parse
> >  will emit: "Fixes: 1"

Yeah, I think it allows for shortcuts and can help with standardizing
the keys in commit messages.

> > (token_from_item prefers order .key, incoming token, .name)
> >
> >
> > The most surprising thing is that it uses prefix matching when finding
> > they key in configuration. If I have "trailer.reviewed.key=Reviewed-By"
> > it is possible to just '--trailer r=XYZ' and it will find the
> > reviewed-by trailer as "r" is a prefix of reviewedby. This also applies
> > to the "--parse".

Yeah, that's also for shortcuts and standardization.

> > This in makes it impossible to have trailer keys that
> > are prefix of each other (e.g: "Acked", "Acked-Tests", "Acked-Docs") if
> > there is multiple matching in configuration it will just pick the one
> > that happens to come first.

That's a downside of the above. I agree that it might seem strange or
bad. Perhaps an option could be added to implement a strict matching,
if people really want it.

Also if you configure trailers in the "Acked", "Acked-Tests",
"Acked-Docs" order, then any common prefix will pick "Acked" which
could be considered ok in my opinion.

> > (token_matches_item uses strncasecmp with token length)
> >
> >
> > I guess these are the questions for the above observations:
> >
> > * Should normalization of spelling happen at all?

Yes, I think it can help.

> > * If so should it only happen when there is a .key config?

Yes, it can help too if that only happens when there is a .key config.

> > * Should replacement to what is in .key happen also in --parse mode, or
> >   only for "--trailer"

I think it's more consistent if it happens in both --parse and
--trailer mode. I didn't implement --parse though.

> > * The prefix matching gotta be a bug, right?

No, it's a feature ;-) Seriously I agree that this could be seen as a
downside, but I think it can be understood that the convenience is
worth it. And in case someone is really annoyed by this, then adding
an option for strict matching should not be very difficult.

> > Here is a patch to the tests showing these things.

Thanks for the patch! I would be ok to add such a patch to the test
suite if it was sent like a regular patch (so with a commit message, a
Signed-off-by: and so on) to the mailing list. While at it some
documentation of the related behavior would also be very nice.