Re: Questions about trailer configuration semantics

Christian Couder writes:

Makes sense.

However I guess one alternative implementation would be that if trailer.X.something is configured but not trailer.X.key it would work as if there was an implicit "trailer.X.key=X" configured. The name of the configuration value would specify the correct spelling.

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?

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

Yeah, that works. But that dependency on order of configuration is quite subtle imho.

E.g consider:

$ cat >msg <<EOF

Acked: acked
Acked-Test: test
Acked-Docs: docs
EOF
$ git -c 'trailer.Acked.key=Acked' \
      -c 'trailer.Acked-Tests.key=Acked-Tests' \
      -c 'trailer.Acked-Docs.key=Acked-Docs' \
      interpret-trailers --parse msg
Acked: acked
Acked-Tests: test
Acked-Docs: docs
$ git -c 'trailer.Acked-Docs.key=Acked-Docs' \
      -c 'trailer.Acked-Tests.key=Acked-Tests' \
      -c 'trailer.Acked.key=Acked' \
      interpret-trailers --parse msg
Acked-Docs: acked
Acked-Tests: test
Acked-Docs: docs
$ git -c 'trailer.Acked-Tests.key=Acked-Tests' \
      -c 'trailer.Acked-Docs.key=Acked-Docs' \
      -c 'trailer.Acked.key=Acked' \
      interpret-trailers --parse msg
Acked-Tests: acked
Acked-Tests: test
Acked-Docs: docs

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.

E.g:
$ git -c 'trailer.signed-off-by.key=Attest' \
      show --pretty='format:%(trailers:key=Attest)' --no-patch \
      0172f7834a31ae7cb9873feaaaa63939352fa3ae
Attest: Christian Couder <chriscool@tuxfamily.org>
Attest: Junio C Hamano <gitster@pobox.com>

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. */)

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.

Should I keep the "test_expect_failure" tests, or change into expecting current behavior and switch them over to "test_except_success"?

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