From: Phillip Wood <phillip.wood123@gmail•com>
To: Li Chen <me@linux•beauty>,
phillipwood <phillip.wood@dunelm•org.uk>,
git <git@vger•kernel.org>, Junio C Hamano <gitster@pobox•com>
Subject: Re: [PATCH v3 2/2] rebase: support --trailer
Date: Wed, 6 Aug 2025 14:19:57 +0100 [thread overview]
Message-ID: <499da566-66a8-4c38-a2b3-13c06092568f@gmail.com> (raw)
In-Reply-To: <e911d897-8664-40a7-b7a9-8eb9f71a8735@gmail.com>
Hi Li
I had a couple more thoughts about the tests ...
On 06/08/2025 11:28, Phillip Wood wrote:
> On 03/08/2025 16:00, Li Chen wrote:
>> +create_expect() {
>> + cat >"$1" <<-EOF
>> + $2
>> +
>> + Reviewed-by: Dev <dev@example•com>
>> + EOF
>> +}
>> +
>> +test_expect_success 'setup repo with a small history' '
>> [...]
>> + create_expect third-signed "third" &&>> +
create_expect conflict-signed "conflict"
>
> Normally we create the "expect" file in the test where it is used.
Thinking about this some more, if we want to use test_commit_message
then I think we can change create_expect to write to stdout and do
test_commit_message HEAD <<-EOF
$(create_expect first)
EOF
rather than having to create a file.
>> +
>> +test_expect_success 'reject empty --trailer argument' '
>> [...]
>> +test_expect_success 'reject trailer with missing key before separator' '
Should we also test for a missing value or are trailers without a value
allowed?
>> + git rebase -m \
>> + --trailer "Signed-off-by: Dev A <a@ex•com>" \
>> + --trailer "Signed-off-by: Dev B <b@ex•com>" HEAD~1 &&
Lets use example.com here rather than some random domain that might
actually exist.
>> +test_expect_success 'rebase -m --trailer adds trailer after conflicts' '
>> + git reset --hard third &&
>> + test_must_fail git rebase -m \
>> + --trailer "Reviewed-by: Dev <dev@example•com>" \
>> + second third &&
>> + git checkout --theirs file &&
>> + git add file &&
>> + git rebase --continue &&
This checks that the commit with conflicts has a trailer added but it
does not check that the commits picked by "git rebase --continue" do. To
check that we actually save the trailers and use them when continuing we
need to add a fourth commit on top of third and check that has a trailer
add here as well.
A couple more thoughts:
- We should check that
git -c trailer.review.key=Reviewed-by rebase \
--trailer=review="Dev <dev@example•com>"
adds a "Reviewed-by:" trailer. We can do that by changing one of the
tests in this patch rather than adding a new one. This checks that we
accept '=' as a separator as well a respecting the config.
- We should check that the todo list
pick first
fixup second
adds the trailer as expected and that
pick first
fixup -C second
also works. To do that we will need to source lib-rebase.sh at the
start of the test file and add a test that uses set_replace_editor()
which should be called in a subshell.
Do please ask if you have any questions about these suggestions
Thanks
Phillip
next prev parent reply other threads:[~2025-08-06 13:20 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-08-03 15:00 [PATCH v3 0/2] rebase: support --trailer Li Chen
2025-08-03 15:00 ` [PATCH v3 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers` Li Chen
2025-08-05 13:17 ` Phillip Wood
2025-08-07 2:45 ` Li Chen
2025-10-21 10:01 ` Li Chen
2025-08-03 15:00 ` [PATCH v3 2/2] rebase: support --trailer Li Chen
2025-08-05 15:38 ` Phillip Wood
2025-08-06 10:28 ` Phillip Wood
2025-08-06 13:19 ` Phillip Wood [this message]
2025-08-07 2:40 ` Li Chen
2025-08-28 23:35 ` Junio C Hamano
2025-09-18 8:36 ` Li Chen
2025-08-07 2:40 ` Li Chen
2025-08-03 16:35 ` [PATCH v3 0/2] " Junio C Hamano
2025-08-04 1:44 ` Li Chen
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=499da566-66a8-4c38-a2b3-13c06092568f@gmail.com \
--to=phillip.wood123@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=me@linux$(echo .)beauty \
--cc=phillip.wood@dunelm$(echo .)org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox