Re: [PATCH for-4.11 v3 1/1] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl

[Ian Jackson <ian.jackson@citrix.com>]

Thanks.  This is much better :-).  I have reviewed this for style,
obvious bugs, and the semantics in the doc comment.  I haven't tried
to follow the algorithm in detail, but I reckon it's probably OK.

I have reordered the patch (and hence the file) to make the grouping
of my comments make more sense.

Lars Kurth writes ("[PATCH for-4.11 v3 1/1] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl"):
> +  --inspatch (top|ccbody|cc---|none) | -p (top|ccbody|cc---|none)
> +    Insert email addresses into *.patch files according to the POLICY
> +    See section POLICY:
> +  --inscover (top|ccend|none) | -c (top|ccend|none)
> +    Insert email addresses into cover letteraccording to the POLICY
> +    See section PROCESSING POLICY:

I'm afraid that I don't understand which addresses are added where,
from the documentation.  In particular, what happens without --tags or
--tagscc ?  Also you should define `tag'; it has a lot of different
meanings, some subtly different, and it is not completely clear which
one you mean.

I think you should formally state the default behaviour.  Something
like:

  By default:
  * get_maintainer is called on each patch to find email addresses
    of maintainers/reviewers for that patch; these are added
    to the patch body near the CC section.
  * further email addresses are found in each patch's commit
    message tags (CC, signed-off-by, reviewed-by, etc.)
  * All of the above addresses are added to the CC mail headers
    of each patch
  * All of the above addresses are added to the CC mail headers
    of the cover letter

I suspect that what I have above is not the real behaviour.  You
should write what is true in that kind of style :-).

> +my $patch_prefix = "0"; # Use a 0, such that v* does not get picked up
> +                        # Obviously this will only work for series with
> +                        # < 999 patches, which should be fine

I don't understand the purpose of this:

> +if ($rerollcount > 0) {
> +    $patch_prefix = "v".$rerollcount;
> +}
...
> +my $pattern = $patch_dir.'/'.$patch_prefix.'*'.$patch_ext;
> +print "Then perform:\n".
> +      "git send-email -to xen-devel\@lists.xenproject.org ".
> +      $patch_dir.'/'.$patch_prefix."*.patch"."\n";

What files matching *.patch exist here that should not be processed ?
If the answer is none then $patch_prefix is redundant, I think ?


> +foreach my $file (@patches) {
> +    if ($file =~ /\Q$cover_letter\E/) {

I know you had me look at this over your shoulder and I said it was
right, but I think in fact this would match hypothetical files
     $patch_dir/0020-do-something-about-0000-cover-letter.patch

I think you need to expect a /.  So one of

  +    if ($file =~ /\/\Q$cover_letter\E/) {
  +    if ($file =~ m{/\Q$cover_letter\E}) {

> +print "Then perform:\n".
> +      "git send-email -to xen-devel\@lists.xenproject.org ".
> +      $patch_dir.'/'.$patch_prefix."*.patch"."\n";
> +
> +exit 0;
> +
> +my $readmailinglists = 0;
> +my @mailinglists = ();

This is a very curious structure.  These assignments are never
executed (but I guess the program works anyway).  I would recommend
moving the main program to the bottom of the file.

> +sub getmailinglists () {
> +   # Read mailing list from MAINTAINERS file and copy
> +   # a list of e-mail addresses to @mailinglists
> +    if (!$readmailinglists) {

I suggest you rename this variable $getmailinglists_done or
something.  As it is it is confusing because `read' might be the
present tense, but then the sense is wrong.

Also, you might find it better to use a structure like one of
      if ($getmailingslists_done) { return; }
      return if $getmailingslists_done;

> +        if (-e $maintainers) {
...
> +            print "Warning: Mailing lists will be treated as CC's\n";
> +        }
> +    # Don't try again, even if the MAINTAINERS file does not exist
> +    $readmailinglists = 1;
> +    # Remove any duplicates
> +    @mailinglists = uniq @mailinglists;
> +    }

Indentation here is misleading.  (But this will go away if you adopt
my suggestion above).

> +sub ismailinglist ($) {
> +    my ($check) = @_;
> +    # Get the mailing list information
> +    getmailinglists();
> +    # Do the check
> +    if ( grep { $_ eq $check} @mailinglists) {

Rather than uniq above, and then grep here, you could use a hash
%mailinglists.  That would be more idiomatic and also less code and
faster.  But as it is is tolerable.

> +sub getmaintainers ($$$) {
> +    my ($file, $rto, $rcc) = @_;
> +    my $get_maintainer_args = join " ", @get_maintainer_args;
> +    my $cmd = "$get_maintainer $get_maintainer_args <$file";
...
> +    open($fh, "-|", $cmd)
> +        or die "Failed to open '$cmd'\n";

You should use the array form of piped open, rather than this string
joining.  That way arguments containing spaces make their way through
correct.

> +    if (! -e $get_maintainer) {
> +        die "$tool: The tool requires $get_maintainer\n";

I still don't like this check.  What if the user specifies an
implementation of $get_maintainer which is on the PATH ?

> +    while(my $line = <$fh>) {
...
> +    }
> +    close $fh;

You need to check the errors here.  See the `perldoc -f close'.

> +        if ($tags & !$tagscc) {

You mean &&, not &.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel