On 07/18/2012 11:35 PM, Roan Kattouw wrote:
On Wed, Jul 18, 2012 at 9:30 PM, Subramanya Sastry
<[email protected]> 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.
_______________________________________________
Wikitech-l mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
