Message ID | 20191203201233.661696-1-mst@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] contrib: git-cpcover: copy cover letter | expand |
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
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>.
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
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.
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.
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 ;-)
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/ ?
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 $_; +}
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