From: Anders Waldenborg <firstname.lastname@example.org> To: Christian Couder <email@example.com> Cc: Junio C Hamano <firstname.lastname@example.org>, git <email@example.com>, Jeff King <firstname.lastname@example.org>, Jonathan Tan <email@example.com> Subject: Re: Questions about trailer configuration semantics Date: Mon, 27 Jul 2020 23:41:16 +0200 [thread overview] Message-ID: <firstname.lastname@example.org> (raw) In-Reply-To: <CAP8UFD1XV_jN10yOc2o4=5PtPcvT-RbxhY1H3swZz2r4g-Uzkw@mail.gmail.com> Christian Couder writes: >> > When configuring a value in trailer.<token>.key it causes the trailer to >> > be normalized to that in "git interpret-trailers --parse". > > 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. > > Yeah, in this case we are not sure if "Acked" or "aCKed" is the right > way to spell it. 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. >> > 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? 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). >> > (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. 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 > >> > (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. 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 <email@example.com> Attest: Junio C Hamano <firstname.lastname@example.org> 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. >> > * 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. 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.
next prev parent reply other threads:[~2020-07-27 21:41 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-27 16:45 Anders Waldenborg 2020-07-27 17:18 ` Junio C Hamano 2020-07-27 18:37 ` Christian Couder 2020-07-27 19:40 ` Jeff King 2020-07-27 22:57 ` Anders Waldenborg 2020-07-27 23:42 ` Jeff King 2020-07-27 20:11 ` Junio C Hamano 2020-07-27 22:17 ` Anders Waldenborg 2020-07-27 23:05 ` Junio C Hamano 2020-07-28 0:01 ` Anders Waldenborg 2020-07-27 21:41 ` Anders Waldenborg [this message] 2020-07-27 22:53 ` Junio C Hamano 2020-07-27 23:17 ` Anders Waldenborg 2020-07-28 7:07 ` Christian Couder 2020-07-28 15:41 ` Jeff King 2020-07-28 16:40 ` Junio C Hamano
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: Questions about trailer configuration semantics' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).