Bitcoin Forum
September 04, 2024, 01:17:46 AM *
News: Latest Bitcoin Core release: 27.1 [Torrent]
 
  Home Help Search Login Register More  
  Show Posts
Pages: [1]
1  Bitcoin / Development & Technical Discussion / Weight of ACKs of New Reviewers on: August 22, 2019, 08:11:40 AM
Say I'm a fresh contributor and I would like to review somebody's pull request. If I read the code and find issues with it and bring them up - that would obviously contribute. The issue raised is proof that I actually reviewed the code. However, if I just pull the branch being reviewed, test it and then report that the code tested ok, I might be, in the worst case scenario, not telling the truth. So, I'm guessing that as a new contributor if I give an ACK, utACK, etc., their weight (I hope) is much lower than that of seasoned contributors.
The question is then - as a new reviewer, should I bother writing 'unprovable' review conclusions (such as "I ran the tests and they pass") and should just concentrate on raising issues with the code?
2  Bitcoin / Development & Technical Discussion / Re: Correct fixing and commit squashing workflow in a pull request on: August 18, 2019, 09:11:49 AM
Thanks! That cleared things up   Smiley

So, just to make sure I understand correctly - if an ACK was given on a certain commit by a reviewer and I force push, that would basically invalidate that ACK and would require that the reviewer reviews the change again, only this time it would be easier for them because they could use git range-diff or the Github UI to see the diff between the commit they already reviewed and the force pushed one, right?
3  Bitcoin / Development & Technical Discussion / Correct fixing and commit squashing workflow in a pull request on: August 17, 2019, 02:22:57 PM
I'm currently working on a new issue and have an open pull request.
I'm pretty sure (please correct me if I'm wrong here) that before a pull request is merged the commits must be squashed. What I'm not sure is when to squash and in either case I can think of there is a problem.
Let's say someone reviewed my code and I fixed it based on their review. Should I:
1. Commit, squash commits and force push, which would require the reviewer, if they want to review my fix, to go over everything again - they can't make a diff with the previous version before the fix (..or can they?)
2. Fix and push without squashing so that the reviewer could easily review the fix, wait for their ACK and then squash commits. However, if I understand correctly, the person responsible to make the merge will look for several ACKs on the same commit and then they should merge that commit into master.  However, if I'm the one squashing the commits after all the ACKs, wouldn't it change the hash of the last commit which was ACKed by everyone and thus, ideally it needs to be reviewed again so that, for example, a malicious "contributor" wouldn't insert some malicious line of code along with the squash?

So option (1) is a wasteful on developer's time while option (2) might be used as a vector to introduce malicious code changes. What's the solution?
Pages: [1]
Powered by MySQL Powered by PHP Powered by SMF 1.1.19 | SMF © 2006-2009, Simple Machines Valid XHTML 1.0! Valid CSS!