Skip to content

[qa] univalue: Bump subtree #11952

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 2 commits into from
Dec 20, 2017
Merged

[qa] univalue: Bump subtree #11952

laanwj merged 2 commits into from
Dec 20, 2017

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Dec 19, 2017

Pulls in the test changes to the univalue subtree.

Beside looking at the code, reviewers should refer to https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/README.md#git-subtree-checksh on how to verify the subtree pull.

@jnewbery
Copy link
Contributor

jnewbery commented Dec 19, 2017

Tested ACK fa58ff9c40632b00961abf9b09acee1bda1c5d3c. I've manually checked the commits, run the tests, and verified git-subtree-check:

./contrib/devtools/git-subtree-check.sh src/univalue
src/univalue in HEAD currently refers to tree 46076c3c05f8fb95a7bdad3f5c93d01f14ecbf9d
src/univalue in HEAD was last updated in commit 351fadd68a49e8955b1b0185da1eb90fa53a4cc6 (tree 46076c3c05f8fb95a7bdad3f5c93d01f14ecbf9d)
src/univalue in HEAD was last updated to upstream commit ec849d9a2889e89ba484dacbc7d2ce258f090545 (tree 46076c3c05f8fb95a7bdad3f5c93d01f14ecbf9d)
GOOD

ryanofsky
ryanofsky reviewed Dec 19, 2017
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NACK (I think) to fa58ff9c40632b00961abf9b09acee1bda1c5d3c, unless I am misunderstanding something. This seems to be using https://github.com/bitcoin-core/univalue/commits/ec849d9a2889e89ba484dacbc7d2ce258f090545 as the upstream commit, but that commit isn't a master commit on either https://github.com/bitcoin-core/univalue/commits/master or https://github.com/jgarzik/univalue/commits/master. I think if you switch to bitcoin-core/univalue-subtree@07947ff instead this should be good.

@maflcko
Copy link
Member Author

maflcko commented Dec 19, 2017

@ryanofsky It is a commit on the master branch for me. C.f.

$ git log univalue/master -3 --oneline && git log ec849d9a2889e89ba484dacbc7d2ce258f090545~ --oneline -1
07947ff2da (univalue/master) Merge #9: [tests] Fix BOOST_CHECK_THROW macro
ec849d9a28 [tests] Fix BOOST_CHECK_THROW macro
31bc9f5a49 Merge #8: Remove unused Homebrew workaround
31bc9f5a49 Merge #8: Remove unused Homebrew workaround

So the 07947ff commit is an "empty" merge commit (artefact of the merge script to create commits even when fast-forward could have been used for the purpose of putting meta-information in there.) As we strip the meta information of commits for the purpose of subtree pulls, I fail to see the need to include it and redo the subtree pull.

@ryanofsky
Copy link
Contributor

ryanofsky commented Dec 19, 2017 •
edited
Loading

That only shows ec849d9a28 is contained in the master branch. I wouldn't call it being on the on the master branch because the master branch has never pointed to that commit and never will. Would it be difficult to update this PR so it merges in the right commit (07947ff2da instead of ec849d9a28)?

It is true that there that there is no tree diff between 07947ff2da and ec849d9a28, but there are a few reasons why I think it is appropriate to update to 07947ff2da instead of ec849d9a28:

  • 07947ff2da unlike ec849d9a28 references the PR number containing the BOOST_CHECK_THROW change. If you look at ec849d9a28 by itself you have no clue where that change is coming from, where to find the code review comments etc. If you wanted to run a changelog script that would include PR numbers, the PR would never be mentioned.
  • 07947ff2da unlike ec849d9a28 is GPG signed.
  • 07947ff2da unlike ec849d9a28 includes Tree-SHA512 hash
  • 07947ff2da unlike ec849d9a28 has travis runs associated with it
  • 07947ff2da unlike ec849d9a28 doesn't require manual diffing in order to verify that it matches the current master branch of the https://github.com/bitcoin-core/univalue repository.

MarcoFalke added 2 commits December 19, 2017 16:44
Squashed 'src/univalue/' changes from fe805ea74f..07947ff2da
88411e9 
07947ff2da Merge #9: [tests] Fix BOOST_CHECK_THROW macro
ec849d9a28 [tests] Fix BOOST_CHECK_THROW macro
31bc9f5a49 Merge bitcoin#8: Remove unused Homebrew workaround
fa042093d1 Remove HomeBrew workaround
a523e08ae4 Merge #7: Declare single-argument (non-converting) constructors "explicit"
a9e53b38ba Merge #4: Pull upstream
16a1f7f6e9 Merge bitcoin#3: Pull upstream
daf1285af6 Merge pull request #2 from jgarzik/master
f32df99e96 Merge branch '2016_04_unicode' into bitcoin
280b191cb1 Merge remote-tracking branch 'jgarzik/master' into bitcoin
2740c4f712 Merge branch '2015_11_escape_plan' into bitcoin

