[PATCH v2] contrib: git-cpcover: copy cover letter

[PATCH v2] contrib: git-cpcover: copy cover letter

[Michael S. Tsirkin]

My flow looks like this:
1. git format-patch -v<n> --cover-letter <params> -o <dir>
2. vi <dir>/v<n-1>-0000-cover-letter.patch <dir>/v<n>-0000-cover-letter.patch

copy subject and blurb, avoiding patchset stats

3. add changelog update blurb as appropriate

4. git send-email <dir>/v<n>-*

The following perl script automates step 2 above.  Hacked together
rather quickly, so I'm only proposing it for contrib for now.  If others
see the need, we can add docs, tests and move it to git proper.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Fixes from v1: support multi-line To/Cc headers.

Any feedback on this? Interest in taking this into contrib/ for now?

 contrib/git-cpcover | 84 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)
 create mode 100755 contrib/git-cpcover

diff --git a/contrib/git-cpcover b/contrib/git-cpcover
new file mode 100755
index 0000000000..fe7006a56d
--- /dev/null
+++ b/contrib/git-cpcover
@@ -0,0 +1,84 @@
+#!/usr/bin/perl -i
+
+use strict;
+
+die "Usage: ${0} <from> [<to>]" unless $#ARGV == 0 or $#ARGV == 1;
+
+my $ffrom = shift @ARGV;
+my @extraheaders = ();
+
+open(FROM, "<", $ffrom) || die "Can not open $ffrom";
+
+my @from = ();
+while (<FROM>) {
+	push @from, $_;
+}
+
+close(FROM) || die "error closing $ffrom";
+
+#get subject
+my $subj;
+my $bodyi;
+my $lastheader="";
+for (my $i = 0; $i <= $#from; $i++) {
+	$_ = $from[$i];
+	#print STDERR "<$line>\n";
+	if (not defined ($subj) and s/^Subject: \[[^]]+\] //) {
+		$subj = $_;
+		chomp $subj;
+	}
+	if (m/^([A-Za-z0-9-_]*:)/) {
+		$lastheader = $1;
+	}
+	if (m/^(To|Cc):/ or (m/^\s/ and $lastheader =~ m/^(To|Cc):/)) {
+		push @extraheaders, $from[$i];
+	}
+	if (defined ($subj) and m/^$/) {
+		$bodyi = $i + 1;
+		last;
+	}
+}
+
+die "No subject found in $ffrom" unless defined($subj);
+
+die "No body found in $ffrom" unless defined($bodyi);
+
+my $bodyl;
+my $statb;
+my $state;
+for (my $i = $#from; $i >= $bodyi; $i--) {
+	$_ = $from[$i];
+	$statb = $i if m/ [0-9]+ files changed, [0-9]+ insertions\(\+\), [0-9]+ deletions\(-\)/;
+	next unless defined($statb);
+	$state = $i if m/^$/;
+	next unless defined($state);
+	next if m/^$/;
+	next if m/^  [^ ]/;
+	next if m/\([0-9]+\):$/;
+	$bodyl = $i;
+	last;
+}
+
+die "No body found in $ffrom" unless defined($bodyl);
+
+#print STDERR $bodyi, "-", $bodyl, "\n";
+my $blurb = join("", @from[$bodyi..$bodyl]);
+
+my $gotsubj = 0;
+my $gotblurb = 0;
+my $gotendofheaders = 0;
+while (<>) {
+	if (not $gotsubj and
+	    s/\*\*\* SUBJECT HERE \*\*\*/$subj/) {
+		$gotsubj = 1;
+	}
+	if (not $gotblurb and
+	    s/\*\*\* BLURB HERE \*\*\*/$blurb/) {
+		$gotblurb = 1;
+	}
+	if (not $gotendofheaders and m/^$/) {
+		print join("", @extraheaders);
+		$gotendofheaders = 1;
+	}
+	print $_;
+}
-- 
MST

Re: [PATCH v2] contrib: git-cpcover: copy cover letter

[Jonathan Nieder]

Hi,

Michael S. Tsirkin wrote:

> My flow looks like this:
> 1. git format-patch -v<n> --cover-letter <params> -o <dir>
> 2. vi <dir>/v<n-1>-0000-cover-letter.patch <dir>/v<n>-0000-cover-letter.patch
>
> copy subject and blurb, avoiding patchset stats
>
> 3. add changelog update blurb as appropriate
>
> 4. git send-email <dir>/v<n>-*
>
> The following perl script automates step 2 above.

Neat.  I wonder, should "git format-patch" learn an option for this?
E.g.

	git format-patch -v<n> --cover-letter \
		--last-cover-letter=<dir>/v<n-1>-0000-cover-letter.patch \
		-o <dir>

What would your ideal interface for this flow look like?

[...]
> Any feedback on this? Interest in taking this into contrib/ for now?

I don't know what Junio's preferences are for new contrib/
contributions, but I kind of like it.  If putting it in contrib/, my
main advice would be to put it in a subdirectory there with a README.
That way, we have a good place to document what it was replaced by
once it has graduated to a standard format-patch feature.

Thanks and hope that helps,
Jonathan

