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

[simo]

On Sat, 2012-11-24 at 04:48 +0100, Jelmer Vernooij 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 ?
> 
> It's not just that. Compared to non-reviewer-push I ended up:
> 
>  * carefully apply all patches locally
>  * add the appropriate reviewed-by tags to the commits

These 2 steps is what we really care about, if you'd send back an email
saying "Reviewed-by: Name, email" that would be enough for me for simple
patches.

>  * run autobuild

For more complex patches I would expect people to make locally/autobuild
and test the change if there isn't a testsuite attached to the patch or
that covers the patch anyway whenever possible.
We do have cases where the setup needed to reproduce a problem being
fixed is hard, in those cases if the reviewer is really convinced the
fix is good, he can say so and add he didn't test. Perhaps it would be
wise to not push right away and ask if someone else think they can test.

>  * get e-mail back, look at logs
>  * check I had applied all patches correctly
>  * reproduce issue locally

I don't think you should have done this step, once it is clear there is
a problem you just mail back the author.

>  * send email to original author
> 
> Cheers,
> 
> Jelmer


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