git-subtree-dir: src/univalue
git-subtree-split: 07947ff2da9ef02a9dfa13346bc5545708e3ebe7
univalue: Bump subtree
fad349c
@maflcko maflcko force-pushed the Mf1712-univalueBump branch from fa58ff9 to fad349c Compare December 19, 2017 21:45
@maflcko
Copy link
Member Author

maflcko commented Dec 19, 2017

Changed commit. Tree shouldn't change, obviously.

ryanofsky
ryanofsky reviewed Dec 19, 2017
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! utACK fad349c, saw "GOOD" from git-subtree-check.sh

@meshcollider
Copy link
Contributor

meshcollider commented Dec 20, 2017

utACK fad349c

@laanwj
Copy link
Member

laanwj commented Dec 20, 2017

utACK fad349c

src/univalue in HEAD currently refers to tree 46076c3c05f8fb95a7bdad3f5c93d01f14ecbf9d
src/univalue in HEAD was last updated in commit 88411e98e50c92f8990733eee0bc353ac8f21a35 (tree 46076c3c05f8fb95a7bdad3f5c93d01f14ecbf9d)
src/univalue in HEAD was last updated to upstream commit 07947ff2da9ef02a9dfa13346bc5545708e3ebe7 (tree 46076c3c05f8fb95a7bdad3f5c93d01f14ecbf9d)
GOOD

@laanwj laanwj merged commit fad349c into bitcoin:master Dec 20, 2017
laanwj added a commit that referenced this pull request Dec 20, 2017
@laanwj
Merge #11952: [qa] univalue: Bump subtree
9ab9963 
88411e9 Squashed 'src/univalue/' changes from fe805ea74f..07947ff2da (MarcoFalke)

Pull request description:

  Pulls in the test changes to the univalue subtree.

  Beside looking at the code, reviewers should refer to https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/README.md#git-subtree-checksh on how to verify the subtree pull.

Tree-SHA512: 09493625a573dca1140570326ee90c1bb84e4893e1dab2cdd51bc23ae1fba1e33c43ed771ca9e112ac71b0242e8a8d058071334562c738d502587eadd5a0f114
@maflcko maflcko deleted the Mf1712-univalueBump branch December 20, 2017 15:55
@ryanofsky ryanofsky mentioned this pull request Dec 20, 2017
Merged
sickpig referenced this pull request in sickpig/BitcoinUnlimited Jan 10, 2018
@laanwj @sickpig
Port of core #11952: [qa] univalue: Bump subtree
c268e28 
88411e9 Squashed 'src/univalue/' changes from fe805ea74f..07947ff2da (MarcoFalke)

Pull request description:

  Pulls in the test changes to the univalue subtree.

  Beside looking at the code, reviewers should refer to https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/README.md#git-subtree-checksh on how to verify the subtree pull.

Tree-SHA512: 09493625a573dca1140570326ee90c1bb84e4893e1dab2cdd51bc23ae1fba1e33c43ed771ca9e112ac71b0242e8a8d058071334562c738d502587eadd5a0f114
@sickpig sickpig mentioned this pull request Jan 10, 2018
Merged
protonn pushed a commit to argentumproject/argentum that referenced this pull request Mar 4, 2018
@laanwj @protonn
Port of core bitcoin#11952: [qa] univalue: Bump subtree
7b19426 
88411e9 Squashed 'src/univalue/' changes from fe805ea74f..07947ff2da (MarcoFalke)

Pull request description:

  Pulls in the test changes to the univalue subtree.

  Beside looking at the code, reviewers should refer to https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/README.md#git-subtree-checksh on how to verify the subtree pull.

Tree-SHA512: 09493625a573dca1140570326ee90c1bb84e4893e1dab2cdd51bc23ae1fba1e33c43ed771ca9e112ac71b0242e8a8d058071334562c738d502587eadd5a0f114
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 31, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11952: [qa] univalue: Bump subtree
f4ed206 
88411e9 Squashed 'src/univalue/' changes from fe805ea74f..07947ff2da (MarcoFalke)

Pull request description:

  Pulls in the test changes to the univalue subtree.

  Beside looking at the code, reviewers should refer to https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/README.md#git-subtree-checksh on how to verify the subtree pull.

Tree-SHA512: 09493625a573dca1140570326ee90c1bb84e4893e1dab2cdd51bc23ae1fba1e33c43ed771ca9e112ac71b0242e8a8d058071334562c738d502587eadd5a0f114
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky left review comments

Assignees

No one assigned

Labels

Tests Utils/log/libs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maflcko @jnewbery @ryanofsky @meshcollider @laanwj