Re: [Qemu-devel] [PATCH] get_maintainer.pl: Default to --no-git-fallback

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Oct 21, 2014 at 03:29:14PM +0200, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Tue, Oct 21, 2014 at 02:22:41PM +0200, Markus Armbruster wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> 
> >> > On Tue, Oct 21, 2014 at 11:31:12AM +0200, Markus Armbruster wrote:
> >> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> >> 
> >> >> > On Mon, Oct 20, 2014 at 03:19:52PM +0100, Peter Maydell wrote:
> >> >> >> On 20 October 2014 15:15, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> >> >> > On Mon, Oct 20, 2014 at 03:04:44PM +0100, Peter Maydell wrote:
> >> >> >> >> On 20 October 2014 10:19, Markus Armbruster
> >> >> >> >> <armbru@redhat.com> wrote:
> >> >> >> >> > Contributors rely on this script to find maintainers to copy.  The
> >> >> >> >> > script falls back to git when no exact MAINTAINERS pattern matches.
> >> >> >> >> > When that happens, recent contributors get copied, which
> >> >> >> >> > tends not be
> >> >> >> >> > particularly useful.  Some contributors find it even annoying.
> >> >> >> >> >
> >> >> >> >> > Flip the default to "don't fall back to git".  Use
> >> >> >> >> > --git-fallback to
> >> >> >> >> > ask it to fall back to git.
> >> >> >> 
> >> >> >> >> Good idea.
> >> >> >> 
> >> >> >> > What do you want to happen in this case?
> >> >> >> 
> >> >> >> It should mail the people who are actually maintainers,
> >> >> >> not anybody who happened to touch the code in the last
> >> >> >> year.
> >> >> >
> >> >> > Right but as often as not there's no data about that
> >> >> > in MAINTAINERS.
> >> >> 
> >> >> The way to fix that is finding maintainers, not scatter-shooting patches
> >> >> to random contributors in the vague hope of hitting someone who cares.
> >> >> 
> >> >> >> > I'm yet to see contributors who are annoyed but we
> >> >> >> > can always blacklist specific people.
> >> >> >> 
> >> >> >> At the moment I just don't use get_maintainers.pl at
> >> >> >> all because I tried it a few times and it just cc'd
> >> >> >> a bunch of irrelevant people...
> >> >> >> 
> >> >> >> I suspect anybody using it at the moment is either
> >> >> >> using the --no-git-fallback flag or trimming the
> >> >> >> cc list a lot.
> >> >> >> 
> >> >> >> thanks
> >> >> >> -- PMM
> >> >> >
> >> >> > I'm using it: sometimes with --no-git-fallback, sometimes without.
> >> >> 
> >> >> I'm using it, but I absolutely want to know when it falls back to git,
> >> >> because then I want to cheack and trim or ignore its output every single
> >> >> time.
> >> >
> >> >
> >> > Well it tells you the role. What else is necessary?
> >> 
> >> For my own use in sending patches, nothing.  I know how to use it to
> >> help me copy the right people.
> >> 
> >> >> > IIUC the default is to have up to 5 people on the Cc list
> >> >> > (--git-max-maintainers).
> >> >> > It's not like it adds 200 random people, is it?
> >> >> >
> >> >> > Anyway experienced contributors can figure it out IMHO.
> >> >> 
> >> >> Experienced contributors can figure out --git-fallback, too.
> >> >
> >> > Exactly.
> >> >
> >> >> What we see is contributors, especially less experienced ones, copying
> >> >> whatever get_maintainers.pl spits out, because they have no idea what
> >> >> get_maintainers.pl actually does.
> >> >
> >> > Exactly. And this seems better than just sending to qemu ML
> >> > and not copying anyone.
> >> 
> >> That's where we disagree.
> >> 
> >> Personally, I don't mind getting punished for contributing patches by
> >> getting copied indiscriminately all that much.  It's a drain on my time,
> >> but I can cope.  However, I know people who do mind, and some of them
> >> have spoken up in this thread.
> >> 
> >> Copying people is not free.  You should *think* before you copy.
> >> 
> >> An entry in MAINTAINERS dispenses you from this obligation, because the
> >> people listed explicitly asked for a copy.
> >> 
> >> Finding someone in git-log does not!
> >> 
> >> get_maintainers.pl encourages its users to treat people found in git-log
> >> exactly like the ones in MAINTAINERS.  Treating them the same is
> >> *wrong*.
> >> 
> >> >> > Question in my mind is what do we want a casual contributor
> >> >> > to do if there's no one listed in MAINTAINERS.
> >> >> > "Look in MAINTAINERS, if not there, look in git log"
> >> >> > sounds very reasonable to me, better than "CC no one".
> >> >> 
> >> >> But that's not what we do!  We do "copy whatever get_maintainers.pl
> >> >> coughs up", which boils down to "use MAINTAINERS, if not there, grab
> >> >> some random victims from git-log".
> >> >
> >> > Sorry, what's the difference?
> >> > "look in" versus "random victims"? what makes them random?
> >> 
> >> The difference is using get_maintainers.pl to help finding whom to copy
> >> vs. blindly copying whoever get_maintainers.pl coughs up.
> >> 
> >> > Maybe you just want to increase git-min-percent?
> >> >
> >> >> Perhaps we'd get slightly better results if get_maintainers.pl told its
> >> >> users clearly about the two kinds of output it may produce: maintainers
> >> >> (must be copied on patches), and recent contributors (you're in trouble;
> >> >> copying some of them may or may not help).
> >> >
> >> > That's what it does: it reports the role, and the percent.
> >> 
> >> Boldly assumes the user of get_maintainers.pl knows what it does, and
> >> knows how to interpret runes like (commit_signer:14/22=64%).
> >
> > OK so you would like a flag for a more readable output?
> > Sounds very reasonable.
> 
> Inexperienced contributors are unlikely to find a flag, so it better be
> the default.

Fine with me, send a patch. Might be useful for Linux where we got
this from, too.

> >> > What's missing?
> >> 
> >> What's really missing is decent coverage by MAINTAINERS.  I figure my
> >> patch is controversial only because MAINTAINERS is so woefully
> >> incomplete.
> >
> > In fact if MAINTAINERS covered everything your patch won't be needed
> > right?
> 
> Correct.  The more MAINTAINERS covers, the less of a difference my patch
> makes.
> 
> >> My patch to get_maintainers.pl triggered a whole thread, while the
> >> message I sent on MAINTAINERS coverage got just one reply so far, and
> >> even that one's really just about get_maintainers.pl.  Disappointing.
> >> Looks like we're still looking for an easy technical fix.  I doubt there
> >> is one.
> >
> > At least for myself, that's because I'm Cc'd directly on the patch
> > but not on the MAINTAINERS coverage mail.
> > And that's ... because get_maintainers picks my mail from git?
> >
> > See how it's useful now?
> 
> Except that's not what happened.
> 
>     $ scripts/get_maintainer.pl --git-fallback -f scripts/get_maintainer.pl 
> 
> No output.  I picked you from git-log manually.

Weird.
It works for me:
 ./scripts/get_maintainer.pl -f scripts/get_maintainer.pl 
"Michael S. Tsirkin" <mst@redhat.com> (commit_signer:1/1=100%)                                                                                    

Maybe --git-fallback is broken?
Another reason to defer this patch ...

> >> If you have better ideas on how to mitigate the excessive and useless
> >> copying we now see, please post a patch.
> >
> > We need more maintainers :)
> 
> Yes, we do.  Until we got them, we need fewer useless copies.

Classical long tail problem. It's hard to get rid of useless copies
without getting rid of useful ones :)

-- 
MST