[PATCH] scripts/get_maintainer.pl: Default to --no-rolestats when output not a terminal

[PATCH] scripts/get_maintainer.pl: Default to --no-rolestats when output not a terminal

[Josh Triplett]

scripts/get_maintainer.pl defaults to showing --rolestats, which
provides annotations explaining why each person or list might want to
know about a patch.  This works well for interactive use, but breaks
when used with git send-email's --to-cmd or --cc-cmd, resulting in
malformed email headers and mails sent to some but not all recipients.

To avoid the need to explicitly pass --no-rolestats for batch use,
enable --rolestats by default only when outputting to a terminal.

Reported-by: Alex Kelly <alex.page.kelly@gmail.com>
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
 scripts/get_maintainer.pl |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 8b673dd..dcb0748 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -40,7 +40,8 @@ my $email_use_mailmap = 1;
 my $output_multiline = 1;
 my $output_separator = ", ";
 my $output_roles = 0;
-my $output_rolestats = 1;
+my $output_rolestats = 0;
+$output_rolestats = 1 if -t STDOUT;
 my $scm = 0;
 my $web = 0;
 my $subsystem = 0;
@@ -768,7 +769,8 @@ Other options:
 
 Default options:
   [--email --nogit --git-fallback --m --n --l --multiline -pattern-depth=0
-   --remove-duplicates --rolestats]
+   --remove-duplicates]
+  When outputting to a terminal, --rolestats defaults to enabled.
 
 Notes:
   Using "-f directory" may give unexpected results:
@@ -781,9 +783,10 @@ Notes:
       Used with "--git-blame", does not iterate all files in directory
   Using "--git-blame" is slow and may add old committers and authors
       that are no longer active maintainers to the output.
-  Using "--roles" or "--rolestats" with git send-email --cc-cmd or any
-      other automated tools that expect only ["name"] <email address>
-      may not work because of additional output after <email address>.
+  Using "--roles" or "--rolestats" with git send-email --to-cmd or --cc-cmd, or
+      any other automated tools that expect only ["name"] <email address> may
+      not work because of additional output after <email address>.  These
+      options default to disabled when not outputting to a terminal.
   Using "--rolestats" and "--git-blame" shows the #/total=% commits,
       not the percentage of the entire file authored.  # of commits is
       not a good measure of amount of code authored.  1 major commit may
-- 
1.7.10.4

Re: [PATCH] scripts/get_maintainer.pl: Default to --no-rolestats when output not a terminal

[Joe Perches]

On Fri, 2012-08-03 at 11:27 -0700, Josh Triplett wrote:
> scripts/get_maintainer.pl defaults to showing --rolestats, which
> provides annotations explaining why each person or list might want to
> know about a patch.  This works well for interactive use, but breaks
> when used with git send-email's --to-cmd or --cc-cmd, resulting in
> malformed email headers and mails sent to some but not all recipients.
> 
> To avoid the need to explicitly pass --no-rolestats for batch use,
> enable --rolestats by default only when outputting to a terminal.

Hi Josh.

I think it's preferable to add --no-rolestats
to the uses that need them.

I have different scripts that I use for git send-email
options --to-cmd and --cc-cmd

$ cat ~/bin/to.sh
#!/bin/bash

opts="--nogit --nogit-fallback --norolestats --pattern-depth=1"

