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

[Lars Kurth <lars.kurth@citrix.com>]

On 08/05/2018, 11:48, "Ian Jackson" <ian.jackson@citrix.com> wrote:

    Lars Kurth writes ("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"):
    > On 04/05/2018, 18:43, "Ian Jackson" <ian.jackson@citrix.com> wrote:
    >     > +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*
    
    OMG.  It had not even occurred to me that anyone would do that.  But I
    think this workflow choice is independent of --reroll-count, which is
    mainly used to control patch subject lines.
    
    A workflow where *different* patch series are mingled in the same
    directory cannot be supported, because what when their reroll counts
    collide?  So these must be different versions (different rerolls) of
    the same series.
    
    I suggest the following approach:
    
    Test whether v<reroll-count>-0000-cover-letter.patch exists.  If it
    does, expect every patch to start with v<reroll-count>.  If it does
    not, expect simply 0000-cover-letter.patch to exist, and every patch
    to start with just the patch number.
    
    So I guess $patch_prefix would be '[0-9]' or "v${reroll_count}-[0-9]".

That makes sense
    
    >     > +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.
    
    For the purposes of documentation and informative messages like this
    one, I think you can assume workflow (b).  I think workflow (a) is not
    to be recommended.
    
    (Anyway, who keeps their g-f-p output directories ?  I don't...)

Sure: I think right now the documentation and messages don't cover this. So if we move to the approach you suggested, all should be fine.
    
    >     > +    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.
    
    You have misunderstood my remark.  I am not talking about the cwd.
    
    I mean, if the user says --get-maintainer=generic-get-maintaienr
    where /usr/bin/generic-get-maintaienr exists.

OK: simply removing the check would just do this

Lars 

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