|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |