Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] contrib: git-cpcover: copy cover letter
@ 2019-12-03 20:13 Michael S. Tsirkin
  2019-12-04  4:44 ` Jonathan Nieder
  2019-12-04  6:58 ` Denton Liu
  0 siblings, 2 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2019-12-03 20:13 UTC (permalink / raw)
  To: git

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] contrib: git-cpcover: copy cover letter
  2019-12-03 20:13 [PATCH v2] contrib: git-cpcover: copy cover letter Michael S. Tsirkin
@ 2019-12-04  4:44 ` Jonathan Nieder
  2019-12-04  5:18   ` Eric Sunshine
  2019-12-04  6:58 ` Denton Liu
  1 sibling, 1 reply; 7+ messages in thread
From: Jonathan Nieder @ 2019-12-04  4:44 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: git

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] contrib: git-cpcover: copy cover letter
  2019-12-04  4:44 ` Jonathan Nieder
@ 2019-12-04  5:18   ` Eric Sunshine
  2019-12-04 16:22     ` Junio C Hamano
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Sunshine @ 2019-12-04  5:18 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Michael S. Tsirkin, Git List

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] contrib: git-cpcover: copy cover letter
  2019-12-03 20:13 [PATCH v2] contrib: git-cpcover: copy cover letter Michael S. Tsirkin
  2019-12-04  4:44 ` Jonathan Nieder
@ 2019-12-04  6:58 ` Denton Liu
  1 sibling, 0 replies; 7+ messages in thread
From: Denton Liu @ 2019-12-04  6:58 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: git

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] contrib: git-cpcover: copy cover letter
  2019-12-04  5:18   ` Eric Sunshine
@ 2019-12-04 16:22     ` Junio C Hamano
  2019-12-04 16:32       ` Eric Sunshine
  0 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2019-12-04 16:22 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Jonathan Nieder, Michael S. Tsirkin, Git List

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] contrib: git-cpcover: copy cover letter
  2019-12-04 16:22     ` Junio C Hamano
@ 2019-12-04 16:32       ` Eric Sunshine
  2019-12-04 17:07         ` Junio C Hamano
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Sunshine @ 2019-12-04 16:32 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jonathan Nieder, Michael S. Tsirkin, Git List

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] contrib: git-cpcover: copy cover letter
  2019-12-04 16:32       ` Eric Sunshine
@ 2019-12-04 17:07         ` Junio C Hamano
  0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2019-12-04 17:07 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Jonathan Nieder, Michael S. Tsirkin, Git List

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 ;-)

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-03 20:13 [PATCH v2] contrib: git-cpcover: copy cover letter Michael S. Tsirkin
2019-12-04  4:44 ` Jonathan Nieder
2019-12-04  5:18   ` Eric Sunshine
2019-12-04 16:22     ` Junio C Hamano
2019-12-04 16:32       ` Eric Sunshine
2019-12-04 17:07         ` Junio C Hamano
2019-12-04  6:58 ` Denton Liu

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git