Reviewer-push (was: Re: [PATCH 00/16] Get rid of obsolete system getpass().)

[simo]

On Sat, 2012-11-24 at 05:22 +0100, Jelmer Vernooij wrote:
> On Fri, 2012-11-23 at 22:48 -0500, simo wrote:
> > On Fri, 2012-11-23 at 22:44 -0500, simo wrote:
> > > On Sat, 2012-11-24 at 03:26 +0100, Jelmer Vernooij wrote:
> > > > On Sat, 2012-11-24 at 03:06 +0100, Jelmer Vernooij wrote:
> > > > > Hi Andreas,
> > > > > 
> > > > > On Fri, 2012-11-23 at 16:41 +0100, Andreas Schneider wrote:
> > > > > > The following patchset introduces a new function named samba_getpass so we can
> > > > > > get rid of the deprecated system getpass() function and the implementation
> > > > > > in libreplace. This way there should be no libreplace on Linux at all.
> > > > > This breaks the samba3 autoconf build:
> > > > > 
> > > > > lib/util_cmdline.o: In function `set_cmdline_auth_info_getpass':
> > > > > /memdisk/jelmer/a/b747076/samba3/source3/lib/util_cmdline.c:262:
> > > > > undefined reference to `samba_getpass'
> > > > > collect2: ld returned 1 exit status
> > > > > make: *** [bin/testparm] Error 1
> > > > > make: *** Waiting for unfinished jobs....
> > > > > lib/util_cmdline.o: In function `set_cmdline_auth_info_getpass':
> > > > > /memdisk/jelmer/a/b747076/samba3/source3/lib/util_cmdline.c:262:
> > > > > undefined reference to `samba_getpass'
> > > > > collect2: ld returned 1 exit status
> > > > > make: *** [bin/smbta-util] Error 1
> > > > > lib/util_cmdline.o: In function `set_cmdline_auth_info_getpass':
> > > > > /memdisk/jelmer/a/b747076/samba3/source3/lib/util_cmdline.c:262:
> > > > > undefined reference to `samba_getpass'
> > > > > collect2: ld returned 1 exit status
> > > > > make: *** [bin/smbcontrol] Error 1
> > > > > lib/util_cmdline.o: In function `set_cmdline_auth_info_getpass':
> > > > > /memdisk/jelmer/a/b747076/samba3/source3/lib/util_cmdline.c:262:
> > > > > undefined reference to `samba_getpass'
> > > > > collect2: ld returned 1 exit status
> > > > > make: *** [bin/nmblookup] Error 1
> > > > 
> > > > Incidentally, this is one of the reasons why I don't like reviewer-push.
> > > > If an autobuild fails, I'd rather have the original developer deal with
> > > > debugging the problem, and coming back to the reviewer with a request
> > > > for a follow-up review. Instead, this just makes reviews more
> > > > time-consuming (thus increasing the bar for reviews to happen).
> > > 
> > > If autobuild fails, you just send the author the logs, or tell him to
> > > check out why it failed. Is it really that hard to fwd an autobuild
> > > email ?
> > 
> > Ah FWIW, I do not care who pushes as long as there is a Reviewed-by:
> > flag given by a reviewer.
> > This is fine for small or obvious patches.
> I agree, I think that's the important thing too.
> 
> > However giving a review flag w/o even building once for anything that is
> > not trivial is not really a good thing. When you review you are supposed
> > to really go through the patch, and for very complex patches it very
> > well deserves a build and a test of what is being changed.
> > Note that this doesn't mean catching things like the autoconf breakage
> > (well unless the patch is about changing the autoconf build I guess :),
> > that is what autobuild is for, whoever pushes.
> autobuild already takes care of making sure that the changes actually
> build successfully and that 'make test' passes. I don't really see what
> me (as reviewer) doing the same thing locally really adds there. 
> 
> I agree carefully going through and understanding the patch is very
> important; that's what reviewing is about. Building it doesn't improve
> my understanding of it.
> 
> It's sometimes indeed also useful to try out the changes manually (and
> that of course implies building), e.g. in the case of changes that don't
> have tests. However, I always expect the original developer to have also
> already tested and I would much more likely ask for unit tests to be
> added. 
> 
> Either way, I think the original author should land so that they can
> deal with any fallout during landing. That way the review process is as
> lightweight as possible and focused on making sure changes are correct,
> rather than being involved in the process of landing code.

+1

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>