ci: Merge master in test-each-commit task (take 2) #32218

does the comment still make sense, the new script already explains this

Not sure what you mean. I don't think the new script explains this. The comment is unrelated to the new script. The comment explains ${{ env.TEST_BASE }}, which I am not changing in this pull.

Run tests on commits after the last merge commit and before the PR head commit

This sounds to me like a more verbose explanation for ci-test-each-commit.

Idk, it was added in 88c8e3a0e4d6bee015a348536c6e12a2c7835896. It makes sense to document this, because it isn't testing each commit, but it excludes merge commits (even if they are in the pull request). In any case, I'll leave it as-is for now, because it is unrelated.

if we need to keep this change, maybe mention that the abort is related to merging

if we need to keep this change, maybe mention that the abort is related to merging

Yes, the abort in git merge --abort is related to merging. I think the code is self-explanatory.

The CI log is also self-explanatory, because the code will print itself before being executed (xtrace).

You can see this by reading the CI logs of this pull and the one I linked to earlier.

we're setting the author info in a separate step above - we could localize it as:

          git rebase --exec "GIT_COMMITTER_NAME="CI" GIT_COMMITTER_EMAIL="ci@ci.com" git merge --no-commit origin/${GITHUB_BASE_REF} && ./.github/ci-test-each-commit-exec.sh" ${{ env.TEST_BASE }}

given that we initiate the merge here, would it make sense to do the abort here as well? And maybe we could do a reset instead which works regardless of whether we're already up-to-date or not, e.g.:

          git rebase --exec "git merge --no-commit origin/${GITHUB_BASE_REF} && ./.github/ci-test-each-commit-exec.sh && git reset --hard" ${{ env.TEST_BASE }}

<details> <summary>Details</summary>

# GIT_COMMITTER_NAME="CI" GIT_COMMITTER_EMAIL="ci@ci.com" git merge master --no-commit
Automatic merge went well; stopped before committing as requested

# git status
On branch feature
All conflicts fixed but you are still merging.
  (use "git commit" to conclude merge)

Changes to be committed:
        new file:   xyz.txt

# git reset --hard
HEAD is now at f7c4340 first 6

# git status
On branch feature
nothing to commit, working tree clean

</details>

Happy to review a follow-up doing this refactor, if people want it. However, I currently don't have the time to try if it works and if it is better.

Happy to review a follow-up doing this refactor, if people want it. However, I currently don't have the time to try if it works and if it is better.

edit: I tested this and it works locally, so it may work when I push it.

Happy to push it, if this is the last blocker for you.

Happy to push it, if this is the last blocker for you.

yes, only the things I've posted

thx, pushed

Please see the reproducer I posted to #32218 (review) - I think this would simplify the code further (untangle separate setup steps and reduce global state)

should we reset if ci-test-each-commit-exec.sh returns with an error?

setup and global state is already required, because the master branch has to be cloned and stored for this. Storing a dummy name and email is harmless in any case.

should we reset if ci-test-each-commit-exec.sh returns with an error?

Why? The CI will discard itself after failure

Yes, this would only reduce global state, not eliminate it completely.

That's why I asked, my understanding was that it doesn't matter.

What about in case of merge conflict, what is the expected outcome, should the script still be executed?

And the script only measures a single commit, not "each commit", is the naming deliberate?

I have a slight preference for the current way. Also, It is extremely tedious and time consuming to test the changes here remotely on GitHub in a separate repo on every tiny change. So I'll leave this as-is.

If there is need for this refactor, it can be done in the future.

I am not changing the name of the script here, so it is unrelated and can be changed in a separate pull, if needed.

The name of the script simply includes the task name.

What about in case of merge conflict, what is the expected outcome, should the script still be executed?

No. Why? The CI will discard itself after it failed after a merge conflict, so there is no need to delay and execute anything.