Perform a weaker subtree check in Travis

[sipa]

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

[laanwj]

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]

With bitcoin-core/secp256k1#478 merged, the test should pass on secp256k1 now right?

[gmaxwell]

@meshcollider No, this won't pass for secp256k1 until we do another subtree merge of secp256k1.

[theuni]

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

[maflcko]

Concept ACK. To err on the safe side, you could amend the yaml:

git:
  depth: 99999

[maflcko]

utACK 9c99247446911a26616d384c9f8fef720c123ad7

[sipa]

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.

[maflcko]

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" ***@***.***> wrote: Rebased after merge of #11420 <#11420> and #11421 <#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 <#11394 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGGmvzcXOCVijZar9eXpqtSSUcAFkWdHks5srUUigaJpZM4PiHMl> .

[maflcko]

re-utACK 487aff4