Hi Li
On 03/08/2025 16:00, Li Chen wrote:
From: Li Chen <chenl311@xxxxxxxxxxxxxxx>
Picking up from where I left off yesterday ...
diff --git a/t/meson.build b/t/meson.build
index 09f3068f98..3c58f562da 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -373,6 +373,7 @@ integration_tests = [
't3436-rebase-more-options.sh',
't3437-rebase-fixup-options.sh',
't3438-rebase-broken-files.sh',
+ 't3440-rebase-trailer.sh',
The alignment looks off here
't3500-cherry.sh',
't3501-revert-cherry-pick.sh',
't3502-cherry-pick-merge.sh',
diff --git a/t/t3440-rebase-trailer.sh b/t/t3440-rebase-trailer.sh
new file mode 100755
index 0000000000..a580449628
--- /dev/null
+++ b/t/t3440-rebase-trailer.sh
@@ -0,0 +1,95 @@
+#!/bin/sh
+#
+
+test_description='git rebase --trailer integration tests
+We verify that --trailer on the merge/interactive/exec/root backends,
There are only two backends "apply" and "merge", the other things you
have listed are just command line options. We don't actually test --exec
or --interactive with --trailer in this file so we should reword that
comment. There is no need to add tests for those.
+and that it is rejected early when the apply backend is requested.'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh # test_commit_message, helpers
+
+create_expect() {
+ cat >"$1" <<-EOF
+ $2
+
+ Reviewed-by: Dev <dev@xxxxxxxxxxx>
+ EOF
+}
+
+test_expect_success 'setup repo with a small history' '
+ git commit --allow-empty -m "Initial empty commit" &&
+ test_commit first file a &&
+ test_commit second file &&
+ git checkout -b conflict-branch first &&
+ test_commit file-2 file-2 &&
+ test_commit conflict file &&
+ test_commit third file &&
+ ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" &&
+ create_expect initial-signed "Initial empty commit" &&
+ create_expect first-signed "first" &&
+ create_expect second-signed "second" &&
+ create_expect file2-signed "file-2" &&
+ create_expect third-signed "third" &&
+ create_expect conflict-signed "conflict"
Normally we create the "expect" file in the test where it is used.
+'
+
+test_expect_success 'apply backend is rejected with --trailer' '
We know HEAD is at third so we don't need this
+ git reset --hard third &&
+ head_before=$(git rev-parse HEAD) &&
+ test_expect_code 128 \
The indentation is off here
+ git rebase --apply --trailer "Reviewed-by: Dev <dev@xxxxxxxxxxx>" \
+ HEAD^ 2>err &&
+ test_grep "requires the merge backend" err &&
I think it is worth checking that the error message includes --trailer
as well to make sure that is the option that is triggering the error.
+ test_cmp_rev HEAD $head_before
+'
+
+test_expect_success 'reject empty --trailer argument' '
+ git reset --hard third &&
The exact commit is not important here
+ test_expect_code 128 git rebase -m --trailer "" HEAD^ 2>err &&
+ test_grep "empty --trailer" err
+'
+
+test_expect_success 'reject trailer with missing key before separator' '
+ git reset --hard third &&
Same here - we're only checking the error message so the exact value of
HEAD is not important.
+ test_expect_code 128 git rebase -m --trailer ": no-key" HEAD^ 2>err &&
+ test_grep "missing key before separator" err
+'
+
+test_expect_success 'CLI trailer duplicates allowed; replace policy keeps last' '
+ git reset --hard third &&
+ git -c trailer.Bug.ifexists=replace -c trailer.Bug.ifmissing=add rebase -m --trailer "Bug: 123" --trailer "Bug: 456" HEAD~1 &&
+ git cat-file commit HEAD | grep "^Bug: 456" &&
+ git cat-file commit HEAD | grep -v "^Bug: 123"
Piping git into another command is discouraged as git can potentially
fail without us noticing. Here I think it would be better to use
test_commit_message() to check the whole message.
+'
+
+test_expect_success 'multiple Signed-off-by trailers all preserved' '
+ git reset --hard third &&
We can avoid this by passing the commit we want to checkout to rebase as
you do in the test below.
+ git rebase -m \
+ --trailer "Signed-off-by: Dev A <a@xxxxxx>" \
+ --trailer "Signed-off-by: Dev B <b@xxxxxx>" HEAD~1 &&
+ git cat-file commit HEAD | grep -c "^Signed-off-by:" >count &&
+ test "$(cat count)" = 2 # two new commits
Let check the actual message here as well so if it fails we can see what
the message is.
+'
+
+test_expect_success 'rebase -m --trailer adds trailer after conflicts' '
+ git reset --hard third &&
We don't need this as the rebase command checks out third for us, saving
a process which is always nice.
+ test_must_fail git rebase -m \
+ --trailer "Reviewed-by: Dev <dev@xxxxxxxxxxx>" \
+ second third &&
+ git checkout --theirs file &&
+ git add file &&
+ git rebase --continue &&
The indentation is off here
+ test_commit_message HEAD~2 file2-signed
It's good to see this uses test_commit_message but why are we checking
HEAD~2 rather than HEAD?> +'
+
+test_expect_success 'rebase --root --trailer updates every commit' '
+ git checkout first &&
+ git rebase --root --keep-empty \
--keep-empty is the default these days so I think we can drop that.
+ --trailer "Reviewed-by: Dev <dev@xxxxxxxxxxx>" &&
+ test_commit_message HEAD first-signed &&
+ test_commit_message HEAD^ initial-signed
Looks good.
While there are some small issues to fix the tests look sensible and I
think you have good coverage of the new option.
Thanks
Phillip