Re: [PATCH v2] contrib: git-cpcover: copy cover letter

[Eric Sunshine]

On Tue, Dec 3, 2019 at 11:45 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
> Michael S. Tsirkin wrote:
> > My flow looks like this:
> > 2. vi <dir>/v<n-1>-0000-cover-letter.patch <dir>/v<n>-0000-cover-letter.patch
> > copy subject and blurb, avoiding patchset stats
> > 3. add changelog update blurb as appropriate
> >
> > The following perl script automates step 2 above.
>
> Neat.  I wonder, should "git format-patch" learn an option for this?
>         git format-patch -v<n> --cover-letter \
>                 --last-cover-letter=<dir>/v<n-1>-0000-cover-letter.patch \
>                 -o <dir>

That was my first thought, as well, although, as this has similar
purpose to the new git-format-patch --cover-from-description= option,
perhaps a more suitable name might be --copy-cover-from= or something?

I could even imagine a new option -V<n> which has the combined effect
of setting the re-roll count (like -v) and automagically copying the
cover letter material from cover letter v<n-1> located in <dir>.

Re: [PATCH v2] contrib: git-cpcover: copy cover letter

[Denton Liu]

Hi Michael,

As others have been talking about in a sibling thread, I'd love to see
something like this incorporated into format-patch itself. I considered
doing something like this myself but my workflow ended up becoming "good
enough" with --cover-from-description that I never bothered to look into
it further.

On Tue, Dec 03, 2019 at 03:13:27PM -0500, Michael S. Tsirkin wrote:
> My flow looks like this:
> 1. git format-patch -v<n> --cover-letter <params> -o <dir>
> 2. vi <dir>/v<n-1>-0000-cover-letter.patch <dir>/v<n>-0000-cover-letter.patch
> 
> copy subject and blurb, avoiding patchset stats
> 
> 3. add changelog update blurb as appropriate
> 
> 4. git send-email <dir>/v<n>-*
> 
> The following perl script automates step 2 above.  Hacked together
> rather quickly, so I'm only proposing it for contrib for now.  If others
> see the need, we can add docs, tests and move it to git proper.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> Fixes from v1: support multi-line To/Cc headers.
> 
> Any feedback on this? Interest in taking this into contrib/ for now?

I took a brief look at it and I have one small suggestion. Would it be
possible to grab the last Message-Id and use it to generate the
In-Reply-To header using this?

Thanks,

Denton

Re: [PATCH v2] contrib: git-cpcover: copy cover letter

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Dec 3, 2019 at 11:45 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Michael S. Tsirkin wrote:
>> > My flow looks like this:
>> > 2. vi <dir>/v<n-1>-0000-cover-letter.patch <dir>/v<n>-0000-cover-letter.patch
>> > copy subject and blurb, avoiding patchset stats
>> > 3. add changelog update blurb as appropriate
>> >
>> > The following perl script automates step 2 above.
>>
>> Neat.  I wonder, should "git format-patch" learn an option for this?
>>         git format-patch -v<n> --cover-letter \
>>                 --last-cover-letter=<dir>/v<n-1>-0000-cover-letter.patch \
>>                 -o <dir>
>
> That was my first thought, as well, although, as this has similar
> purpose to the new git-format-patch --cover-from-description= option,
> perhaps a more suitable name might be --copy-cover-from= or something?
>
> I could even imagine a new option -V<n> which has the combined effect
> of setting the re-roll count (like -v) and automagically copying the
> cover letter material from cover letter v<n-1> located in <dir>.

I actually looked into doing something similar but without any new
option (i.e. unconditionally --cover-letter with -v<n> would check
for v<n-1>-0000-cover.letter and does the right thing) some time
ago.  I do not recall why I gave up (not that I tried very hard),
but IIRC, the current reroll-count was not passed down in the
callchain to make_cover_letter() to do this.

But I think that was even before we integrated the range-diff stuff,
which does seem to use the "given we are doing <n>, let's compare
with <n-1>" thing, so perhaps it is not too difficult.

I am just saying that I think the change would not have to be opt-in,
but can be unconditionally made, simply because replacing the BLURB
HERE placeholder with *anything* written by human user previously is
a 100% improvement ;-)

Thanks.

Re: [PATCH v2] contrib: git-cpcover: copy cover letter

[Eric Sunshine]

On Wed, Dec 4, 2019 at 11:23 AM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > I could even imagine a new option -V<n> which has the combined effect
> > of setting the re-roll count (like -v) and automagically copying the
> > cover letter material from cover letter v<n-1> located in <dir>.
>
> I actually looked into doing something similar but without any new
> option (i.e. unconditionally --cover-letter with -v<n> would check
> for v<n-1>-0000-cover.letter and does the right thing) some time
> ago.

Yes, I like that better than a new option, and wanted to suggest it as
well, however... (see below)

> But I think that was even before we integrated the range-diff stuff,
> which does seem to use the "given we are doing <n>, let's compare
> with <n-1>" thing, so perhaps it is not too difficult.

Yup.

> I am just saying that I think the change would not have to be opt-in,
> but can be unconditionally made, simply because replacing the BLURB
> HERE placeholder with *anything* written by human user previously is
> a 100% improvement ;-)

