[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [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




On 04/05/2018, 18:43, "Ian Jackson" <ian.jackson@xxxxxxxxxx> wrote:

    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.

Is that because of the way I structured it? The actual behaviour
Is stated under PROCESSING POLICY. Sure: I can explain what is read
and what is done with the data.

    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 :-).

Sure. That makes things a lot clearer.
    
    > +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:
 
This is a bit of a hack! 

There are several different usage patterns for g-f-p when working on a series,
which result in $patch_dir being used differently. In one case
a) the user stores patches for multiple series in $patch_dir
Thus, $patchdir may contain files starting with 0000*, 0001*, ... v1-000*, 
v2-000*
I have directories that contain entries generated by 

Case a1)
g-f-p  ...
g-f-p  --reroll-count=2 ...
Etc.

and 
Case a2) g-f-p  --reroll-count=1 ...
Etc.

b) the user stores patches in separate directories, aka one directory per g-f-p

What I was trying to do here is to use $patch_prefix to select what to process
in $patchdir. The problem is that in case a1, when g-f-p was called with no 
--reroll-count, I need to select entries 0000*, 0001*, ... as otherwise the 
entire
directory is processed.

The only way to identify these is via 0*.patch. But I may have missed something.

    > +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 ?

Well, it depends. G-s-m will send everything in $patch_dir.
I have not checked whether it ignores backups (~ .bak), but I assume it does.
In any case, for scenario a1) and a2) I do need to select which files
to select in g-s-e.
    
    > +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}) {
    
Sure

    > +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.

Sure
    
    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.

Will look into this.
    
    > +    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 ?

The way get_maintainer.pl works is that it has to be called in the root
directory of the Xen and Linux trees. There are some checks in the
tool that throw an error when you call it from another location.

The relevant code in get_maintainer.pl is

 268 if (!top_of_tree($xen_path)) {
 269     die "$P: The current directory does not appear to be "
 270         . "a Xen source tree.\n";
 271 }

Right now, $get_maintainer is not really configurable. So this
is not an issue now, but could become one in future. 

I think for now, I will leave this as-is, but add a comment.
    
    > +    while(my $line = <$fh>) {
    ...
    > +    }
    > +    close $fh;
    
    You need to check the errors here.  See the `perldoc -f close'.

OK
    
    > +        if ($tags & !$tagscc) {
    
    You mean &&, not &.

Indeed

Thanks for the review

Lars
 

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.