github actions: Fix test-one-commit when parent of head is merge commit

ryanofsky commented at 1:05 pm on October 3, 2023: contributor

Instead of figuring out the commit after the last merge and rebasing on that with a ~1 suffix, just figure out the last merge commit directly and rebase on it. This way, if HEAD happens to be a merge commit, the rebase just succeeds immediately without blank variables or errors.

Explanation of the problem from #28497 (comment):

The problem is that the PR only contains a one commit after the last merge, so the job should be skipped, but the pull_request.commits != 1 check is not smart enough to skip it because the PR is based on another PR and has merge ancestor commits. So specifically what happens is that after HEAD~ is checked out, the new HEAD is a merge commit, so the range $(git log --merges -1 --format=%H)..HEAD is equivalent to HEAD..HEAD, which is empty, so the COMMIT_AFTER_LAST_MERGE variable is empty and the rebase command fails.

Note: In the current version of this PR, the “test each commit” job is skipped, because this PR only contains a single commit. But I manually verified the code works in earlier versions of the PR that included dummy commits.

DrahtBot commented at 1:05 pm on October 3, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	MarcoFalke, RandyMcMillan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

ryanofsky force-pushed on Oct 3, 2023

maflcko added the label DrahtBot Guix build requested on Oct 3, 2023

ryanofsky force-pushed on Oct 3, 2023

DrahtBot commented at 0:27 am on October 4, 2023: contributor

Guix builds (on x86_64)

File	commit 97f756b12c8d8a9d3b621f296725dd7bf36bc8a9(master)	commit 6307893d248c73c29d449eaf4749fb6e77a09491(master and this pull)
SHA256SUMS.part	`fa4d8592bca59c4a...`	`1306ba4631a4ef15...`
*-aarch64-linux-gnu-debug.tar.gz	`5580ec85fb2ce064...`	`459a52f461d39e59...`
*-aarch64-linux-gnu.tar.gz	`78b6d232d5120a1b...`	`b957063d725ce128...`
*-arm-linux-gnueabihf-debug.tar.gz	`3d982ec7d8bf74c8...`	`e6763e27bc2760fb...`
*-arm-linux-gnueabihf.tar.gz	`d668677bff8761e9...`	`e3bb01f3bd1ae2b7...`
*-arm64-apple-darwin-unsigned.tar.gz	`27d5c72f99042fd1...`	`4fbe35f8c9c26506...`
*-arm64-apple-darwin-unsigned.zip	`0a40c1c9d69914d8...`	`df1c30ac6d0026d6...`
*-arm64-apple-darwin.tar.gz	`58a0486742a08c78...`	`9f78f7648ac61eaa...`
*-powerpc64-linux-gnu-debug.tar.gz	`e995b1125000df19...`	`5611a7db5002f33e...`
*-powerpc64-linux-gnu.tar.gz	`a299139b47c371a6...`	`16ceeb250faf19f2...`
*-powerpc64le-linux-gnu-debug.tar.gz	`9a7bceae7ca985ff...`	`4a83cd3ec117031d...`
*-powerpc64le-linux-gnu.tar.gz	`ceffc3d58f9a85c9...`	`164bc2e891b49173...`
*-riscv64-linux-gnu-debug.tar.gz	`2fbc6bb55aca9e4d...`	`c0163cb35062c278...`
*-riscv64-linux-gnu.tar.gz	`0e4f173506cd129a...`	`bb2753593fcad11d...`
*-x86_64-apple-darwin-unsigned.tar.gz	`8d369cd8f6307c6d...`	`984c45b5b11cd568...`
*-x86_64-apple-darwin-unsigned.zip	`79fd330511b22d30...`	`051a60c655956a3c...`
*-x86_64-apple-darwin.tar.gz	`4823f3b58d113f18...`	`e5588ef943838709...`
*-x86_64-linux-gnu-debug.tar.gz	`3dc47f3e2f985104...`	`53973ff976b337b0...`
*-x86_64-linux-gnu.tar.gz	`ea95470b6d8861d4...`	`7ea69e767ccb06cb...`
*.tar.gz	`c40470ff5c25bfb3...`	`2f81ff640a1831ea...`
guix_build.log	`adc6a469498c651d...`	`4b83bdc86274211c...`
guix_build.log.diff		`e062eb0dad2a14ba...`

DrahtBot removed the label DrahtBot Guix build requested on Oct 4, 2023

DrahtBot added the label CI failed on Oct 4, 2023

in .github/workflows/ci.yml:62 in 3447796bf5 outdated

50+          # HEAD in order by timestamp, excluding ancestors of the most recent
51+          # merge commit, and taking the first one. If branch contains up to
52+          # MAX_COUNT ancestor commits after the last merge commit, all of those
53+          # commits will be tested. If it contains more, only the most recent
54+          # MAX_COUNT commits will be tested.
55+          echo "TEST_BASE=$(git rev-list -n$((${{ env.MAX_COUNT }} + 1)) --reverse HEAD ^$(git rev-list -n1 --merges HEAD)^@ | head -1)" >> "$GITHUB_ENV"

maflcko commented at 3:12 pm on October 4, 2023:

0          echo "TEST_BASE=$(git rev-list -n$((${{ env.MAX_COUNT }} + 1)) HEAD ^$(git rev-list -n1 --merges HEAD)^@ | tail -1)" >> "$GITHUB_ENV"

maflcko commented at 3:12 pm on October 4, 2023:

(Can be written shorter)

maflcko commented at 3:16 pm on October 4, 2023:

Can you link to the documentation of the ^commit_id^@ syntax?

ryanofsky commented at 4:28 pm on October 4, 2023:

re: #28573 (review)

(Can be written shorter)

This is a little shorter, but I prefer head instead of tail in cases like this because the rev-list command can detect when the pipe is closed and avoid generating unnecessary output.

ryanofsky commented at 4:29 pm on October 4, 2023:

re: #28573 (review)

Can you link to the documentation of the ^commit_id^@ syntax?

Sure, added links to https://git-scm.com/docs/git-rev-parse#_other_rev_parent_shorthand_notations and https://git-scm.com/docs/git-rev-list

ryanofsky force-pushed on Oct 4, 2023

ryanofsky commented at 4:47 pm on October 4, 2023: contributor

Thanks for the review

Updated 3447796bf524d7e3eaa3287df284ef9718bc3e81 -> a32719139954e268a8c9dd02e3982731fdbc1e4d (pr/onecommit.4 -> pr/onecommit.5, compare) just adding comment

DrahtBot removed the label CI failed on Oct 5, 2023

in .github/workflows/ci.yml:47 in a327191399 outdated

45+          # Checkout HEAD~ because it would be wasteful to rerun tests on the PR
46+          # head commit that are already run by other jobs.
47           git checkout HEAD~
48-          echo "COMMIT_AFTER_LAST_MERGE=$(git log $(git log --merges -1 --format=%H)..HEAD --format=%H --max-count=${{ env.MAX_COUNT }} | tail -1)" >> "$GITHUB_ENV"
49+          # Figure out test base commit by listing up to MAX_COUNT ancestors of
50+          # HEAD in order by timestamp, excluding ancestors of the most recent

maflcko commented at 11:16 am on October 5, 2023:

0          # HEAD, excluding ancestors of the most recent

I don’t think this is relevant, because merges are excluded and the remaining commits are linear, thus topologically sorted.

ryanofsky commented at 2:42 pm on October 5, 2023:

re: #28573 (review)

Thanks, removed the part about timestamps.

I didn’t realize the list was topologically sorted, but this does seem to be the case according to documentation (“Show no parents before all of its children are shown, but otherwise show commits in the commit timestamp order.”) and some local testing. Before I thought that git commit lists had same problem github commit lists have and relied on timestamps, so the script might test more or fewer commits than supposed to, and the comment would be relevant. But glad the script is more reliable and the comment is wrong.

maflcko approved

maflcko commented at 11:18 am on October 5, 2023: member

lgtm

maflcko commented at 11:19 am on October 5, 2023: member

lgtmcr ACK a32719139954e268a8c9dd02e3982731fdbc1e4d

github actions: Fix test-one-commit when parent of head is merge commit

Instead of figuring out the commit *after* the last merge and rebasing on that
with a ~1 suffix, just figure out the last merge commit directly and rebase on
it. This way, if HEAD happens to be a merge commit, the rebase just succeeds
immediately without blank variables or errors.

From https://github.com/bitcoin/bitcoin/pull/28497#issuecomment-1743430631:

    The problem is that the PR only contains a one commit after the last merge,
    so the job _should_ be skipped, but the `pull_request.commits != 1` check
    is not smart enough to skip it because the PR is based on another PR and
    has merge ancestor commits. So specifically what happens is that after
    HEAD~ is checked out, the new HEAD is a merge commit, so the range `$(git
    log --merges -1 --format=%H)..HEAD` is equivalent to HEAD..HEAD, which is
    empty, so the `COMMIT_AFTER_LAST_MERGE` variable is empty and the rebase
    command fails.

88c8e3a0e4

ryanofsky commented at 3:02 pm on October 5, 2023: contributor

Updated a32719139954e268a8c9dd02e3982731fdbc1e4d -> 88c8e3a0e4d6bee015a348536c6e12a2c7835896 (pr/onecommit.5 -> pr/onecommit.6, compare) updating comment again

ryanofsky force-pushed on Oct 5, 2023

maflcko commented at 3:07 pm on October 5, 2023: member

lgtmrecr ACK 88c8e3a0e4d6bee015a348536c6e12a2c7835896

RandyMcMillan commented at 5:47 pm on October 5, 2023: contributor

utACK 88c8e3a

fanquake merged this on Oct 9, 2023

fanquake closed this on Oct 9, 2023

github actions: Fix test-one-commit when parent of head is merge commit #28573

Code Coverage

Reviews

Conflicts

Guix builds (on x86_64)