if [[ $(basename $1) =~ ^0000- ]] ; then
    ./scripts/get_maintainer.pl --nom $opts  $(dirname $1)/*
else
    maint=$(./scripts/get_maintainer.pl --nol $opts $1)
    if [ "$maint" == "" ] ; then
	echo "linux-kernel@vger.kernel.org"
    else
	echo "$maint"
    fi
fi

$ cat ~/bin/cc.sh
#!/bin/bash

opts="--nogit --nogit-fallback --norolestats"

if [[ $(basename $1) =~ ^0000- ]] ; then
    ./scripts/get_maintainer.pl --nom $opts  $(dirname $1)/*
else
    ./scripts/get_maintainer.pl $opts $1
fi

Re: [PATCH] scripts/get_maintainer.pl: Default to --no-rolestats when output not a terminal

[Josh Triplett]

On Fri, Aug 03, 2012 at 11:33:21AM -0700, Joe Perches wrote:
> On Fri, 2012-08-03 at 11:27 -0700, Josh Triplett wrote:
> > scripts/get_maintainer.pl defaults to showing --rolestats, which
> > provides annotations explaining why each person or list might want to
> > know about a patch.  This works well for interactive use, but breaks
> > when used with git send-email's --to-cmd or --cc-cmd, resulting in
> > malformed email headers and mails sent to some but not all recipients.
> > 
> > To avoid the need to explicitly pass --no-rolestats for batch use,
> > enable --rolestats by default only when outputting to a terminal.
> 
> Hi Josh.
> 
> I think it's preferable to add --no-rolestats
> to the uses that need them.

Why?

> I have different scripts that I use for git send-email
> options --to-cmd and --cc-cmd
[...snip scripts...]

You've submitted enough patches that you've automated as much of the
process as you can; I don't think that makes the defaults less
error-prone.  Given that you've had to explicitly add --no-rolestats to
your scripts, that seems like evidence in *favor* of making this change.

As it stands now, the current default of --rolestats makes the obvious
command line of
git send-email --to-cmd='scripts/get_maintainer.pl' *.patch
send broken emails that go to some maintainers but not all.  I think it
makes sense to change the default so that the obvious usage becomes the
correct one.

- Josh Triplett

Re: [PATCH] scripts/get_maintainer.pl: Default to --no-rolestats when output not a terminal

[Joe Perches]

On Fri, 2012-08-03 at 11:47 -0700, Josh Triplett wrote:
> On Fri, Aug 03, 2012 at 11:33:21AM -0700, Joe Perches wrote:
> > On Fri, 2012-08-03 at 11:27 -0700, Josh Triplett wrote:
> > > scripts/get_maintainer.pl defaults to showing --rolestats, which
> > > provides annotations explaining why each person or list might want to
> > > know about a patch.  This works well for interactive use, but breaks
> > > when used with git send-email's --to-cmd or --cc-cmd, resulting in
> > > malformed email headers and mails sent to some but not all recipients.
> > > 
> > > To avoid the need to explicitly pass --no-rolestats for batch use,
> > > enable --rolestats by default only when outputting to a terminal.
> > 
> > Hi Josh.
> > 
> > I think it's preferable to add --no-rolestats
> > to the uses that need them.
> 
> Why?
> 
> > I have different scripts that I use for git send-email
> > options --to-cmd and --cc-cmd
> [...snip scripts...]
> 
> You've submitted enough patches that you've automated as much of the
> process as you can; I don't think that makes the defaults less
> error-prone.

I think the default use of the get_maintainer script is
actually not scripted but interactive, where the user is
just trying to figure out who the maintainer is.

Anyone using get_maintainer in a scripted way should go
through the effort of figuring out in advance who will
be a recipient.

>   Given that you've had to explicitly add --no-rolestats to
> your scripts, that seems like evidence in *favor* of making this change.

Probably not.

> As it stands now, the current default of --rolestats makes the obvious
> command line of
> git send-email --to-cmd='scripts/get_maintainer.pl' *.patch
> send broken emails that go to some maintainers but not all.  I think it
> makes sense to change the default so that the obvious usage becomes the
> correct one.

There were some discussions awhile back in 2010 about the
preferred defaults.

Perhaps you can read those discussions about why the default
is the way it is.

cheers, Joe

Re: [PATCH] scripts/get_maintainer.pl: Default to --no-rolestats when output not a terminal

[Josh Triplett]

On Fri, Aug 03, 2012 at 05:37:30PM -0700, Joe Perches wrote:
> On Fri, 2012-08-03 at 11:47 -0700, Josh Triplett wrote:
> > On Fri, Aug 03, 2012 at 11:33:21AM -0700, Joe Perches wrote:
> > > On Fri, 2012-08-03 at 11:27 -0700, Josh Triplett wrote:
> > > > scripts/get_maintainer.pl defaults to showing --rolestats, which
> > > > provides annotations explaining why each person or list might want to
> > > > know about a patch.  This works well for interactive use, but breaks
> > > > when used with git send-email's --to-cmd or --cc-cmd, resulting in
> > > > malformed email headers and mails sent to some but not all recipients.
> > > > 
> > > > To avoid the need to explicitly pass --no-rolestats for batch use,
> > > > enable --rolestats by default only when outputting to a terminal.
> > > 
> > > Hi Josh.
> > > 
> > > I think it's preferable to add --no-rolestats
> > > to the uses that need them.
> > 
> > Why?
> > 
> > > I have different scripts that I use for git send-email
> > > options --to-cmd and --cc-cmd
> > [...snip scripts...]
> > 
> > You've submitted enough patches that you've automated as much of the
> > process as you can; I don't think that makes the defaults less
> > error-prone.
> 
> I think the default use of the get_maintainer script is
> actually not scripted but interactive, where the user is
> just trying to figure out who the maintainer is.

I agree entirely; that's why I didn't change the default to always use
--no-rolestats, but rather to continue using --rolestats when
interactive and --no-rolestats when scripted.

> > As it stands now, the current default of --rolestats makes the obvious
> > command line of
> > git send-email --to-cmd='scripts/get_maintainer.pl' *.patch
> > send broken emails that go to some maintainers but not all.  I think it
> > makes sense to change the default so that the obvious usage becomes the
> > correct one.
> 
> There were some discussions awhile back in 2010 about the
> preferred defaults.
> 
> Perhaps you can read those discussions about why the default
> is the way it is.

I found commit 7e1863af1636b304a5f59aab6fb78d38e4079875, but that commit
does not serve the intended purpose.  Defaulting to --rolestats doesn't
make it "harder" to use get_maintainer.pl with git send-email, it just
makes it broken when used.  Meanwhile, the few discussions I see about
get_maintainer.pl just mention the problems caused by using --git, and
get_maintainer.pl has already improved to address those problems by not
using git commit signers for patches to files with active maintainers.

I don't see any value in making it intentionally harder to invoke
correctly while making it easier to invoke incorrectly.  Why not make it
actually work?

- Josh Triplett