-
Notifications
You must be signed in to change notification settings - Fork 38.7k
devtools: replace github-merge with python version #7378
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
340310a to
b86484c
Compare
|
Nice! Thought for future additions:
|
|
utACK b86484c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could make sense to "sanitize" the title such that it fits in the git(hub) subject line.
That is, return title[0:min(80, len(title)) - len("Merge pull request #xxxx: "]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know - I don't like it cut off.
At least github tends to handle long title lines already by adding ... and continuing on the next line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other, less lossy ways to shorten the text may be:
- leave out the "pull request" completely, as in "Merge During initial sync, chain download pauses if peer goes away #1234: bla"
- abbreviate it as PR
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK on shorten the text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK on keeping the full title but removing "pull request".
b86484c to
04bdb61
Compare
The simplest idea there would be to print the commit message for the merge to the console. It has all context information: the pull #, the pull title, and the SHA (not 256) hashes for the commits in topological order. |
This is meant to be a direct translation of the bash script,
with the difference that it retrieves the PR title from github,
thus creating pull messages like:
Merge bitcoin#12345: Expose transaction temperature over RPC
04bdb61 to
da6d18b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW: is anyone opposed to combining 'accept' and 'sign off' into one step? I may be overlooking something, but I've never completely understood why these are separate, why you would accept a change but then not sign it (which is mandatory).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They were separate at a time when the signing was not mandatory :)
|
Tested ACK da6d18b |
da6d18b devtools: replace github-merge with python version (Wladimir J. van der Laan)
da6d18b devtools: replace github-merge with python version (Wladimir J. van der Laan)
This is meant to be a direct translation of the bash script.
The only major difference is that it retrieves the PR title from github, thus creating pull messages like:
[skip ci]