ci: improve "test each commit" job to handle more complex scenarios

josibake commented at 10:22 am on July 16, 2025: member

In #28122, I start by updating the libsecp subtree with git subtree pull --prefix src/secp256k1 <myfork> <mybranch> --squash. When rebasing on this commit, I often need to use something like:

0git rebase --rebase-merges --strategy subtree -X theirs

.. which Just Works. However, now the CI fails when running:

0git rebase --exec 'git merge --no-commit ...'

.. as seen in https://github.com/bitcoin/bitcoin/actions/runs/16315746903/job/46081155541?pr=28122.

Opening this issue to see if anyone has any ideas on this can be improved. I took a look at the job and didn’t seen an obvious fix, but mostly because the logic for determining the list of commits seems very specific and I wasn’t sure why its done that way.

josibake added the label Feature on Jul 16, 2025

willcl-ark added the label CI failed on Jul 16, 2025

maflcko commented at 10:52 am on July 16, 2025: member

That’s interesting. For other subtree merges, it does seem to work.

E.g. https://github.com/bitcoin/bitcoin/actions/runs/16022448902/job/45202410762?pr=32856#step:7:1, where it correctly skips over all commits, because after the merge commit, there is only one commit.
E.g. https://github.com/bitcoin/bitcoin/actions/runs/16008106824/job/45159350306?pr=32345#step:7:19, where it correctly starts at commit f8fd3959d51f6f80b428828c4fd965e06a8fa19e (after the merge) and stops at commit 008f4ae01ebb7b2cd8bc95b5ea5afd65905db749 (https://github.com/bitcoin/bitcoin/actions/runs/16008106824/job/45159350306?pr=32345#step:7:6009, the second-to-last commit)

Is your subtree merge commit correct? Does it also happen when you recreate the subtree merge commit from scratch?

josibake commented at 12:43 pm on July 16, 2025: member

For other subtree merges, it does seem to work

AFAICT, this only happens when files in the subtree have been changed, and files in the main repo with the same name have also been changed, i.e., CMakePresets.json from the failure I linked. This file is in the bitcoin core root and the secp256k1 root.

Is your subtree merge commit correct

I hope so! I’m using the same command we us to update the subtree in the project. I’m not sure what you mean by “create the subtree merge commit from scratch.”

maflcko commented at 1:12 pm on July 16, 2025: member

I can’t reproduce this locally, when copy-pasting the command (https://github.com/bitcoin/bitcoin/actions/runs/16315746903/job/46081155541?pr=28122#step:7:3), even when installing the git version from GHA (https://github.com/actions/runner-images/commit/723cd0240d0c380f28bfd66e4ec6556dc3b536fd#diff-cc12e5c5d005932feaacf280af949d83e2cb6020dff8203d9ead2e00c30e8b7fR77):

 0# git checkout a4c84b90c0f2be478e84c39785cbda2a8b2512db && git rebase --exec "git merge --no-commit origin/master && echo ./.github/ci-test-each-commit-exec.py && git reset --hard" c42c7351a6316805fdfb20163762c1ab8293dd0d
 1HEAD is now at a4c84b90c0 tests: add BIP352 test vectors as unit tests
 2Executing: git merge --no-commit origin/master && echo ./.github/ci-test-each-commit-exec.py && git reset --hard
 3Automatic merge went well; stopped before committing as requested
 4./.github/ci-test-each-commit-exec.py
 5HEAD is now at 98c84190dc crypto: add read-only method to KeyPair
 6Executing: git merge --no-commit origin/master && echo ./.github/ci-test-each-commit-exec.py && git reset --hard
 7Automatic merge went well; stopped before committing as requested
 8./.github/ci-test-each-commit-exec.py
 9HEAD is now at 9219e4f2a6 Add "sp" HRP
10Executing: git merge --no-commit origin/master && echo ./.github/ci-test-each-commit-exec.py && git reset --hard
11Automatic merge went well; stopped before committing as requested
12./.github/ci-test-each-commit-exec.py
13HEAD is now at 41b4e225b1 Add V0SilentPaymentDestination address type
14Executing: git merge --no-commit origin/master && echo ./.github/ci-test-each-commit-exec.py && git reset --hard
15Automatic merge went well; stopped before committing as requested
16./.github/ci-test-each-commit-exec.py
17HEAD is now at bacb0f26fe common: add bip352.{h,cpp} secp256k1 module
18Executing: git merge --no-commit origin/master && echo ./.github/ci-test-each-commit-exec.py && git reset --hard
19Automatic merge went well; stopped before committing as requested
20./.github/ci-test-each-commit-exec.py
21HEAD is now at 4efc2a5656 wallet: disable sending to silent payment address
22Executing: git merge --no-commit origin/master && echo ./.github/ci-test-each-commit-exec.py && git reset --hard
23Automatic merge went well; stopped before committing as requested
24./.github/ci-test-each-commit-exec.py
25HEAD is now at a4c84b90c0 tests: add BIP352 test vectors as unit tests
26Successfully rebased and updated detached HEAD.
27# git --version 
28git version 2.50.1

maflcko commented at 1:14 pm on July 16, 2025: member

I’m not sure what you mean by “create the subtree merge commit from scratch.”

Sorry, I misread that you were rebasing the subtree merge commit, but you were rebasing on the subtree merge commit, which should be correct.

maflcko removed the label Feature on Jul 16, 2025

josibake commented at 4:44 pm on July 16, 2025: member

I can’t reproduce this locally

Let me know if I can help with reproducing this. I haven’t tried to reproduce the CI failure locally, but I can recreate a similar failure by first creating a branch where I pull in the latest secp subtree, and then try to rebase my branch on top of that. This will fail with a merge conflict message, unless I use git rebase --rebase-merges --strategy subtree.

I’ll also try digging into the CI task a bit more, later this week.

maflcko commented at 4:50 pm on July 16, 2025: member

I think when you rebase on top of a subtree merge, you’ll want to do git rebase --interactive c42c7351a6316805fdfb20163762c1ab8293dd0d and then delete the old subtree commits:

0# delete ! 497f1536ef # Squashed 'src/secp256k1/' changes from 4187a46649..c0db6509bd
1# delete ! bb0dc6d75c # Squashed 'src/secp256k1/' changes from c0db6509bd..6264c3d093
2pick da3494063f # crypto: add read-only method to KeyPair
3pick 687eb1c44f # Add "sp" HRP
4pick ebc062ba2a # Add V0SilentPaymentDestination address type
5pick 8d07a4e097 # common: add bip352.{h,cpp} secp256k1 module
6pick b1848aa0c9 # wallet: disable sending to silent payment address
7pick 8c073407aa # tests: add BIP352 test vectors as unit tests
8
9# Rebase c42c7351a6..8c073407aa onto c42c7351a6 (8 commands)

This passes fine for me locally as well.

josibake commented at 12:57 pm on July 17, 2025: member

you’ll want to do git rebase --interactive c42c7351a6316805fdfb20163762c1ab8293dd0d and then delete the old subtree commits

Apologies if I’m being dense, but what you’re suggesting doesn’t make sense to me. What I’m doing currently:

0# starting from master
1git checkout -b refresh-secp256k1
2
3# git subtree pull --prefix src/secp256k1/ <myfork> <mybranch> --squash
4git subtree pull --prefix src/secp256k1/ josie-secp256k1 bip352-silentpayments-module-2025 --squash
5
6# rebase my bitcoin core branch on the latest secp256k1 changes
7git rebase --onto refresh-secp256k1 702eb96c46904288f72f02f968339c27c6524195^ implement-bip352

This results in something like:

 0...
 1commit 702eb96c46904288f72f02f968339c27c6524195
 2Author: josibake <josibake@protonmail.com>
 3Date:   Fri Apr 4 12:37:55 2025 +0200
 4
 5    crypto: add read-only method to KeyPair
 6
 7    Add a method for passing a KeyPair object to secp256k1 functions expecting a secp256k1_keypair.
 8    This allows for passing a KeyPair directly to a secp256k1 function without needing to create a
 9    temporary secp256k1_keypair object.
10
11commit 14224fd2f3b31ed869397989d88306df2a3cfe6f (origin/refresh-secp256k1)
12Merge: 9f713b83dc 46f5c2f1f3
13Author: josibake <josibake@protonmail.com>
14Date:   Thu Jul 17 12:25:41 2025 +0100
15
16    Merge commit '46f5c2f1f382922c19a070b78803fbd29cedd62b' into refresh-secp256k1
17
18commit 46f5c2f1f382922c19a070b78803fbd29cedd62b
19Author: josibake <josibake@protonmail.com>
20Date:   Thu Jul 17 12:25:41 2025 +0100
21
22    Squashed 'src/secp256k1/' changes from 4187a46649..9e85256bbe
23...

Calling git rebase --interactive 14224fd2f3b31ed869397989d88306df2a3cfe6f just gives me the commits from my branch, not the squash commits.

Annoyingly, I rebased the PR on master using this exact process and pushed the branch and now on the PR the test each commit task is no longer failing, which is perhaps why whatever you were doing was working? But again, I’m not sure what you’re doing.

maflcko commented at 1:23 pm on July 17, 2025: member

git rebase –onto refresh-secp256k1 702eb96c46904288f72f02f968339c27c6524195^ implement-bip352

Yeah, that looks correct. It should also be identical to my version. The only difference is that yours works also non-iteractively. That is, the two commands give the same result:

0git rebase --interactive --onto=14224fd2f3b31ed869397989d88306df2a3cfe6f da3494063f^ 8c073407aa  # no interactive changes needed

0git checkout 8c073407aa && git rebase --interactive 14224fd2f3b31ed869397989d88306df2a3cfe6f  # and then interactively dropping the subtree commits.

josibake commented at 4:29 pm on July 17, 2025: member

Following up, this was my mistake! AFAICT, this happened because I had two squashes into the refresh-secp256k1 branch. I attempted to reproduce this on my fork and couldn’t get a failure, and the only difference I can see is that the original failure had two squashes followed by a merge commit.

Thanks for taking a look @maflcko and for the help in reproducing.

josibake closed this on Jul 17, 2025

ci: improve “test each commit” job to handle more complex scenarios #32991