utACK 6d1016f

utACK

Ping @TheBlueMatt

Which features are bash-specific (I'm pretty sure we went through this when it was merged, and there were none left)? We should just remove the bash-specific ones, not revert to forcing bash.

Also, I do prefer to not remove the commits list message...its not there to prevent someone who has malicious access to the repo, its there to encourage people to audit the commits that were merged. If you prefer, just replace with it a notice to tell people to run git log on contrib/verify-commits manually.

@TheBlueMatt The two constructs that don't seem to work with /bin/sh are:

https://github.com/petertodd/bitcoin/blob/2016-03-fix-verify-commits/contrib/verify-commits/verify-commits.sh#L28

https://github.com/petertodd/bitcoin/blob/2016-03-fix-verify-commits/contrib/verify-commits/gpg.sh#L2

@TheBlueMatt Again, if you want to encourage people, do it in a README file or similar which doesn't give the wrong impression about what's the right way to do it.

Anyway, the necessity to audit the codebase in general should be something every security conscious developer should be aware of; calling out this program specifically may in itself give people the wrong impression.

utACK 6d1016f543837fe3c7b0a48573771f4b17e35ff5

@sipa Sorry, just rebased and added one more commit.

ACK e2764cb23826908bde171067fc5d0b0659b18952

utACK e2764cb

NACK. Please fix the two bash-isms instead of requiring bash, and avoid adding more OS-specificisms with GNU-isms (which aren't even in some BSDs). Feel free to move the warnings to a README file of somesort, though.

@TheBlueMatt I don't know enough about bash to know what needs to be fixed or how; if you'd like to suggest some changes please do. But if it's not going to get fixed I'd rather the failure be clear - "bash is needed and isn't installed" - than unclear.

The three commits at https://github.com/TheBlueMatt/bitcoin/commits/2016-20-7713-fixes should replace your top three just fine.

Incorporated @TheBlueMatt's /bin/sh fixes, so no longer depending on bash again.

reACK 11164ec (is it me, or is it obviously slower now?)

Please do leave the warning telling people to check git log in... making people see it every time until it's removed definitely has value. If not it should be added to a README somewhere.

On May 21, 2016 3:27:17 AM PDT, Gregory Maxwell notifications@github.com wrote:

reACK 11164ec

---

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#7713 (comment)

@TheBlueMatt Git has had vulnerabilities before (example) where even just checking out source is sufficient to pwn your system; if you can run git log on the verify-commits directory it may be too late. If we're going to be giving advice on how to use verify-commits we should do it properly; for now removing that advice is very appropriate given that it's misleading at best.

I find it rather unlikely that git log would have a vulnerability that cant also be triggered by some other checkout/pull/etc flow. I dont think telling people to run git log is bad advice here.
I agree the existing auto-run-git log isnt ideal, but I dont see why telling people to do so is bad, let alone why removing it entirely is better.

Please add the warning back in. Seems otherwise we don't get agreement here to merge this. Its removal can be discussed separately.

Added README explaining how to use verify-commits.sh safely; this should answer @TheBlueMatt's objection to removing the warning.

Tested ACK (checked out this branch in one worktree, master in another, verified master using the code from this branch).

Anything left to do here?

@MarcoFalke IMO it's ready for merging.

"This is an incomplete work in progress,"

It is? I'd say it works pretty well for its goals?

On June 16, 2016 3:53:32 AM PDT, Peter Todd notifications@github.com wrote:

@MarcoFalke IMO it's ready for merging.

---

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#7713 (comment)

Also you might want to note that you can run git log with paths to get a list of changes to that path in the README, just because it's useful for these scripts and not really an obvious feature that everyone is aware of.

On June 16, 2016 3:53:32 AM PDT, Peter Todd notifications@github.com wrote:

@MarcoFalke IMO it's ready for merging.

---

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#7713 (comment)

Can we please finish this?
This is pretty much a trivial change, but every time there comes up a new load of nits, which of those things is critical and can't be fixed later?

@laanwj I think we can improve the wording of the doc later.

I mean I think the README is just wrong right now? We could have also taken the easy solution of splitting out the contentious change.

On June 17, 2016 4:01:23 AM PDT, MarcoFalke notifications@github.com wrote:

@laanwj I think we can improve the wording of the doc later.

---

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#7713 (comment)

@TheBlueMatt Sorry, but I'm kinda mystified why you think the README is wrong; I don't see it as controversial at all to describe this functionality as incomplete, given that we definitely need a convenient way for users to verify commits safely, prior to accidentally checking out repos and possibly running malicious code. I think it's good to point that out in the hope that others continue this starting point - and it's work that many other projects need as well (I personally use verify-commits in python-bitcoinlib, as well as some internal-use-only repos).

@MarcoFalke Fixed your nit re: sipa's old key.

ACK 1e9aab0

Readme seems fine to me.