Make nicer pull request merge messages #5623

pull btcdrak wants to merge 1 commits into bitcoin:master from btcdrak:prtitle changing 1 files +4 −2

btcdrak commented at 7:50 PM on January 8, 2015: contributor
Produces merge commit messages as follows
```
Merge [#12345](/bitcoin-bitcoin/12345/): Pull request title
```
I've written this code semi blind because I'm not sure how to test it in the final run, but essentially the magic comes from taking the title from Github API.

Needs testing.
jonasschnelli commented at 8:52 PM on January 8, 2015: contributor

Nice!

utACK
btcdrak commented at 8:55 PM on January 8, 2015: contributor

Can someone restart the travis build please https://travis-ci.org/bitcoin/bitcoin/jobs/46357658 - the job timed out.
sipa commented at 9:18 PM on January 8, 2015: member

Can you use the env variables for user and repository name? This script is not solely used for Bitcoin Core.
btcdrak force-pushed on Jan 8, 2015
btcdrak commented at 9:59 PM on January 8, 2015: contributor

Done.
btcdrak commented at 10:28 PM on January 8, 2015: contributor

@sipa can we assume this script is always for github projects? Otherwise we'd need something like this https://gist.github.com/btcdrak/bd20efaec5ac3e5e6a92
laanwj commented at 10:25 AM on January 9, 2015: member

@btcdrak it's called github-merge.sh ;)

Concept ACK, haven't tested.
laanwj added the label Docs and Output on Jan 9, 2015
in contrib/devtools/github-merge.sh:None in fed7d66047 outdated
```
  82 | @@ -83,12 +83,13 @@ function cleanup() {
  83 |  
  84 |  # Create unsigned merge commit.
  85 |  (
  86 | -  echo "Merge pull request #$PULL"
  87 | +  PRTITLE=`curl -s https://api.github.com/repos/$REPO/pulls/$PULL | grep -e '  "title": ".*",'| awk -F'"' '{print $4}'`
```
fanquake commented at 1:41 AM on January 11, 2015:

This needs to have the organisation in the URL. i.e api.github.com/repos/bitcoin/bitcoin/pulls/5623 As is, it just returns "Not Found" messages.

btcdrak commented at 7:42 AM on January 11, 2015:

$REPO is supposed to be something like bitcoin/bitcoin as per this

fanquake commented at 7:49 AM on January 11, 2015:

Right, forgot we already had that defined. Although I still can't get the script to run. Only get "ERROR: Creating merge failed (already merged?)."

btcdrak commented at 8:39 AM on January 11, 2015:

Disregard last message, there is a problem.

btcdrak commented at 9:09 AM on January 11, 2015:

Fixed. TIL: bash has scoping.
btcdrak force-pushed on Jan 11, 2015
btcdrak force-pushed on Jan 11, 2015
btcdrak commented at 9:10 AM on January 11, 2015: contributor

Example tested here: https://github.com/btcdrak/bitcointest/commit/87582f4e35997e7d6beef6578438a0dab252debe from btcdrak/bitcointest#2
Make nicer pull request merge messages 1078fb0885
btcdrak force-pushed on Jan 11, 2015
in contrib/devtools/github-merge.sh:None in 1078fb0885
```
  81 | @@ -82,13 +82,15 @@ function cleanup() {
  82 |  }
  83 |  
  84 |  # Create unsigned merge commit.
  85 | +PRTITLE=`curl -s https://api.github.com/repos/$REPO/pulls/$PULL | grep -e '  "title": ".*",'| awk -F'"' '{print $4}'`
```
laanwj commented at 11:10 AM on January 16, 2015:

I think this will fail with "'s in the pull title?

btcdrak commented at 4:46 PM on January 16, 2015:

@laanwj It works for me. I posted the test merge examples above.

btcdrak commented at 4:47 PM on January 16, 2015:

Another example PR: https://github.com/btcdrak/bitcointest/pull/3 Merge commit: https://github.com/btcdrak/bitcointest/commit/fb41e01abcca2ca0e52ed64e7cd04dd256cf7ad4

laanwj commented at 3:54 PM on January 19, 2015:

But none of those titles has a " (or quote) symbol in it, or am I missing something? I'm worried about escaping here.
laanwj commented at 4:06 PM on January 19, 2015: member
E.g. I've tried merging https://github.com/btcdrak/bitcointest/pull/4 using your script, which results in a chopped-off title:
```
Merge [#4](/bitcoin-bitcoin/4/): \
```
I'm kind of distrustful of these kinds of constructions in bash script. It's very hard to make them cope with all given input without introducing bugs. As this is data coming from the network this problem is compounded, and may even result in vulnerabilities.
btcdrak commented at 4:37 PM on January 19, 2015: contributor
@laanwj Bash certainly isn't ideal here, and maybe I should use a helper python script to fetch the data, check it's valid json and can then extract the correct field. The current solution is just trying to identify the following pattern from the API call rather than parse the json.
```
"title": "pr title goes here",
```
laanwj commented at 11:00 AM on February 4, 2015: member

Another idea in this direction (but even hackier to implement in shellscript - maybe we should bite the bullet and rewrite this script in Python some day) would be to include the pull request message in the body of the pull merge message. For larger projects, developers tend to write 'higher level' context information in the pull request message, that's not in the commits themselves.

Not arguing that you should do this here, better is better.
laanwj merged this on Feb 9, 2015
laanwj closed this on Feb 9, 2015
laanwj referenced this in commit f69941620b on Feb 9, 2015
btcdrak deleted the branch on Feb 13, 2015
laanwj referenced this in commit aaba10f275 on Feb 20, 2015
laanwj commented at 9:02 AM on February 20, 2015: member
Had to revert this (aaba10f) due to various issues
- Pull request names get cut off at ", see e.g. a026a56
- Merge script no longer copes with pulls that have a milestone attached, due to a duplicate 'title' in JSON that is not handled by the ad-hoc parsing.
If anyone is still interested in doing this, I'd suggest rewriting the script in a language that can do real JSON parsing, preferably Python.
sipa commented at 8:58 PM on April 3, 2016: member

For posteriority: this was improved in #7378
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#12345 Asymmetric encryption stubs.