Re: Questions about trailer configuration semantics

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

On Mon, Jul 27, 2020 at 11:41 PM Anders Waldenborg <anders@0x63.nu> wrote:

> Christian Couder writes:

[...]

> >> > 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.
>
> Maybe what I'm missing is a clear picture of the different cases that
> "git interpret-trailers" is being used in. The "--trailer x=10" option
> seems clearly designed for human input trying to be as helpful as
> possible (e.g always allowing '=' regardless of separators
> configured). But when reading a message with trailers, is same
> helpfulness useful? Or is it always only reading proper trailers
> previously added by --trailer?

I guess it depends on the purpose of reading a message with trailers.
If you want to do stats for example, do you really want to consider
"Reviewed", "Reviewer" and "Reviewed-by" as different trailers in your
stats?

> The standardization of trailers is interesting. If I want to standardize
> "ReviewedBy", "Reviewer", "Code-Reviewer" trailers to "Reviewed-By" I
> would add these configs:
>
> trailer.reviewer.key = Reviewed-By
> trailer.ReviewedBy.key = Reviewed-By
> trailer.Code-Reviewer.key = Reviewed-By
>
> Seems useful (and works out of the box as a msg-filter to
> filter-branch). But the configuration seems a bit backwards, it is
> configured a 3 different trailer entries, rather that a single trailer
> entry with three aliases (or something like that).

Maybe taking advantage of the shortcuts and using only the following could work:

trailer.review.key = Reviewed-By
trailer.code-review.key = Reviewed-By

> >> > 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.
>
> Yeah, that works. But that dependency on order of configuration is quite
> subtle imho.

[...]

Yeah, I agree that documenting this would be nice.

> >> > * 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.
>
> Keep in mind that the machinery used by interpret-trailers is also used
> by pretty '%(trailers)' so whatever normalization and shortcuts
> happening also shows up there. Is shortcuts useful in that case too?
> There the input is commit history, not some user input.

Pretty %(trailer) was added later by someone else, but I guess it also
depends on the use case. Is it going to be used for stats? Is it
interesting to distinguish between very similar trailers in these
stats?

And again if people are interested in very strict processing, then
adding an option for that could be the right thing to do.

[...]

> There is also some inconsistency here. If one use '%(trailers) the
> normalization doesn't happen. Only if using '%(trailers:only)' or some
> other option.
>
> (because optimization in format_trailer_info:
>  /* If we want the whole block untouched, we can take the fast path. */)

Maybe that's a bug. Peff, it looks like you added the above comment.
Do you think it's a bug?

> I guess the fact that expansion happens also in these cases can get
> confusing if I have configured a trailer "Bug-Introduced-In" locally and
> the commit message contains "Bug: 123".
>
> 'git log --pretty=format:"%h% (trailers:key=Bug-Introduced-In,valueonly,separator=%x20)"'
> would pick up data from the wrong trailer just because I added
> configuration for Bug-Introduced-In trailer.

Yeah, I understand that it could be an issue.

> >> > 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.
>
> Should I keep the "test_expect_failure" tests, or change into expecting
> current behavior and switch them over to "test_except_success"?

I think you should switch them over to "test_except_success".

> I'll see if I can do something about documentation.

Thanks,
Christian.