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

[simo]

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.

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.

Simo.

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