Perform a weaker subtree check in Travis #11394

pull sipa wants to merge 2 commits into bitcoin:master from sipa:201709_travis_subtree changing 2 files +30 −9

sipa commented at 1:46 AM on September 25, 2017: member
Apparently many of our subtrees get modified by PRs in this repository, without getting noticed.

To improve upon this:
- Make git-subtree-check.sh capable of doing a weaker consistency check (that doesn't need access to external repositories), but which should be sufficient to detect unintended changes. It can be fooled by a fake subtree merge commit, but that would hopefully be obvious to reviewers.
- Make Travis invoke this subtree check for each of our subtrees.
Note that Travis is currently expected to fail on this PR, as 2 out of 4 subtrees (src/secp156k1 and src/univalue have been modified directly in master).
fanquake added the label Scripts and tools on Sep 25, 2017
sipa force-pushed on Sep 25, 2017
laanwj commented at 9:52 AM on September 25, 2017: member

Apparently many of our subtrees get modified by PRs in this repository, without getting noticed.

We try to catch these in review, but apparently some changes still sneaked through. Thanks for adding a travis check.
meshcollider commented at 10:06 AM on September 25, 2017: contributor

With https://github.com/bitcoin-core/secp256k1/pull/478 merged, the test should pass on secp256k1 now right?
gmaxwell commented at 4:53 PM on September 25, 2017: contributor

@MeshCollider No, this won't pass for secp256k1 until we do another subtree merge of secp256k1.
theuni commented at 6:55 PM on September 25, 2017: member

Travis uses a shallow clone with depth 50: https://travis-ci.org/bitcoin/bitcoin/jobs/279351451#L478

In reality, git stores not only 50 commits, but far enough back to grab parents of any of those 50, which may be pretty far back if they're merges (which all of ours should be).

So.. Travis will need some luck to find all of the previous subtree merge commits, though the odds are pretty good. If the last merge was quite a long time ago, though, I believe we'd fail to notice any non-subtree-merge changes.

As a belt-and-suspenders, maybe add a dumb non-merge-commit checker? something like testing: git rev-list --count --no-merges -n1 $TRAVIS_COMMIT_RANGE -- src/secp256k1
MarcoFalke commented at 1:04 PM on September 27, 2017: member
Concept ACK. To err on the safe side, you could amend the yaml:
```
git:
  depth: 99999
```
MarcoFalke commented at 1:54 PM on September 29, 2017: member

utACK 9c99247446911a26616d384c9f8fef720c123ad7

Improve git-subtree-check.sh

We have several pieces of information about subtrees:
1) What their current directory contents is
2) What their directory contents was at the time of the last subtree merge
3) What the directory contents of the upstream project is in the commit referred to by the subtree merge.

Normally, all 3 should be identical. git-subtree-check.sh so far only compared (1) with (3) however.

Fix this by comparing all three, and give some more useful diff output in the case of mismatch.

The added benefit is that (1) and (2) can be compared without needing to see the upstream repository.

e1d0cc23a9

Check subtree consistency in Travis 487aff4218
sipa force-pushed on Oct 11, 2017
sipa commented at 10:44 PM on October 11, 2017: member

Rebased after merge of #11420 and #11421; passes cleanly now. @theuni Unsure if we should make it fetch that much back - I assume that would come with some unnecessary performance load?

Giving a good error message to detect this case would be useful though.
MarcoFalke commented at 2:15 AM on November 4, 2017: member

Last time I checked it failed with an appropriate error message. If no one objects, I will schedule this for merge

On Oct 11, 2017 15:45, "Pieter Wuille" notifications@github.com wrote:

Rebased after merge of #11420 https://github.com/bitcoin/bitcoin/pull/11420 and #11421 https://github.com/bitcoin/bitcoin/pull/11421; passes cleanly now.

@theuni https://github.com/theuni Unsure if we should make it fetch that much back - I assume that would come with some unnecessary performance load?

Giving a good error message to detect this case would be useful though.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/11394#issuecomment-335970130, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmvzcXOCVijZar9eXpqtSSUcAFkWdHks5srUUigaJpZM4PiHMl .
MarcoFalke commented at 10:03 PM on November 9, 2017: member

re-utACK 487aff42184117ae3b8be4381b77587448e297a5
MarcoFalke referenced this in commit c838283ecd on Nov 9, 2017
MarcoFalke merged this on Nov 9, 2017
MarcoFalke closed this on Nov 9, 2017
MarcoFalke referenced this in commit 6e4e98ee8c on Nov 9, 2017
zkbot referenced this in commit caed4adf50 on Nov 10, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#11649 Revert "Remove unused variable in shell script"