From: Jeff King <peff@peff.net>
To: Anders Waldenborg <anders@0x63.nu>
Cc: Christian Couder <christian.couder@gmail.com>,
git <git@vger.kernel.org>,
Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH 06/21] t4205: add test for trailer in log with nonstandard separator
Date: Tue, 10 Nov 2020 14:54:39 -0500 [thread overview]
Message-ID: <20201110195439.GC1987088@coredump.intra.peff.net> (raw)
In-Reply-To: <87y2jap4ch.fsf@0x63.nu>
On Mon, Nov 09, 2020 at 11:12:14PM +0100, Anders Waldenborg wrote:
> > Why is this new test important?
>
> The test that checks that 'git log --pretty=format:%(trailers)' shows
> the output in the form "Closes: 1234" even if input was "Closes #1234"
> is interesting both because it checks that this behavior is kept intact
> in the patches later in the series which modifies handling of separator
> and because it is a behavior that can be surprising and not well defined
> in documentation and those tend to be the ones that are easiest to
> accidentally break. Maybe the addition of the test should come later in
> the series where the changes that potentially could break it happen.
That makes sense, but should be in the commit message.
I also found the expected output confusing. I thought at first we were
mis-parsing to include part of the subject in the trailer, but it is
just that we put "%s" into the format argument.
> It seems like you stopped reviewing my patch series at patch 06/21. That
> is IMHO just before it starts to get interesting :) Now I don't know if
> rest of it was rubbish or uninteresting or just there was no time to
> look at it.
>
> I've updated according to the suggestions, but not sure if I should
> repost the series with just such small adjustments.
Reviewing this has been on my todo list, but I'd just as soon do it from
your latest version. Since it has been a while, it may make sense to
just repost with the fixes, and note in the cover letter that it didn't
get a lot of review yet.
-Peff
next prev parent reply other threads:[~2020-11-10 19:55 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-25 21:26 [PATCH 00/21] trailer fixes Anders Waldenborg
2020-10-25 21:26 ` [PATCH 01/21] trailer: change token_{from,matches}_item into taking conf_info Anders Waldenborg
2020-10-26 11:56 ` Christian Couder
2020-10-25 21:26 ` [PATCH 02/21] trailer: don't use 'struct arg_item' for storing config Anders Waldenborg
2020-10-25 21:26 ` [PATCH 03/21] doc: mention canonicalization in git i-t manual Anders Waldenborg
2020-10-26 12:14 ` Christian Couder
2020-10-25 21:26 ` [PATCH 04/21] pretty: allow using aliases in %(trailer:key=xyz) Anders Waldenborg
2020-10-26 12:38 ` Christian Couder
2020-10-25 21:26 ` [PATCH 05/21] trailer: rename 'free_all' to 'free_all_trailer_items' Anders Waldenborg
2020-10-26 12:42 ` Christian Couder
2020-11-10 19:52 ` Jeff King
2020-10-25 21:26 ` [PATCH 06/21] t4205: add test for trailer in log with nonstandard separator Anders Waldenborg
2020-10-26 12:43 ` Christian Couder
2020-11-09 22:12 ` Anders Waldenborg
2020-11-10 7:55 ` Christian Couder
2020-11-10 19:54 ` Jeff King [this message]
2020-10-25 21:26 ` [PATCH 07/21] trailer: simplify 'arg_item' lifetime Anders Waldenborg
2020-10-25 21:26 ` [PATCH 08/21] trailer: keep track of conf in trailer_item Anders Waldenborg
2020-11-10 19:58 ` Jeff King
2020-10-25 21:26 ` [PATCH 09/21] trailer: refactor print_tok_val into taking item Anders Waldenborg
2020-10-25 21:26 ` [PATCH 10/21] trailer: move trailer token canonicalization print time Anders Waldenborg
2020-10-25 21:26 ` [PATCH 11/21] trailer: remember separator used in input Anders Waldenborg
2020-10-25 21:26 ` [PATCH 12/21] trailer: handle configured nondefault separators explicitly Anders Waldenborg
2020-11-10 20:06 ` Jeff King
2020-10-25 21:26 ` [PATCH 13/21] trailer: add option to make canonicalization optional Anders Waldenborg
2020-11-10 20:10 ` Jeff King
2020-10-25 21:26 ` [PATCH 14/21] trailer: move skipping of blank lines to own loop when finding trailer Anders Waldenborg
2020-10-25 21:26 ` [PATCH 15/21] trailer: factor out classify_trailer_line Anders Waldenborg
2020-10-25 21:26 ` [PATCH 16/21] t7513: add failing test for configured trailing line classification Anders Waldenborg
2020-10-25 21:26 ` [PATCH 17/21] trailer: don't treat line with prefix of known trailer as known Anders Waldenborg
2020-11-10 20:16 ` Jeff King
2020-10-25 21:26 ` [PATCH 18/21] trailer: factor out config lookup to separate function Anders Waldenborg
2020-10-25 21:26 ` [PATCH 19/21] trailer: move config lookup out of parse_trailer Anders Waldenborg
2020-10-25 21:26 ` [PATCH 20/21] trailer: add failing tests for matching trailers against input Anders Waldenborg
2020-10-25 21:26 ` [PATCH 21/21] trailer: only do prefix matching for configured trailers on commandline Anders Waldenborg
2020-11-10 7:44 ` [PATCH 00/21] trailer fixes Christian Couder
2020-12-05 1:39 ` [PATCH 0/5] pretty format %(trailers): improve machine readability Ævar Arnfjörð Bjarmason
2020-12-05 18:18 ` Anders Waldenborg
2020-12-07 8:53 ` Ævar Arnfjörð Bjarmason
2020-12-06 0:24 ` [PATCH v2 " Ævar Arnfjörð Bjarmason
2020-12-09 15:52 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
2020-12-10 10:48 ` Christian Couder
2020-12-10 19:00 ` Junio C Hamano
2020-12-09 15:52 ` [PATCH v3 1/5] pretty format %(trailers) test: split a long line Ævar Arnfjörð Bjarmason
2020-12-09 15:52 ` [PATCH v3 2/5] pretty format %(trailers) doc: avoid repetition Ævar Arnfjörð Bjarmason
2020-12-10 19:01 ` Junio C Hamano
2020-12-09 15:52 ` [PATCH v3 3/5] pretty-format %(trailers): fix broken standalone "valueonly" Ævar Arnfjörð Bjarmason
2020-12-09 15:52 ` [PATCH v3 4/5] pretty format %(trailers): add a "keyonly" Ævar Arnfjörð Bjarmason
2020-12-09 15:52 ` [PATCH v3 5/5] pretty format %(trailers): add a "key_value_separator" Ævar Arnfjörð Bjarmason
2020-12-06 0:24 ` [PATCH v2 1/5] pretty format %(trailers) test: split a long line Ævar Arnfjörð Bjarmason
2020-12-06 0:24 ` [PATCH v2 2/5] pretty format %(trailers) doc: avoid repetition Ævar Arnfjörð Bjarmason
2020-12-07 9:09 ` Christian Couder
2020-12-06 0:24 ` [PATCH v2 3/5] pretty-format %(trailers): fix broken standalone "valueonly" Ævar Arnfjörð Bjarmason
2020-12-06 0:24 ` [PATCH v2 4/5] pretty format %(trailers): add a "keyonly" Ævar Arnfjörð Bjarmason
2020-12-07 9:17 ` Christian Couder
2020-12-06 0:24 ` [PATCH v2 5/5] pretty format %(trailers): add a "key_value_separator" Ævar Arnfjörð Bjarmason
2020-12-05 1:39 ` [PATCH 1/5] pretty format %(trailers) test: split a long line Ævar Arnfjörð Bjarmason
2020-12-05 1:39 ` [PATCH 2/5] pretty format %(trailers): avoid needless repetition Ævar Arnfjörð Bjarmason
2020-12-05 5:43 ` Christian Couder
2020-12-05 1:39 ` [PATCH 3/5] pretty format %(trailers): add a "keyonly" Ævar Arnfjörð Bjarmason
2020-12-05 6:11 ` Christian Couder
2020-12-05 12:26 ` Ævar Arnfjörð Bjarmason
2020-12-05 1:39 ` [PATCH 4/5] pretty-format %(trailers): fix broken standalone "valueonly" Ævar Arnfjörð Bjarmason
2020-12-05 6:46 ` Christian Couder
2020-12-05 1:39 ` [PATCH 5/5] pretty format %(trailers): add a "key_value_separator" Ævar Arnfjörð Bjarmason
2020-12-05 7:13 ` Christian Couder
2020-12-05 8:49 ` Ævar Arnfjörð Bjarmason
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201110195439.GC1987088@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=anders@0x63.nu \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=jonathantanmy@google.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.