contrib: github-merge improvements #9670
Conversation
|
This is not a dev script, so it might be better located in the maintainer repo. OTOH, this is probably the most critical maintainer script, so keeping it here to have more eyes watching it is a good thing. |
|
utACK 90f97df regardless. |
|
NACK moving this to another repo. utACK changes. |
Yes I've thought about that. We could have the main development copy in the maintainer-tools repo, and sync it to Bitcoin Core once in a while. I use the tool with a few different repositories so I have it installed system-wide and hardly ever use the one in the Bitcoin Core tree. |
|
Needs rebase |
JeremyRubin
left a comment
There was a problem hiding this comment.
utack 90f97df.
A couple suggestions on further improvements, but those are addressable in another PR if desired.
contrib/devtools/github-merge.py
Outdated
There was a problem hiding this comment.
nit: maybe a bit nicer with some python3 isms
print('{color_pr}#{pr}{nocolor} {title}{color_pr} into {branch}{nocolor}'.format(color_pr = ATTR_RESET+ATTR_PR, nocolor=ATTR_RESET, **locals()))There was a problem hiding this comment.
Agree, but I didn't change this code in this pull only moved it.
contrib/devtools/github-merge.py
Outdated
There was a problem hiding this comment.
May be nice to change this to be "type the remote and branch to confirm" so that you are forced to actually read where you're pushing it :)
There was a problem hiding this comment.
in a similar vein, may be good to also add an additional confirm if the branch is prefixed with a +, ie, a force push.
There was a problem hiding this comment.
May be nice to change this to be "type the remote and branch to confirm" so that you are forced to actually read where you're pushing it :)
Not sure this will help much. I don't think I've ever pushed something to the wrong place with this script after I made it take the target branch from the pull metadata on github.
in a similar vein, may be good to also add an additional confirm if the branch is prefixed with a +, ie, a force push.
We don't support force-pushing from this script, and force pushes to master and all the version branches are disabled on github.
Some minor github-merge improvements I've made over time: User interface: - Print merge details again before signing off, to refresh your memory - usually I'll have done lots of different things in the shell so this will have scrolled out a long time ago. - Require a valid answer on the prompts. One of the requested answers must be typed, if not, the prompt will re-ask. This prevents accidentally rejecting. Efficiency: - Condense "accept merge" and "sign off" prompts. There's no reason to have this as two separate prompts, both are just opportunities to skip out on the merge, no action is performed in between. Merging: - Strip spaces from github title. This avoids redundant spaces surrounding it from getting into the commit message.
90f97df to
b508424
Compare
b508424 contrib: github-merge improvements (Wladimir J. van der Laan) Tree-SHA512: 56a34e887716bf6bfcd1b6520f6b9a1bb742e1ad17e75618caf982af71fceb75d50caec1bf4279cb9a2f7a74319f1bcec4c824682841bd6e994acc0991616451
b508424 contrib: github-merge improvements (Wladimir J. van der Laan) Tree-SHA512: 56a34e887716bf6bfcd1b6520f6b9a1bb742e1ad17e75618caf982af71fceb75d50caec1bf4279cb9a2f7a74319f1bcec4c824682841bd6e994acc0991616451
Some minor github-merge improvements I've made over time:
User interface:
Print merge details again before signing off, to refresh your memory - usually at least I'll have done lots of different things in the shell so this will have scrolled out a long time ago.
Require a valid answer on the prompts. One of the requested answers must be typed, if not, the prompt will re-ask. This prevents accidentally rejecting.
Efficiency:
Merging: