On 07/18/2012 11:35 PM, Roan Kattouw wrote:
On Wed, Jul 18, 2012 at 9:30 PM, Subramanya Sastry ssastry@wikimedia.org wrote:
(b) Commit amends hide evolution of an idea and the tradeoffs and considerations that went into development of something -- the reviews are all separate from git commit history. All that is seen is the final amended commit -- the discussion and the arguments even that inform the commit are lost. Instead, if as in (a), a topic branch was explicitly (or automatically created when a reviewer rejects a commit), and fixes are additional commits, then, review documentation could be part of the commit history of git and shows the considerations that went into developing something and the evolution of an idea.
There was an email recently on wikitext-l where Mark Bergsma was asked to squash his commits (http://osdir.com/ml/general/2012-07/msg20847.html) -- I personally think it is a bad idea that is a result of the gerrit review model. In a different review model, a suggestion like this would not be made.
Although squashing and amending has downsides, there is also an advantage: now that Jenkins is set up properly for mediawiki/core.git , we will never put commits in master that don't pass the tests. With the fixup commit model, intermediate commits often won't pass the tests or even lint, which leads to false positives in git bisect and makes things like rolling back deployments harder.
Agree somewhat. But, two things to consider:
(a) Not all commits that are reviewed break tests. I am not arguing that amends and squashing via rebase dont have a place. I am arguing that amends are overused right now. Where commits break an existing test suite, part of the review requirement could be that the previous commit be amended so it passes tests. But, applying this logic in all cases doesn't seem right to me.
(b) What happens if I submit a new test that breaks HEAD? Are we arguing that the test cannot be committed till HEAD passes all tests? Will my test be held in abeyance till someone gets around to fixing HEAD?
I think there is an argument to be made for a non-straitjacketed approach to review: always pre-commit review, commit is always a single unit of review, always amend commits, always squash multiple commits -- this is the gerrit model which I am chafing against.
I am new here and I also have limited exposure to review tools and processes, so I dont want to overstate my case. I also understand that this is not a simple problem, and maybe this is the best we have -- but I at least want to express my annoyance with what we have, and that we should reduce this to being entirely an UI issue (which is important in and of itself).
Also, my gratitude to all of you who have previously spent the time instituting processes and tools -- it is better than nothing, and it can be a thankless job with people like me griping :) but my intention here is to only help this forward where I can. Can I work with what we have? Yes, I can. Will I be happier with a different process / tool? Yes, I will be :).
Subbu.