I had started writing the same in my previous reply but then realized
that it could break existing tooling which uses -v and --cover-letter
together and which searches for the well-known BLURB HERE placeholder
to replace it automatically. If I'm wrong about possibly breaking
existing tooling, then I'd also vote for this behavior kicking in
automatically with -v and --cover-letter specified together.

Re: [PATCH v2] contrib: git-cpcover: copy cover letter

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Dec 4, 2019 at 11:23 AM Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>> > I could even imagine a new option -V<n> which has the combined effect
>> > of setting the re-roll count (like -v) and automagically copying the
>> > cover letter material from cover letter v<n-1> located in <dir>.
>>
>> I actually looked into doing something similar but without any new
>> option (i.e. unconditionally --cover-letter with -v<n> would check
>> for v<n-1>-0000-cover.letter and does the right thing) some time
>> ago.
>
> Yes, I like that better than a new option, and wanted to suggest it as
> well, however... (see below)
>
>> But I think that was even before we integrated the range-diff stuff,
>> which does seem to use the "given we are doing <n>, let's compare
>> with <n-1>" thing, so perhaps it is not too difficult.
>
> Yup.
>
>> I am just saying that I think the change would not have to be opt-in,
>> but can be unconditionally made, simply because replacing the BLURB
>> HERE placeholder with *anything* written by human user previously is
>> a 100% improvement ;-)
>
> I had started writing the same in my previous reply but then realized
> that it could break existing tooling which uses -v and --cover-letter
> together and which searches for the well-known BLURB HERE placeholder
> to replace it automatically. If I'm wrong about possibly breaking
> existing tooling, then I'd also vote for this behavior kicking in
> automatically with -v and --cover-letter specified together.

Well, they can now disable that "copy over the material from the
previous" step, which is good.  I actually think we should add the
well-known BLURB HERE string *after* populating the new version of
the coer letter with the materials from old one to *force* users to
proofread and adjust it to the updated reality, so if that happens,
then the existing tooling would also work as before ;-)

Re: [PATCH v2] contrib: git-cpcover: copy cover letter

[Michael S. Tsirkin]

On Tue, Dec 03, 2019 at 08:44:49PM -0800, Jonathan Nieder wrote:
> Hi,
> 
> Michael S. Tsirkin wrote:
> 
> > My flow looks like this:
> > 1. git format-patch -v<n> --cover-letter <params> -o <dir>
> > 2. vi <dir>/v<n-1>-0000-cover-letter.patch <dir>/v<n>-0000-cover-letter.patch
> >
> > copy subject and blurb, avoiding patchset stats
> >
> > 3. add changelog update blurb as appropriate
> >
> > 4. git send-email <dir>/v<n>-*
> >
> > The following perl script automates step 2 above.
> 
> Neat.  I wonder, should "git format-patch" learn an option for this?
> E.g.
> 
> 	git format-patch -v<n> --cover-letter \
> 		--last-cover-letter=<dir>/v<n-1>-0000-cover-letter.patch \
> 		-o <dir>
> 
> What would your ideal interface for this flow look like?

I use it in several ways
- new version - generate a new version from previous one
	git format-patch -o patches/series ....
	git cpcover patches/series/saved-cover-letter.patch patches/series/vN-cover-letter.patch

	So ideally it would automatically pick up vN-1 cover letter if it's there.

- update - I generate patches but don't post yet.
	write a cover letter with e.g. an explanation, decide
	to make some changes before posting. Then:
	cp patches/series/vN-cover-letter.patch patches/series/saved-cover-letter.patch
	git format-patch -o patches/series ....
	git cpcover patches/series/saved-cover-letter.patch patches/series/vN-cover-letter.patch


	So ideally if cover letter already exists, ask whether to copy it.
	or at least whether it's ok to over-write it...

- series history
	I start with a non-versioned cover letter,
        and maintain it in a directory:
	cp patches/series/0000-cover-letter.patch patches/series/cover-letter.patch
	This is where
	I then put notes about design changes, list questions
	to be answered, add people who contributed
	to the discussion and should be Cc'd on new versions
	Then each time I do
	git format-patch -o patches/series ....
	git cpcover patches/series/cover-letter.patch patches/series/vN-cover-letter.patch
- public-inbox
	get series from public inbox (from someone else
	or myself) and work on it

	Ideally if cover-letter without a version exists, pick it up.

During all of this, it's important to pick up description, subject, to/cc.

Also one thing I need to remember to do is update changelog.

If there's a standard way to document version changes, ideally it will
be possible to check whether latest version changes have been
documented, and print a reminder if not.


> [...]
> > Any feedback on this? Interest in taking this into contrib/ for now?
> 
> I don't know what Junio's preferences are for new contrib/
> contributions, but I kind of like it.  If putting it in contrib/, my
> main advice would be to put it in a subdirectory there with a README.
> That way, we have a good place to document what it was replaced by
> once it has graduated to a standard format-patch feature.
> 
> Thanks and hope that helps,
> Jonathan

OK so contrib/format-patch/ ?

-- 
MST