contrib: github-merge improvements #9670

laanwj commented at 12:14 PM on February 2, 2017: member

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:

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.

laanwj added the label Scripts and tools on Feb 2, 2017

MarcoFalke commented at 10:04 PM on February 2, 2017: member

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.

MarcoFalke commented at 10:09 PM on February 2, 2017: member

utACK 90f97df regardless.

fanquake commented at 3:28 AM on February 3, 2017: member

NACK moving this to another repo.

utACK changes.

laanwj commented at 8:00 AM on February 3, 2017: member

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.

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.

MarcoFalke commented at 11:20 AM on March 9, 2017: member

Needs rebase

in contrib/devtools/github-merge.py:131 in 90f97dfe14 outdated

  68 | @@ -69,6 +69,10 @@ def ask_prompt(text):
  69 |      print("",file=stderr)
  70 |      return reply
  71 |  
  72 | +def print_merge_details(pull, title, branch, base_branch, head_branch):
  73 | +    print('%s#%s%s %s %sinto %s%s' % (ATTR_RESET+ATTR_PR,pull,ATTR_RESET,title,ATTR_RESET+ATTR_PR,branch,ATTR_RESET))

JeremyRubin commented at 8:12 PM on April 5, 2017:

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()))

laanwj commented at 2:13 PM on April 20, 2017:

Agree, but I didn't change this code in this pull only moved it.

in contrib/devtools/github-merge.py:323 in 90f97dfe14 outdated

 235 | @@ -242,9 +236,13 @@ def main():
 236 |          subprocess.call([GIT,'branch','-q','-D',local_merge_branch],stderr=devnull)
 237 |  
 238 |      # Push the result.
 239 | -    reply = ask_prompt("Type 'push' to push the result to %s, branch %s." % (host_repo,branch))
 240 | -    if reply.lower() == 'push':
 241 | -        subprocess.check_call([GIT,'push',host_repo,'refs/heads/'+branch])
 242 | +    while True:
 243 | +        reply = ask_prompt("Type 'push' to push the result to %s, branch %s, or 'x' to exit without pushing." % (host_repo,branch)).lower()

JeremyRubin commented at 8:16 PM on April 5, 2017:

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 :)

JeremyRubin commented at 8:19 PM on April 5, 2017:

in a similar vein, may be good to also add an additional confirm if the branch is prefixed with a +, ie, a force push.

laanwj commented at 2:16 PM on April 20, 2017:

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.

JeremyRubin approved

JeremyRubin commented at 8:21 PM on April 5, 2017: contributor

utack 90f97df.

A couple suggestions on further improvements, but those are addressable in another PR if desired.

contrib: github-merge improvements

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.

b508424104

laanwj force-pushed on Apr 21, 2017

laanwj merged this on Apr 26, 2017

laanwj closed this on Apr 26, 2017

laanwj referenced this in commit bd9ec0ef1e on Apr 26, 2017

laanwj referenced this in commit d9376d76ce on Sep 5, 2017

PastaPastaPasta referenced this in commit 799adcd55b on May 10, 2019

PastaPastaPasta referenced this in commit a94107f3a3 on May 15, 2019

PastaPastaPasta referenced this in commit 317b797e2a on May 20, 2019

PastaPastaPasta referenced this in commit 2dd3091474 on May 21, 2019

barrystyle referenced this in commit 146ba2556e on Jan 22, 2020

MarcoFalke locked this on Sep 8, 2021