On 05/16/2012 05:28 PM, Matt Wagner wrote:
On Wed, May 16, 2012 at 10:03:17AM -0400, Jason Guiditta wrote:
> Hey, list, had a thought on pushing patches. I have noticed some
> people (inconsistently) pushing patches others send after they review
> them, and adding a 'signed-off by' to the patch. I kind of like this,
> as it adds a simple way to see who reviewed a patch.
This is easily accomplished with adding "-s" to your git-am run.
> This can make it
> easy to ask that person 'hey, I am hitting this problem after that
> patch, did you see this when you tested it?'. Right now, it is a
> matter of combingthe list to find the ack, and if you delete your
> local copies of pushed patches (as I do), that means going to the
> archive, just t see who reviewed a patch so you can ask them a
> question. I would personally much rather just do 'git log' and see
> who it was, so I can get hold of them as needed. Thoughts, concerns,
> vehement opposition? General apathy?
Actually, I think I do have some opposition. :-\ Adding a signed-off-by
line is nice, but it implies that the reviewer is the one that pushes.
This is all well and good, except I think that we're going to have to
start juggling multiple branches soon, as we backport things to
maintenance branches.
Compounding matters, I don't think many people really enjoy doing patch
reviews -- it's just something we begrudgingly do to help get patches
approved. (I can't say I speak for everyone, but I do know that given
the choice of writing code or reviewing someone's patch, I'm not the
only one that would choose to keep coding.) If reviewing someone's patch
suddenly saddles you with the responsibility of figuring out what
branches it needs to go onto, pushing it to all of them, and resolving
any conflicts that might arise, it's going to become considerably more
unappealing than it already is.
Just MHO, of course.
-- Matt
OT:
Honestly, I like both parts (writing, reviewing) because:
- reading others code is sometimes inspiring
- you can look at a review as a challenge if you are able to break the
code (find weaknesses)
- it's easiest place where you can effect quality of code
Though it's quite difficult/annoying to keep both in balance because:
- it's quite difficult to keep overview if some patch needs a review or
not, there are many hands on this project and big traffic on the list
(where are those times when it was possible to read through all patches
on the list)
- most patches land on the list few days before sprint ends, and doing
only reviews couple of days is not very attractive
even more OT:
What is really demotivating for me is if I send a patch which then waits
week(s) on the list and I have to beg for reviewing it, I hope this gets
better when we start using a review tool.
Jan