Make verify-commits.sh test that merges are clean #12708

Unsure if we want this.

This modifies verify-commits.sh to redo all merges along the leftmost commit branch (which includes all PR merges), and verify whether they match the merge commit's trees.

The benefit is that it will detect a case where one of the maintainers merges a PR, but makes an unrelated change inside the merge commit. This on itself is not very strong, as unrelated changes can also be included in the merged branch itself - but perhaps the merge commit is not something that people are otherwise likely to look at.

Fixes #8089

Thanks. Concept ACK

On Mar 16, 2018 18:29, "Pieter Wuille" notifications@github.com wrote:

Unsure if we want this.

This modifies verify-commits.sh to redo all merges along the leftmost commit branch (so it should contain all PR merges), and verify whether the match the result of the merge.

The benefit is that it will detect a case where one of the maintainers merges a PR, but makes an unrelated change inside the merge commit. This on itself is not very strong, as unrelated changes can also be included in the merged branch itself, but perhaps the merge commit is not something that people are otherwise likely to look.

You can view, comment on, or merge this pull request online at:

#12708 Commit Summary

• Make verify-commits.sh test that merges are clean

File Changes

• M contrib/verify-commits/verify-commits.sh https://github.com/bitcoin/bitcoin/pull/12708/files#diff-0 (32)

Patch Links:

• #12708.patch
• #12708.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/12708, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv3mzklUc9NUFx-O3YjjuqTA6YX-Nks5te_aFgaJpZM4SuKjf .

Concept Ack

Concept ACK, this makes sense because the merge script github-merge.py (that everyone is supposed to use) will only do clean merges.

• How does this affect performance of the verify script? it's already very slow, so the difference might be negligible)
• How deep should this check be done?

The script becomes several times slower (I didn't measure; just my impression from waiting... maybe a few minutes). Perhaps we should restrict it to just one day of commits or so?

Similar to the SHA512 hash, we could only check it for HEAD. (Or maybe make that configurable to check all the way down to trusted_root if you have time)

Fixes #8089

utACK 577f11141c5155345c68809aac8511b018cc4050, just reviewing the shell script code. Concept also seems good. I'm almost surprised there is no standalone tool like git-is-clean-merge <commit>, since merge commits are more cumbersome to verify than normal commits.

utACK 577f111

Let's observe how much this adds to Travis checks of master (it does not run on PR branches). If it's unacceptably slow, we can modify the script to only test merge cleanliness for the last few merges or days or so.

Yes, I had the same reasoning, it's good to have this check in, if it becomes too slow we can always tone that down.

#8672 is not clean, maybe we should ignore this

Merge commit 6052d509105790a26b3ad5df43dd61e7f1b24a12 is not clean
diff --git a/src/qt/transactiondesc.cpp b/src/qt/transactiondesc.cpp
index 65144e7..32b3955 100644
--- a/src/qt/transactiondesc.cpp
+++ b/src/qt/transactiondesc.cpp
@@ -241,7 +241,7 @@ QString TransactionDesc::toHTML(CWallet *wallet, CWalletTx &wtx, TransactionReco
         strHTML += "<br><b>" + tr("Comment") + ":</b><br>" + GUIUtil::HtmlEscape(wtx.mapValue["comment"], true) + "<br>";
 
     strHTML += "<b>" + tr("Transaction ID") + ":</b> " + rec->getTxID() + "<br>";
-    strHTML += "<b>" + tr("Transaction total size") + ":</b> " + QString::number(wtx.GetTotalSize()) + " bytes<br>";
+    strHTML += "<b>" + tr("Transaction size") + ":</b> " + QString::number(wtx.GetTotalSize()) + " bytes<br>";
     strHTML += "<b>" + tr("Output index") + ":</b> " + QString::number(rec->getOutputIndex()) + "<br>";
 
     // Message from normal bitcoin:URI (bitcoin:123...?message=example)