git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Christian Couder <christian.couder@gmail.com>
Cc: git@vger.kernel.org, Anders Waldenborg <anders@0x63.nu>
Subject: Re: Questions about trailer configuration semantics
Date: Mon, 27 Jul 2020 10:18:39 -0700	[thread overview]
Message-ID: <xmqqr1swg9lc.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <87blk0rjob.fsf@0x63.nu> (Anders Waldenborg's message of "Mon, 27 Jul 2020 18:45:24 +0200")

[Redirecting it to the resident expert of the trailers]

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"
>
> 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)
>
>
> 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"
>
> (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". 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.
>
> (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?
>
> * If so should it only happen when there is a .key config?
>
> * Should replacement to what is in .key happen also in --parse mode, or
>   only for "--trailer"
>
> * The prefix matching gotta be a bug, right?
>
>
>
> Here is a patch to the tests showing these things.
>
>
>
> From 49a4bb64a7ebf1f2d50897a024deb86b4f8056b1 Mon Sep 17 00:00:00 2001
> From: Anders Waldenborg <anders@0x63.nu>
> Date: Mon, 27 Jul 2020 18:34:37 +0200
> Subject: [PATCH] trailers: add tests for unclear/undocumented behavior
>
> ---
>  t/t7513-interpret-trailers.sh | 70 +++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
> index 2e6d406edf..d5d19cf89b 100755
> --- a/t/t7513-interpret-trailers.sh
> +++ b/t/t7513-interpret-trailers.sh
> @@ -99,6 +99,64 @@ test_expect_success 'with config option on the command line' '
>  	test_cmp expected actual
>  '
>
> +test_expect_success 'parse normalizes spelling and separators from configs with key' '
> +	cat >patch <<-\EOF &&
> +		non-trailer-line
> +
> +		ReviEweD-bY :abc
> +		ReviEwEd-bY) rst
> +		ReviEweD-BY ; xyz
> +		aCked-bY: not normalized
> +	EOF
> +	cat >expected <<-\EOF &&
> +		Reviewed-By: abc
> +		Reviewed-By: rst
> +		Reviewed-By: xyz
> +		aCked-bY: not normalized
> +	EOF
> +	git \
> +		-c "trailer.separators=:);" \
> +		-c "trailer.rb.key=Reviewed-By" \
> +		-c "trailer.Acked-By.ifmissing=doNothing" \
> +		interpret-trailers --parse patch >actual &&
> +	test_cmp expected actual
> +'
> +
> +# Matching currently is prefix matching, causing "This-trailer" to be normalized too
> +test_expect_failure 'config option matches exact only' '
> +	cat >patch <<-\EOF &&
> +
> +		This-trailer: a
> +		 b
> +		This-trailer-exact: b
> +		 c
> +		This-trailer-exact-plus-some: c
> +		 d
> +	EOF
> +	cat >expected <<-\EOF &&
> +		This-trailer: a b
> +		THIS-TRAILER-EXACT: b c
> +		This-trailer-exact-plus-some: c d
> +	EOF
> +	git -c "trailer.tte.key=THIS-TRAILER-EXACT" interpret-trailers --parse patch >actual &&
> +	test_cmp expected actual
> +'
> +
> +# Matching currently uses the config key even if key value is different
> +test_expect_failure 'config option matches exact only' '
> +	cat >patch <<-\EOF &&
> +
> +		Ticket: 1234
> +		Reference-ticket: 99
> +	EOF
> +	cat >expected <<-\EOF &&
> +		Ticket: 1234
> +		Reference-Ticket: 99
> +	EOF
> +	git -c "trailer.ticket.key=Reference-Ticket" interpret-trailers --parse patch >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'with only a title in the message' '
>  	cat >expected <<-\EOF &&
>  		area: change
> @@ -473,6 +531,18 @@ test_expect_success 'with config setup' '
>  	test_cmp expected actual
>  '
>
> +# currently this matches the "Acked-by: " value in ack key set by previous test
> +test_expect_failure 'with config setup matches key exactly' '
> +	cat >expected <<-\EOF &&
> +
> +		A: B
> +	EOF
> +	git interpret-trailers --trailer "A=10" empty >actual &&
> +	test_cmp expected actual
> +'
> +
> +
> +
>  test_expect_success 'with config setup and ":=" as separators' '
>  	git config trailer.separators ":=" &&
>  	git config trailer.ack.key "Acked-by= " &&
> --
> 2.25.1

  reply	other threads:[~2020-07-27 17:18 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 [this message]
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
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 \
    --in-reply-to=xmqqr1swg9lc.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=anders@0x63.nu \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --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).