devtools: replace github-merge with python version #7378

pull laanwj wants to merge 1 commits into bitcoin:master from laanwj:2016_01_python_github_merge changing 4 files +227 −189

laanwj commented at 9:15 AM on January 19, 2016: member
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:
```
Merge pull request [#12345](/bitcoin-bitcoin/12345/): Expose transaction temperature over RPC
```
[skip ci]
laanwj added the label Dev Scripts on Jan 19, 2016
laanwj force-pushed on Jan 19, 2016
jonasschnelli commented at 2:19 PM on January 19, 2016: contributor
Nice! Tested ACK (on a different repo: https://github.com/digitalbitbox/dbb-app/commit/a592638e733a124426145cb0601907561ecd9975).

Thought for future additions:
- show the head SHA256 hash from the PR (possible short compare against the one you have ACKed)
- short command to diff the PR (temp bash alias instead of typing git diff HEAD~1?)
MarcoFalke commented at 3:31 PM on January 19, 2016: member

utACK b86484c
in contrib/devtools/github-merge.py:None in b86484c61e outdated
```
  41 | +    import urllib2,json
  42 | +    try:
  43 | +        req = urllib2.Request("https://api.github.com/repos/"+repo+"/pulls/"+pull)
  44 | +        result = urllib2.urlopen(req)
  45 | +        result = json.load(result)
  46 | +        return result['title']
```
MarcoFalke commented at 4:00 PM on January 19, 2016:

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: "]

laanwj commented at 9:51 AM on January 20, 2016:

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.
laanwj commented at 9:58 AM on January 20, 2016:

Other, less lossy ways to shorten the text may be:

leave out the "pull request" completely, as in "Merge #1234: bla"

abbreviate it as PR ...
MarcoFalke commented at 10:02 AM on January 20, 2016:

ACK on shorten the text.

jonasschnelli commented at 10:04 AM on January 20, 2016:

ACK on keeping the full title but removing "pull request".
laanwj force-pushed on Jan 20, 2016
laanwj commented at 11:28 AM on January 20, 2016: member

show the head SHA256 hash from the PR (possible short compare against the one you have ACKed)

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 can be done in a later pull, not going to do so now)

devtools: replace github-merge with python version

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 #12345: Expose transaction temperature over RPC

da6d18b6c7

laanwj force-pushed on Jan 20, 2016

in contrib/devtools/github-merge.py:None in da6d18b6c7

 181 | +            print("Run 'git diff HEAD~' to show the changes being merged.",file=stderr)
 182 | +            print("Type 'exit' when done.",file=stderr)
 183 | +            if os.path.isfile('/etc/debian_version'): # Show pull number on Debian default prompt
 184 | +                os.putenv('debian_chroot',pull)
 185 | +            subprocess.call([BASH,'-i'])
 186 | +            reply = ask_prompt("Type 'm' to accept the merge.")

laanwj commented at 12:30 PM on January 20, 2016:

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

sipa commented at 8:59 PM on April 3, 2016:

They were separate at a time when the signing was not mandatory :)

MarcoFalke commented at 12:39 PM on January 20, 2016: member

Tested ACK da6d18b
laanwj merged this on Jan 20, 2016
laanwj closed this on Jan 20, 2016
laanwj referenced this in commit e6f97efbca on Jan 20, 2016
codablock referenced this in commit 54d984d012 on Sep 16, 2017
codablock referenced this in commit 59f9cbbfae on Sep 19, 2017
codablock referenced this in commit 79e5af135f on Dec 9, 2017
codablock referenced this in commit 3be77849d7 on Dec 9, 2017
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#12345 Asymmetric encryption stubs.