Skip to content

sync-upstream: simplify#360

Draft
real-or-random wants to merge 1 commit into
BlockstreamResearch:masterfrom
real-or-random:202604-clean-sync-script
Draft

sync-upstream: simplify#360
real-or-random wants to merge 1 commit into
BlockstreamResearch:masterfrom
real-or-random:202604-clean-sync-script

Conversation

@real-or-random
Copy link
Copy Markdown
Member

This is a half-rewrite of the sync-upstream script because I still wasn't happy with it. Looking for early feedback on this.

Advantages:

  • The script does not hardcode any repo URLs, branches, etc.
  • Nicer separation of responsibilities between the script and the GHA workflow. This brings back the usefulness of the script when used locally outside GHA.
  • Much simpler code, and we could simplify also the GHA workflow.

Still to do:

  • Rewrite the "Tips" section in the generated PR description, e.g., add how to resolve merge conflicts.
  • Change the GHA workflow accordingly.

@mllwchrry
Copy link
Copy Markdown
Collaborator

Nice cleanup! A few issues and suggestions:

  1. The new pattern replaces the fixed bitcoin-core/secp256k1 with .*, but .* will consume up to the last # in the line, capturing the wrong number on commit messages that have multiple # occurrences.
    For example, in the lib, there exists a commit message:
    Merge bitcoin-core/secp256k1#1819: tests: Improve tests (Issue #1812)
    New pattern captures 1812 instead of 1819.
    I suggest using this: sed s/'Merge [^#]*#\([0-9]*\).*'/'\1'/. This way [^#]* stops at the first #. And since * already handles the zero-characters case (e.g., Merge #123), \? is not needed, so omitting it will make sed supported on macOS as well.

  2. The generated script sets BASE, but the gh pr create command references $BASE_BRANCH, which is never defined and expands to an empty string. The fix is to change "\$BASE_BRANCH" to "\$BASE" in the gh pr create line in sync-upstream.sh.

  3. The branch is created before $BASE_BRANCH is validated. $UPSTREAM_REF is implicitly validated by the first git rev-parse call, so the script exits if it's invalid. But $BASE_BRANCH isn't used until git merge-base, which runs after the branch is already created. A typo in $BASE_BRANCH can lead to an orphaned branch that must be manually deleted before retrying. Although in the case of automatic sync, this may not be that important, the fix is quick and easy, and still worth applying. For this, RANGESTART_COMMIT=$(git merge-base ...) has to be moved above the branch creation.

  4. Inside the loop, git log -1 is called twice per commit (once for PRNUM and once for the body). Since they are identical, we can store the result once and derive both title and body from it.

@real-or-random
Copy link
Copy Markdown
Member Author

All of this makes sense, thanks! Though at this stage, I was more looking for conceptual feedback. Is this the right way and does it give us the claimed advantages? Any disadvantages you see?

@mllwchrry
Copy link
Copy Markdown
Collaborator

Yes, it definitely gives us the advantages you mentioned, and I like the concept. That's why I went ahead and looked at the code more closely as well. As for disadvantages, I don't see any right now. I'm only wondering about placing gh-pr-create.sh in CWD instead of contrib/. What's the reason for that change?

@DarkWindman
Copy link
Copy Markdown
Contributor

Generally, I agree that this concept is more useful! Local sync support is always a plus, just in case we need to do sync earlier than next month.

Also, from what I understand, we are moving the branch creation logic back to the script, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants