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

Jelmer Vernooij jelmer at samba.org
Fri Nov 23 21:22:48 MST 2012


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.

Cheers,

Jelmer



More information about the samba-technical mailing list