[PATCH] contrib: git-cpcover: copy cover letter
My flow looks like this:
1. git format-patch -v --cover-letter -o
2. vi /v--cover-letter.patch /v--cover-letter.patch
copy subject and blurb, avoiding patchset stats
3. add changelog update blurb as appropriate
4. git send-email /v-*
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
---
contrib/git-cpcover | 80 +
1 file changed, 80 insertions(+)
create mode 100755 contrib/git-cpcover
diff --git a/contrib/git-cpcover b/contrib/git-cpcover
new file mode 100755
index 00..d80bd141ba
--- /dev/null
+++ b/contrib/git-cpcover
@@ -0,0 +1,80 @@
+#!/usr/bin/perl -i
+
+use strict;
+
+die "Usage: ${0} []" unless $#ARGV == 0 or $#ARGV == 1;
+
+my $ffrom = shift @ARGV;
+my @extraheaders = ();
+
+open(FROM, "<", $ffrom) || die "Can not open $ffrom";
+
+my @from = ();
+while () {
+ push @from, $_;
+}
+
+close(FROM) || die "error closing $ffrom";
+
+#get subject
+my $subj;
+my $bodyi;
+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/^(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: cover letter cc's [was: [PATCH 60/67] hw/s390x: add include directory headers]
On Fri, May 04, 2018 at 08:07:53AM -0500, Eric Blake wrote: > [adding a cross-post to the git mailing list] > > On 05/04/2018 02:10 AM, Cornelia Huck wrote: > > On Thu, 3 May 2018 22:51:40 +0300 > > "Michael S. Tsirkin" wrote: > > > > > This way they are easier to find using standard rules. > > > > > > Signed-off-by: Michael S. Tsirkin > > > --- > ... > > > [Goes to find cover letter to figure out what this is all about. > > *Please*, cc: people on the cover letter so they can see immediately > > what this is trying to do!] > > Is there an EASY way to make 'git format-patch --cover-letter $commitid' > (and git send-email, by extension) automatically search for all cc's any any > of the N/M patches, and auto-cc ALL of those recipients on the 0/N cover > letter? And if that is not something easily built into git format-patch > directly, is it something that can easily be added to sendemail.cccmd? This > is not the first time that someone has complained that automatic cc's are > not sending the cover letter context to a particular maintainer interested > (and auto-cc'd) in only a subset of an overall series. > > On the other hand, cc'ing all recipients for a largely mechanical patch > series that was split into 67 parts, in part because it touches so many > different maintainers' areas, may make the cover letter have so many > recipients that various mail gateways start rejecting it as potential spam. I do this sometimes (pipe to this script): grep -e ^Signed-off-by -e ^Acked -e ^Reported -e ^Tested -e ^Cc | sed 's/.*.*//'|sort | uniq | sed 's/^/Cc: /' > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org
Re: storing cover letter of a patch series?
On Fri, Aug 05, 2016 at 08:39:58AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" writes:
>
> > On Thu, Sep 10, 2015 at 11:39:49AM -0700, Junio C Hamano wrote:
> >> The problem with "empty commit trick" is that it is a commit whose
> >> sole purpose is to describe the series, and its presence makes it
> >> clear where the series ends, but the topology does not tell where
> >> the series begins, so it is an unsatisifactory half-measure.
> >
> > Actually, when using topic branches the series always ends at head, so
> > it's better to keep the empty commit where series begins.
>
> But that would mean that you would need to destroy and recreate more
> commits than you would need to. If you have a five-commit series
> (with the bottom "description" one, you would have six commits) and
> you are already happy with the bottom two but want to update the
> third one, you wuld have to "rebase -i" all six of them, reword the
> bottom "description" to adjust it to describe the new version of the
> third one _before_ you even do the actual update of the third one.
>
> That somehow feels backwards, and that backward-ness comes from the
> fact that you abused a single-parent commit for the purpose it is
> not meant to be used (i.e. they are to describe individual changes),
> because you did not find a better existing mechanism (and I suspect
> there isn't any, in which case the solution is to invent one, not
> abusing an existing mechanism that is not suited for it).
A flag that marks a commit "beginning of series" then?
> If this were part of a workflow like this, I would understand it:
>
> * Build a N-commit series on a topic.
>
> * You keep a "local integration testing" branch ("lit"), forked
>from a mainline and updated _every time_ you do something to your
>topics. You may or may not publish this branch. This is the
>aggregation of what you locally have done, a convenient place to
>test individual topics together before they get published.
This seems to assume topic branches. I know you use them,
but not overyone does, I don't.
> * A new topic, when you merge it to the "lit" branch, you describe
>the cover as the merge commit message.
>
> * When you updated an existing topic, you tell a tool like "rebase
>-i -p" to recreate "lit" branch on top of the mainline. This
>would give you an opportunity to update the cover.
Combining patchsets might need conflict resolution,
redoing this each time might be a lot of work.
> Now the tool support for the last one is the missing piece. In
> addition to what "rebase -i -p" would, it at least need to
> automatically figure out which topics have been updated, so that
> their merge commit log messages need to be given in the editor to
> update, while carrying over the merge log message for other topics
> intact (by default).
>
> With that, you should also be able to teach "format-patch --cover"
> to take these merge messages on "lit" into account when it creates
> the cover letter.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: storing cover letter of a patch series?
On Thu, Sep 10, 2015 at 11:39:49AM -0700, Junio C Hamano wrote: > The problem with "empty commit trick" is that it is a commit whose > sole purpose is to describe the series, and its presence makes it > clear where the series ends, but the topology does not tell where > the series begins, so it is an unsatisifactory half-measure. Actually, when using topic branches the series always ends at head, so it's better to keep the empty commit where series begins. This was actually suggested by Philip Oakley on this thread but I'm not sure it was noticed as it was part of a bigger email. It also maps much better to git am uses - you apply patch 0/N first to create the empty commit, then the rest of the patches. This does mean you need to use git rebase to edit that cover commit, but maybe that is not a big deal, and git rebase could learn --cover to find and edit that. -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: storing cover letter of a patch series?
On Thu, Sep 10, 2015 at 02:03:48PM -0700, Jacob Keller wrote: > On Thu, Sep 10, 2015 at 1:09 PM, Philip Oakley wrote: > > From: "Jacob Keller" > >> > >> On Thu, Sep 10, 2015 at 11:44 AM, Junio C Hamano > >> wrote: > >>> > >>> Jacob Keller writes: > >>> > I hadn't thought of separating the cover letter from git-send-email. > That would be suitable for me. > >>> > >>> > >>> Yeah, I said this number of times over time, and I said it once > >>> recently in another thread, but I think it was a mistake to allow > >>> git-send-email to drive format-patch. It may appear that it will > >>> make things convenient in the perfect world where no user makes > >>> mistakes, but people are not perfect in real life. Expecting them > >>> to be is being naive. > >>> > >> > >> Yep. I didn't even know cover-letter was an option of format-patch > >> only thought it was in send-email. > >> > > Actually, the one feature I'd like (I think) is to be able to join together > > the empty commit mechanism and the cover letter mechanism within format > > patch so that: > > > > * the empty commit message would detected and automatically become the [0/N] > > in the patch series (without need to say --cover-letter) > > > > * the cover letter would still have some 'template' markings to say "*** > > insert what's changed here***" or smilar (with option to exclude them). > > > > That way, when starting a series / branch, the first item would be to add > > the explanatory 'empty commit' that states the requirements of what one > > hopes to achieve (a key cover letter content), which is then followed by > > commits that move toward that goal. > > > > The series can then be rebased as the user develops the code, and that cover > > note can be edited as required during the rebase. > > > > When it comes time to show it to the list, the format patch will *know* from > > the empty commit that it is the [0/N] cover letter and (perhaps -option) add > > the appropriate markers ready for editing. And perhaps git am could learn an option to apply 0/N as a cover commit. > > The user edits the cover letter with the extra 'what's changed' / interdiff > > / whatever, and sends. sendmail barfs if the user hasn't edited the markers. > > > > This could also work with the sendmail patch formating (though I've never > > used that workflow) as now the cover letter becomes automatic for the > > upstream. > > > > Philip > > If there was a way to store this empty commit message tagged as "cover > letter" that could work well, though generally I prefer the > non-fast-forward merges as this shows you where the series ended *and* > began. It's somewhat confusing to newer users.. and this doesn't get > rebased very well either. > > Some way to indicate a particular "empty" commit is actually a cover > letter seems easy enough. This seems like the way that I was thinking. Start the subject with "cover! "? I have a patch that teaches git-rebase to keep empty commits where the subject has a given prefix, that might be helpful there. > Using "edit description" of git-branch seems also to be pretty > effective for this, even if it doesn't get shared across remotes. (not > really a necessary feature for what I do). > > But having some way to indicate "cover letter" which gets used as the > beginning of a log message when doing a particular "merge > --tip-as-cover" or something like Junio suggested above seems like the > nicest approach. > > Regards, > Jake > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git-fixup: automatically create a fixup commit
Sometimes I get a broken patch, apply it and then need to fix it up. git commit --fixup is perfect for this, but makes me look up the commit that created the breakage manually. git-fixup is a tool to speed this up. Several heuristics would be reasonable for locating the problematic commit: 1. look up the last commit that touched the file(s) affected by the fixup 2. look up the last commit that touched the line(s) affected by the fixup this implements the first heuristic. Signed-off-by: Michael S. Tsirkin --- contrib/git-fixup | 8 1 file changed, 8 insertions(+) create mode 100755 contrib/git-fixup diff --git a/contrib/git-fixup b/contrib/git-fixup new file mode 100755 index 000..79f4e34 --- /dev/null +++ b/contrib/git-fixup @@ -0,0 +1,8 @@ +if + git diff --cached --quiet +then + echo "Nothing to commit!" + exit 1 +else + git commit --fixup=$(git log --pretty=format:%H -1 $(git diff --cached --name-only)) "$@" +fi -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] xdiff: implement empty line chunk heuristic
On Tue, Apr 19, 2016 at 04:07:35PM -0700, Junio C Hamano wrote: > Jacob Keller writes: > > > On Tue, Apr 19, 2016 at 10:06 AM, Jeff King wrote: > >> On Tue, Apr 19, 2016 at 08:17:38AM -0700, Stefan Beller wrote: > >> > >>> On Mon, Apr 18, 2016 at 10:03 PM, Jeff King wrote: > >>> > >>> > I guess this will invalidate old patch-ids, but there's not much to be > >>> > done about that. > >>> > >>> What do you mean by that? (What consequences do you imagine?) > >>> I think diffs with any kind of heuristic can still be applied, no? > >> > >> I mean that if you save any old patch-ids from "git patch-id", they > >> won't match up when compared with new versions of git. We can probably > >> ignore it, though. This isn't the first time that patch-ids might have > >> changed, and I think the advice is already that one should not count on > >> them to be stable in the long term. > >> > >> -Peff > > > > Plus they'll be stable within a version of Git, it's only recorded > > patch ids that change, which hopefully isn't done very much if at all. > > > > Thanks, > > Jake > > Some people, like those who did things like 30e12b92 (patch-id: make > it stable against hunk reordering, 2014-04-27), _may_ care. > FWIW IIRC what that commit is about is ability to reorder the chunks in a patch without changing patch-id. Not about keeping id stable across git revisions. -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] rebase -i: add ack action
On Tue, Apr 12, 2016 at 09:07:30AM -0700, Junio C Hamano wrote: > Matthieu Moy writes: > > > "Michael S. Tsirkin" writes: > > > >> Interesting. An empty commit would be rather easy to create on any > >> branch, not just the current one, using git-commit-tree. > > > > This "modify a branch without checking-it out" makes me think of "git > > notes". It may make sense to teach "git rebase -i" to look for notes in > > rebased commits and append them to the commit message when applying. > > Just an idea, not necessarily a good one ;-). > > Yeah that may actually fly well, as a note is designed to attach to > an exact commit, not to a branch, so that feels more natural. We'd have to invent a way to show that in the rebase -i output though. > As to the "use commit-tree", well, personally I am not interested in > a solution that can work well in my workflow ONLY if I further script > it. That's half-solution and unless that half is done very well, > I'd rather do a full solution better. Absolutely. But that's not what I meant. I will add a flag to git-ack to select a branch and use commit-tree to put the ack commit there *internally*. Would this do everything you need? How do you select a branch? Automatically or do you remember the mapping from topic to branch name? > Note: this is a continuation of "I personally would not use > it, even though other people might" discussion. > > I was also wondering if I should just script around filter-branch, > if all I am futzing with is the data in the trailer block, doing the > munging of the trailer block with interpret-trailers, naturally. > > In any case, a recent occasion that I had to do something related to > this topic may illustrate the boundary of requirements: > > Two developers, Michael and David, are involved. David sends a > 24-patch series, some of which were written by Michael and > others by David. The in-body "From:" lines are set right and > the resulting patches record authorship correctly. > > Michael reminds David that patches authored by Michael still > need to be signed-off by David. David sends a single message > "those by Michael in this series are signed off by me". > > Michael also says that he reviewed all patches authored by > David, i.e. "Add Acked-by Michael to all patches in this series > authored by David". > > Now this is an extreme case where a simple "OK I received an > e-mailed Ack, so I can rely on the subject line matching to mark it > to be squashed" approach will never work (i.e. if we were automating > it I'd expect that the script in DSL to the automation machinery to > take at last as many (conceptual) bits as the above problem > description). So here's how I solve the second part for now - that is very common: I expect Michael to write something like For series: Acked-by: Michael S. Tsirkin then I run git ack -s to put the signature in a file .git/ACKS. (git ack -s is just writing acks into .git/ACKS so if the email format is wrong I just edit it manually). And then I tag the series in email and run git ack -r to add the ack tag. For first part, that is less common but also happens (for example I get "for patches 1,7 and 23 in series: ACK") - I would do git ack -s to store David's signoff, then tag just messages by David (probably just using limit ~b From:\ David in mutt) and pipe them to git ack -r. Does this sound user-friendly enough? What would you do differently? -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] rebase -i: add ack action
On Mon, Apr 11, 2016 at 10:03:39PM +0200, Matthieu Moy wrote: > "Michael S. Tsirkin" writes: > > > On Mon, Apr 11, 2016 at 12:48:22PM -0700, Junio C Hamano wrote: > >> "Michael S. Tsirkin" writes: > >> > >> > Repost, sorry about the noise. > >> > > >> > On Mon, Apr 11, 2016 at 05:36:45PM +0200, Johannes Schindelin wrote: > >> >> Hi Michael, > >> >> > >> >> On Mon, 11 Apr 2016, Michael S. Tsirkin wrote: > >> >> > >> >> > So far I only see examples of adding footers. If that's all we can > >> >> > think > >> >> > up, why code in all this genericity? > >> >> > >> >> Because as far as I can see, the only benefitor of your patches would be > >> >> you. > >> >> > >> >> Ciao, > >> >> Johannes > >> > > >> > This seems unlikely. Just merging the patches won't benefit me directly > >> > - I have maintained them in my tree for a couple of years now with very > >> > little effort. For sure, I could benefit if they get merged and then > >> > someone improves them further - that was the point of posting them - but > >> > then I'm not the only benefitor. > >> > > >> > The workflow including getting acks for patches by email is not handled > >> > well by upstream git right now. It would surprise me if no one uses it > >> > if it's upstream, as you seem to suggest. But maybe most people moved > >> > on and just do pull requests instead. > >> > >> I doubt I would use this in its current form myself. > >> > >> Patch series I receive are all queued on their own separate topic > >> branches, and having to switch branches only to create a fake empty > >> commit to record received Acked-by and Reviewed-by is a chore that > >> serves only half of what needs to be done. > > > > Interesting. An empty commit would be rather easy to create on any > > branch, not just the current one, using git-commit-tree. > > This "modify a branch without checking-it out" makes me think of "git > notes". It may make sense to teach "git rebase -i" to look for notes in > rebased commits and append them to the commit message when applying. > Just an idea, not necessarily a good one ;-). Two things making it harder - machinery to look for commits is part of git rebase anyway - notes are expected to come after --- at the moment > > Does it sounds interesting if I teach > > git ack to get an active branch as a parameter? > > I think "ack" is not a good name for this feature: you use it to append > "Acked-by", but it can be used to append any trailer (for example, > Reviewed-by: would make complete sense too). Yes - I use it to append all trailers. > I think using a better name > would help the discussion (to remove the "it's my use-case" biais). > Perhaps "append"? Or "trailer". > -- > Matthieu Moy > http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] rebase -i: add ack action
On Mon, Apr 11, 2016 at 12:48:22PM -0700, Junio C Hamano wrote: > "Michael S. Tsirkin" writes: > > > Repost, sorry about the noise. > > > > On Mon, Apr 11, 2016 at 05:36:45PM +0200, Johannes Schindelin wrote: > >> Hi Michael, > >> > >> On Mon, 11 Apr 2016, Michael S. Tsirkin wrote: > >> > >> > So far I only see examples of adding footers. If that's all we can think > >> > up, why code in all this genericity? > >> > >> Because as far as I can see, the only benefitor of your patches would be > >> you. > >> > >> Ciao, > >> Johannes > > > > This seems unlikely. Just merging the patches won't benefit me directly > > - I have maintained them in my tree for a couple of years now with very > > little effort. For sure, I could benefit if they get merged and then > > someone improves them further - that was the point of posting them - but > > then I'm not the only benefitor. > > > > The workflow including getting acks for patches by email is not handled > > well by upstream git right now. It would surprise me if no one uses it > > if it's upstream, as you seem to suggest. But maybe most people moved > > on and just do pull requests instead. > > I doubt I would use this in its current form myself. > > Patch series I receive are all queued on their own separate topic > branches, and having to switch branches only to create a fake empty > commit to record received Acked-by and Reviewed-by is a chore that > serves only half of what needs to be done. Interesting. An empty commit would be rather easy to create on any branch, not just the current one, using git-commit-tree. Does it sounds interesting if I teach git ack to get an active branch as a parameter? > Once I decide to switch > back to the topic branch after receiving Acked-by and Reviewed-by, > I'd rather "rebase -i" to directly record them at that point, with > "reword". > > If the "trailers" stuff is packaged into an easier-to-use format to > use with "git commit --amend", I may use that together with "exec" > to automatically add these while doing so, but again, I do not see > any need for fake empty commits out of received e-mails in the > resulting workflow. > > That does not at all mean nobody other than Michael would use it, > though. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] rebase -i: add ack action
Repost, sorry about the noise. On Mon, Apr 11, 2016 at 05:36:45PM +0200, Johannes Schindelin wrote: > Hi Michael, > > On Mon, 11 Apr 2016, Michael S. Tsirkin wrote: > > > So far I only see examples of adding footers. If that's all we can think > > up, why code in all this genericity? > > Because as far as I can see, the only benefitor of your patches would be > you. > > Ciao, > Johannes This seems unlikely. Just merging the patches won't benefit me directly - I have maintained them in my tree for a couple of years now with very little effort. For sure, I could benefit if they get merged and then someone improves them further - that was the point of posting them - but then I'm not the only benefitor. The workflow including getting acks for patches by email is not handled well by upstream git right now. It would surprise me if no one uses it if it's upstream, as you seem to suggest. But maybe most people moved on and just do pull requests instead. -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[no subject]
Cc junio Bcc: Subject: Re: [PATCH 1/4] rebase -i: add ack action Message-ID: <20160411184535-mutt-send-email-...@redhat.com> Reply-To: In-Reply-To: On Mon, Apr 11, 2016 at 05:36:45PM +0200, Johannes Schindelin wrote: > Hi Michael, > > On Mon, 11 Apr 2016, Michael S. Tsirkin wrote: > > > So far I only see examples of adding footers. If that's all we can think > > up, why code in all this genericity? > > Because as far as I can see, the only benefitor of your patches would be > you. > > Ciao, > Johannes This seems unlikely. Just merging the patches won't benefit me directly - I have maintained them in my tree for a couple of years now with very little effort. For sure, I could benefit if they get merged and then someone improves them further - that was the point of posting them - but then I'm not the only benefitor. The workflow including getting acks for patches by email is not handled well by upstream git right now. It would surprise me if no one uses it if it's upstream, as you seem to suggest. But maybe most people moved on and just do pull requests instead. -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] rebase -i: add ack action
On Mon, Apr 11, 2016 at 01:02:07PM +0200, Johannes Schindelin wrote: > Hi Michael, > > On Sun, 10 Apr 2016, Michael S. Tsirkin wrote: > > > This implements a new ack! action for git rebase -i > > It is essentially a middle ground between fixup! and squash!: > > - commits are squashed silently without editor being started > > - commit logs are concatenated (with action line being discarded) > > - because of the above, empty commits aren't discarded, > > their log is also included. > > > > I am using it as follows: > > git am -s < mailbox #creates first commit > > hack ... > > get mail with Ack > > git commit --allow-empty -m `cat <<-EOF > > ack! first > > > > Acked-by: maintainer > > EOF` > > repeat cycle > > git rebase --autosquash -i origin/master > > before public branch push > > > > The "cat" command above is actually a script that > > parses the Ack mail to create the empty commit - > > to be submitted separately. > > This looks awfully complicated, still, and not very generic. > > How about making it easier to use, and much, much more generic, like this? I can look at using a different syntax but the below does not support the workflow I described, which is a standard email based one: get email, handle it. > 1. introducing an `--add-footer` flag to `git commit` that you could use > like this: > > git commit --amend --add-footer "Acked-by: Bugs Bunny" > 2. introducing an `--exec-after` flag to `git commit` that would be a new > sibling of `--fixup` and `--squash` and would work like this: > > git commit --exec-after HEAD~5 \ > 'git commit --amend --add-footer "Acked-by: Bugs Bunny"' But it wouldn't address my use-case where I get an ack by email. If I have to dig up the relevant commit(s) by hand anyway, then what was the point? > > (it should imply `--allow-empty`, of course, and probably even fail if > anything was staged for commit at that point.) The commit message would > then look something like > > exec-after! Fix broken breakage > > git commit --amend --add-footer "Acked-by: Bugs Bunny" So if I happen to fetch a branch from someone and rebase it, stuff gets auto-executed on my local system? That looks scary. > This way would obviously benefit a lot more users. It might benefit others who have the commit handy but it does not look like it helps email based workflow. > For example, you could > easily say (and alias) > > git commit --amend --add-footer 'Reviewed-by: Arrested Developer" > > i.e. support all kind of use cases where developers need to slap on > footers in a quick & easy way. > > And the --exec-after option would obviously have *a lot* more use cases > than just squashing in ACKs. > > Ciao, > Johannes So far I only see examples of adding footers. If that's all we can think up, why code in all this genericity? All these small scripts scattered around just make things hard to use, and add security issues. -- MSR -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git interpret-trailers with multiple keys
On Mon, Apr 11, 2016 at 09:09:48AM +0200, Matthieu Moy wrote: > "Michael S. Tsirkin" writes: > > > On Sun, Apr 10, 2016 at 06:57:53PM +0200, Christian Couder wrote: > >> What I meant is that we could create new options called maybe > >> trailer.autocommands and trailer..autocommands that default to > >> 'true' and if 'false' the command would not be run automatically and > >> the corresponding trailer would not be added. > > > > I don't think it has to do with commands. > > For example, if we add "value" it should behave the same. > > > > So I think a better name is "ifnotlisted", with values "add" > > and "donothing". > > Having a negation in the variable name feels wrong. When the token is > listed on the command-line and ifnotlisted=donothing, I have to apply a > double-negation to guess what would happen (=> "if listed then do > something"). Isn't this similar to ifmissing? > I agree that having such variable would be a good thing. It would solve > your issue (i.e. "How to I configure a token for quick use from the > command-line without applying it unconditionally"), and allow full > backward compatibility. > > I'd call the option "apply" or perhaps "run", with values 1/true/always > = default = current behavior, or "auto" = "apply when asked from the > command-line". I'm wondering whether other values could make sense (not > to implement it right now, but to keep the design open to further > extensions): perhaps apply=ifauthor could mean "apply this trailer to > patches I'm the author of" for example. > > -- > Matthieu Moy > http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git interpret-trailers with multiple keys
On Sun, Apr 10, 2016 at 06:57:53PM +0200, Christian Couder wrote: > What I meant is that we could create new options called maybe > trailer.autocommands and trailer..autocommands that default to > 'true' and if 'false' the command would not be run automatically and > the corresponding trailer would not be added. I don't think it has to do with commands. For example, if we add "value" it should behave the same. So I think a better name is "ifnotlisted", with values "add" and "donothing". Thoughts? -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git interpret-trailers with multiple keys
On Sun, Apr 10, 2016 at 06:57:53PM +0200, Christian Couder wrote:
> On Sun, Apr 10, 2016 at 11:32 AM, Michael S. Tsirkin wrote:
> > On Wed, Apr 06, 2016 at 10:28:21PM -0400, Christian Couder wrote:
> >> On Wed, Apr 6, 2016 at 3:30 PM, Michael S. Tsirkin wrote:
> >> > On Wed, Apr 06, 2016 at 10:42:42AM -0700, Junio C Hamano wrote:
> >> >> "Michael S. Tsirkin" writes:
> >> >>
> >> >> > On Wed, Apr 06, 2016 at 06:58:30PM +0200, Matthieu Moy wrote:
> >> >> >> "Michael S. Tsirkin" writes:
> >> >> >>
> >> >> >> > I have this in .git/config
> >> >> >> >
> >> >> >> > [trailer "r"]
> >> >> >> > key = Reviewed-by
> >> >> >> > command = "echo \"Michael S. Tsirkin >> >> >> > [trailer "s"]
> >> >> >> > key = Signed-off-by
> >> >> >> > command = "echo \"Michael S. Tsirkin >> >> >> >
> >> >> >> > whenever I run git interpret-trailers -t r I see these lines added:
> >> >> >> >
> >> >> >> > Reviewed-by: Michael S. Tsirkin >> >> >> > Signed-off-by: Michael S. Tsirkin >> >> >> > Reviewed-by: Michael S. Tsirkin >> >> >> >
> >> >> >> > Why is Reviewed-by repeated? Bug or feature?
> >> >> >>
> >> >> >> The first two lines are added unconditionally:
> >> >> >>
> >> >> >> $ echo | git interpret-trailers
> >> >> >>
> >> >> >> Reviewed-by: Michael S. Tsirkin >> >> >> Signed-off-by: Michael S. Tsirkin >> >> >>
> >> >> >> The last line is added because you've asked for it with --trailer r.
> >>
> >> Yes, and because the default is to add the trailer at the end.
> >>
> >> >> >> I don't think it's currently possible to get the behavior you seem to
> >> >> >> expect, ie. to define trailer tokens fully (key and value) in your
> >> >> >> config file but use them only on request.
> >>
> >> Yes, because you could define for example a function like this:
> >>
> >> reviewed() {
> >> git interpret-trailers --trailer 'Reviewed-by: Michael S. Tsirkin
> >> ' --in-place "$@"
> >> }
> >>
> >> So it is kind of easy already to make things requestable.
> >
> > Not if any commands are configured. interpret-trailers will
> > insist on running them in any case.
>
> If one want something requestable instead of automatic, one should
> definitely not configure any command.
Then one can't set any values, only keys.
> >> If people really want some configured trailers to be used only on
> >> request, it is possible to add a config option for that.
> >
> > this is not what the documentation says though:
>
> What I meant is that we could create new options called maybe
> trailer.autocommands and trailer..autocommands that default to
> 'true' and if 'false' the command would not be run automatically and
> the corresponding trailer would not be added.
>
> > I would say that if people really want to run all trailers while also
> > passing some on command line, *that* should be a config option.
> > Current default violates the principle of least surprise.
>
> Current default is documented and is the most powerful default.
I'm not sure what makes you say that. What makes it the most powerful?
> Yes, it might be surprising though.
> >> >> >> (BTW, I think you wanted a closing > at the end)
> >> >> >
> >> >> > Is this worth fixing? It doesn't look like a behaviour anyone
> >> >> > would want...
> >> >>
> >> >> CC'ing Christian who's done the "trailers" thing.
> >> >>
> >> >> Personally, I do not think adding any configured trailers without
> >> >> being asked is a sensible behaviour, but it is likely that people
> >> >> already depend on it, as we seem to see "How do I configure to
> >> >> always add this and that trailer?" from time to time. I do not
> >> >> think it is unreasonable to disable the "automatically add
> >> >> everything that is configured" when the command line arguments ask
> >> >> for some specific trailer, but I haven't thought deeply about it.
> >> >>
> >> >> An additional (uninformed) observation is that the 'echo' looks like
> >> >> an ugly workaround for the lack of "always use this string as the
> >> >> value" configuration.
> >> >
> >> > Or at least a default.
> >> >
> >> >> Perhaps next to trailer..command, we
> >> >> would need trailer..value?
> >>
> >> Yeah, that is possible too.
> >> It could be bit redundant if we already have a config option to say if
> >> the trailer has to be requested.
> >
> > Seems unrelated - if one just wants a string, using echo as
> > a command is inefficient and inconvenient.
>
> Efficiency is not very high in the list for this kind of things. Also
> when these features were developed, many people wanted different
> powerful things and many people said they could help develop them
> though very few did help. So if you think trailer..value is
> really needed you are welcome to develop it.
>
> Thanks,
> Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git interpret-trailers with multiple keys
On Wed, Apr 06, 2016 at 10:28:21PM -0400, Christian Couder wrote:
> On Wed, Apr 6, 2016 at 3:30 PM, Michael S. Tsirkin wrote:
> > On Wed, Apr 06, 2016 at 10:42:42AM -0700, Junio C Hamano wrote:
> >> "Michael S. Tsirkin" writes:
> >>
> >> > On Wed, Apr 06, 2016 at 06:58:30PM +0200, Matthieu Moy wrote:
> >> >> "Michael S. Tsirkin" writes:
> >> >>
> >> >> > I have this in .git/config
> >> >> >
> >> >> > [trailer "r"]
> >> >> > key = Reviewed-by
> >> >> > command = "echo \"Michael S. Tsirkin >> >> > [trailer "s"]
> >> >> > key = Signed-off-by
> >> >> > command = "echo \"Michael S. Tsirkin >> >> >
> >> >> > whenever I run git interpret-trailers -t r I see these lines added:
> >> >> >
> >> >> > Reviewed-by: Michael S. Tsirkin >> >> > Signed-off-by: Michael S. Tsirkin >> >> > Reviewed-by: Michael S. Tsirkin >> >> >
> >> >> > Why is Reviewed-by repeated? Bug or feature?
> >> >>
> >> >> The first two lines are added unconditionally:
> >> >>
> >> >> $ echo | git interpret-trailers
> >> >>
> >> >> Reviewed-by: Michael S. Tsirkin >> >> Signed-off-by: Michael S. Tsirkin >> >>
> >> >> The last line is added because you've asked for it with --trailer r.
>
> Yes, and because the default is to add the trailer at the end.
>
> >> >> I don't think it's currently possible to get the behavior you seem to
> >> >> expect, ie. to define trailer tokens fully (key and value) in your
> >> >> config file but use them only on request.
>
> Yes, because you could define for example a function like this:
>
> reviewed() {
> git interpret-trailers --trailer 'Reviewed-by: Michael S. Tsirkin
> ' --in-place "$@"
> }
>
> So it is kind of easy already to make things requestable.
Not if any commands are configured. interpret-trailers will
insist on running them in any case.
> If people really want some configured trailers to be used only on
> request, it is possible to add a config option for that.
this is not what the documentation says though:
If some = arguments are also passed on the
command line, when a trailer..command is configured, the
command will also be executed for each of these arguments.
And the part of these arguments, if any, will be used to
replace the $ARG string in the command.
so it says command *for a given token* is run.
I would say that if people really want to run all trailers while also
passing some on command line, *that* should be a config option.
Current default violates the principle of least surprise.
> >> >> (BTW, I think you wanted a closing > at the end)
> >> >
> >> > Is this worth fixing? It doesn't look like a behaviour anyone
> >> > would want...
> >>
> >> CC'ing Christian who's done the "trailers" thing.
> >>
> >> Personally, I do not think adding any configured trailers without
> >> being asked is a sensible behaviour, but it is likely that people
> >> already depend on it, as we seem to see "How do I configure to
> >> always add this and that trailer?" from time to time. I do not
> >> think it is unreasonable to disable the "automatically add
> >> everything that is configured" when the command line arguments ask
> >> for some specific trailer, but I haven't thought deeply about it.
> >>
> >> An additional (uninformed) observation is that the 'echo' looks like
> >> an ugly workaround for the lack of "always use this string as the
> >> value" configuration.
> >
> > Or at least a default.
> >
> >> Perhaps next to trailer..command, we
> >> would need trailer..value?
>
> Yeah, that is possible too.
> It could be bit redundant if we already have a config option to say if
> the trailer has to be requested.
Seems unrelated - if one just wants a string, using echo as
a command is inefficient and inconvenient.
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] builtin/interpret-trailers: suppress blank line
On Thu, Apr 07, 2016 at 10:34:51AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" writes:
>
> > No - but then I will need to re-run mailinfo to parse the result,
> > will I not?
>
> By the way, I suspect (if Christian did his implementation right
> when he did interpret-trailers) all these points may become moot.
>
> I haven't re-reviewed what is in interpret-trailers, but the vision
> has been that its internal workings should be callable directly into
> instead of running it via run_commands() interface passing the data
> via on-disk file. In the codepath you touch in 3/4 and 4/4, you
> already have not just mi.log_message but msg that has the whole
> payload to create a commit object out of already, so shouldn't it be
> just the matter of passing to some API function
> that was prepared to implement interpret-trailers?
That's certainly possible, though it will need a rework
of the internal API: we currently have:
void process_trailers(const char *file, int in_place, int trim_empty,
int suppress_blank_line, struct string_list *trailers)
{
struct trailer_item *in_tok_first = NULL;
struct trailer_item *in_tok_last = NULL;
struct trailer_item *arg_tok_first;
struct strbuf **lines;
int trailer_end;
FILE *outfile = stdout;
/* Default config must be setup first */
git_config(git_trailer_default_config, NULL);
git_config(git_trailer_config, NULL);
lines = read_input_file(file);
So process_trailers can be changed to get struct strbuf ** instead.
But it seems that the output would have to go into a temporary file
anyway, unless trailer.c is completely rewritten, since it
currently does all output by writing it into a file.
Is that an issue?
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] git-ack: record an ack
This is a simple script that I use by piping
incoming mail with an ack to it.
It produces an empty ack commit suitable for
squshing with git rebase -i -autosquash.
Works best if people ack individual commits: you simply
pipe each ack to git ack, before pushing your branch,
rebase.
Some people ack series by responding to cover letter
or to commit 1.
To address this usecase, there are two additional
flags: -s saves the ack signature in a file (you can
save several in a row), -r creates an ack for
a given patch using the saved signature.
Thus: pipe ack(s) to git ack -s, then select and pipe
each individual patch to git ack -r.
If it's found useful, this script can either
become a first-class command (with documentation
and tests) or be integrated as a flag into git am.
Limitations: requires that index is clean, this is
so we can create an empty commit recording the ack.
Signed-off-by: Michael S. Tsirkin
---
contrib/git-ack | 91 +
1 file changed, 91 insertions(+)
create mode 100755 contrib/git-ack
diff --git a/contrib/git-ack b/contrib/git-ack
new file mode 100755
index 000..d8cba95
--- /dev/null
+++ b/contrib/git-ack
@@ -0,0 +1,91 @@
+msg=$(mktemp)
+patch=$(mktemp)
+info=$(git mailinfo $msg $patch)
+subj=$(echo "$info" | sed -n 's/^Subject: //p')
+#strip ack!/fixup!/squash! prefix
+subj=$(echo "$subj" | sed "s/^fixup![ ]*//")
+subj=$(echo "$subj" | sed "s/^squash![ ]*//")
+subj=$(echo "$subj" | sed "s/^ack![]*//")
+author=$(echo "$info" | sed -n 's/^Author: //p')
+email=$(echo "$info" | sed -n 's/^Email: //p')
+auth="$author <$email>"
+date=$(echo "$info" | sed -n 's/^Date: //p')
+sign=$(mktemp)
+echo "ack! $subj" >"$sign"
+echo "" >> $sign
+if
+ git diff --exit-code --cached HEAD
+then
+ :
+else
+ echo "DIFF in cache. Not acked, reset or commit!"
+ exit 1
+fi
+GIT_DIR=$(git rev-parse --git-dir)
+
+usage () {
+ echo "Usage: git ack " \
+ "[-s|--save|-a|--append|-r|--restore |-c|--clear]\n" >&2;
+ exit 1;
+}
+
+append=
+save=
+clear=
+restore=
+
+while test $# != 0
+do
+ case "$1" in
+ -a|--append)
+ append="y"
+ ;;
+ -s|--s)
+ save="y"
+ ;;
+ -r|--restore)
+ restore="y"
+ ;;
+ -c|--clear)
+ clear="y"
+ ;;
+ *)
+ usage ;;
+ esac
+ shift
+done
+
+if
+ test "$clear"
+then
+ rm -f "${GIT_DIR}/ACKS"
+fi
+
+if
+ test "$save"
+then
+ if
+ test "$append"
+ then
+ cat $msg >>"${GIT_DIR}/ACKS"
+ else
+ cat $msg >"${GIT_DIR}/ACKS"
+ fi
+ exit 0
+fi
+
+if
+ test "$restore"
+then
+ msg=${GIT_DIR}/ACKS
+fi
+
+echo $msg > /dev/tty
+if
+ grep '^[A-Z][A-Za-z-]*-by:' $msg >> $sign
+then
+ git commit --allow-empty -F $sign --author="$auth" --date="$date"
+else
+ echo "No signature found!"
+ exit 2
+fi
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] rebase -i: add ack action
This implements a new ack! action for git rebase -i
It is essentially a middle ground between fixup! and squash!:
- commits are squashed silently without editor being started
- commit logs are concatenated (with action line being discarded)
- because of the above, empty commits aren't discarded,
their log is also included.
I am using it as follows:
git am -s < mailbox #creates first commit
hack ...
get mail with Ack
git commit --allow-empty -m `cat <<-EOF
ack! first
Acked-by: maintainer
EOF`
repeat cycle
git rebase --autosquash -i origin/master
before public branch push
The "cat" command above is actually a script that
parses the Ack mail to create the empty commit -
to be submitted separately.
Signed-off-by: Michael S. Tsirkin
---
git-rebase--interactive.sh | 36
1 file changed, 28 insertions(+), 8 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 4cde685..6a766ca 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -150,6 +150,7 @@ Commands:
r, reword = use commit, but edit the commit message
e, edit = use commit, but stop for amending
s, squash = use commit, but meld into previous commit
+ a, ack = like "squash", but append commit body only to previous commit
f, fixup = like "squash", but discard this commit's log message
x, exec = run command (the rest of the line) using shell
d, drop = remove commit
@@ -438,6 +439,15 @@ update_squash_messages () {
echo
commit_message $2
;;
+ ack)
+ if test -f "$fixup_msg"
+ then
+ commit_message $2 | git stripspace --strip-comments |
sed -e '1,2d' >> "$fixup_msg"
+ fi
+ printf '%s\n' "$comment_char This is the $(nth_string $count)
commit message:"
+ echo
+ commit_message $2
+ ;;
fixup)
echo
printf '%s\n' "$comment_char The $(nth_string $count) commit
message will be skipped:"
@@ -479,7 +489,7 @@ record_in_rewritten() {
echo "$oldsha1" >> "$rewritten_pending"
case "$(peek_next_command)" in
- squash|s|fixup|f)
+ squash|s|fixup|f|ack|a)
;;
*)
flush_rewritten_pending
@@ -551,8 +561,11 @@ do_next () {
warn "Stopped at $sha1... $rest"
exit_with_patch $sha1 0
;;
- squash|s|fixup|f)
+ squash|s|fixup|f|ack|a)
case "$command" in
+ ack|a)
+ squash_style=ack
+ ;;
squash|s)
squash_style=squash
;;
@@ -576,7 +589,7 @@ do_next () {
die_failed_squash $sha1 "$rest"
fi
case "$(peek_next_command)" in
- squash|s|fixup|f)
+ squash|s|fixup|f|ack|a)
# This is an intermediate commit; its message will only
be
# used in case of trouble. So use the long version:
do_with_author output git commit --amend --no-verify -F
"$squash_msg" \
@@ -587,7 +600,7 @@ do_next () {
# This is the final command of this squash/fixup group
if test -f "$fixup_msg"
then
- do_with_author git commit --amend --no-verify
-F "$fixup_msg" \
+ do_with_author git commit --quiet --amend
--no-verify -F "$fixup_msg" \
${gpg_sign_opt:+"$gpg_sign_opt"} ||
die_failed_squash $sha1 "$rest"
else
@@ -717,7 +730,7 @@ skip_unnecessary_picks () {
done <"$todo" >"$todo.new" 3>>"$done" &&
mv -f "$todo".new "$todo" &&
case "$(peek_next_command)" in
- squash|s|fixup|f)
+ squash|s|fixup|f|ack|a)
record_in_rewritten "$onto"
;;
esac ||
@@ -764,7 +777,7 @@ rearrange_squash () {
do
test -z "${format}" || message=$(git log -n 1 --format="%s"
${sha1})
case "$message" in
- "squash! "*|"fixup! "*)
+ "squash! "*|"fixup! "*|"ack! "*)
action="${message%%!*}"
[PATCH 0/4] support for ack commits
This is a repost after rebasing, and addressing comments by Eric Sunshine and
Fabian Ruch. I'd like to try getting this upstream so I can stop maintaining
it. So reposting - rebased to latest master, with a better motivation in the
cover letter.
As a maintainer, I get patches by mail, then
acked-by,reviewed-by etc responses are sent by separate
mail.
The result is that I have a patch applied, and now
I need to find it and apply the ack responses to it.
The flow I use to handle this, is to record an
empty commit (which I'm calling an ack commit)
with just the ack in the log, and ack! tag
and the subject of the original patch in the
subject.
Sometimes, I would also make a small change with
that ack commit, typically using commit --amend,
for example if the ack mail says:
Subject: Re: [PATCH] xyz
please rename xyz to foo. With that:
Acked-by: Michael S. Tsirkin
I would apply ack and make the change as part of that.
Later, once in a while I rebase and squash the ack commits
into the regular one: the rebase autosquash mechanics
find the original commit and update the commit log,
appending the ack.
from example, we start with an email:
From: Michael S. Tsirkin
Subject: [PATCH] foo.c: change b to c
Date: Wed Apr 6 22:07:34 2016 +0300
foo.c: change b to c
Change B to C
Signed-off-by: Michael S. Tsirkin
---
diff --git a/foo.c b/foo.c
index 8e5be91..34654fc 100644
--- a/foo.c
+++ b/foo.c
@@ -676,6 +676,6 @@ FFF(
A
- B
- B
+ C
+ C
D
and I apply it using git am.
then I get an email:
Subject: Re: [PATCH] foo.c: change b to c
> foo.c: change b to c
>
> Change B to C
>
> Signed-off-by: Michael S. Tsirkin
>
> ---
>
> diff --git a/foo.c b/foo.c
> index 8e5be91..34654fc 100644
> --- a/foo.c
> +++ b/foo.c
> @@ -676,6 +676,6 @@ FFF(
> A
>
> - B
> - B
> + C
> + C
>
> D
Acked-by: Junio C Hamano
I then create an empty commit using the subject and the ack line:
commit 4d54b237d8d03323933e27119272e93cf33b4e98
Author: Michael S. Tsirkin
Date: Wed Apr 6 22:07:34 2016 +0300
ack! foo.c: change b to c
Acked-by: Junio C Hamano
(with no change) and then after rebase -i --autosquash it is combined
with the original commit by squashing changes (easy as the
second one has an empty change), and appending the commit log
from the second one to first one:
commit ef7b6d457c28bcd06d0118a889c7070fc800f3d5
Author: Michael S. Tsirkin
Date: Wed Apr 6 14:55:59 2016 +0300
foo.c: change b to c
Change BBBBB to C
Signed-off-by: Michael S. Tsirkin
Acked-by: Junio C Hamano
diff --git a/foo.c b/foo.c
index 8e5be91..34654fc 100644
--- a/foo.c
+++ b/foo.c
@@ -676,6 +676,6 @@ FFF(
A
- B
- B
+ C
+ C
D
The empty ack commits can be created by hand. I have also
written a small script for that - included
as patch 4/4 but it is still rather rough so only putting it
under contrib for now - would like to try
and merge the rebase machinery in place first.
Long term, it might be cleaner to teach git-am about an --ack flag.
But it is already helpful to explain how this is intended to be used.
That script can be used in one of two ways:
1. pipe the mail with ack to it. we extract
subject and prepend ack!, extract the ack
trailer line that needs to be appended to commit,
and record the result as en empty commit.
2. pipe the mail with ack to it with flag -s,
this saves the ack trailer into a file.
then pipe the original patch(es) to it
with flag -r, now the subject is taken from
patch, with ack! prepended, but the ack trailer
is from the ack email.
this is useful to handle series acks, similar to:
For series:
Acked-by: Mic
[PATCH 2/4] git-rebase: document ack
document ack! behaviour and use
Signed-off-by: Michael S. Tsirkin
---
Documentation/git-rebase.txt | 45 +++-
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 0387b40..257d75c 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -402,7 +402,7 @@ or by giving more than one `--exec`:
+
If `--autosquash` is used, "exec" lines will not be appended for
the intermediate commits, and will only appear at the end of each
-squash/fixup series.
+squash/fixup/ack series.
+
This uses the `--interactive` machinery internally, but it can be run
without an explicit `--interactive`.
@@ -419,13 +419,13 @@ without an explicit `--interactive`.
--autosquash::
--no-autosquash::
- When the commit log message begins with "squash! ..." (or
- "fixup! ..."), and there is a commit whose title begins with
+ When the commit log message begins with "squash! ..." ("fixup! ..."
+ or "ack! ..."), and there is a commit whose title begins with
the same ..., automatically modify the todo list of rebase -i
so that the commit marked for squashing comes right after the
commit to be modified, and change the action of the moved
- commit from `pick` to `squash` (or `fixup`). Ignores subsequent
- "fixup! " or "squash! " after the first, in case you referred to an
+ commit from `pick` to `squash` (`fixup` or `ack`). Ignores subsequent
+ "ack! ", "fixup! " or "squash! " after the first, in case you referred
to an
earlier fixup/squash with `git commit --fixup/--squash`.
+
This option is only valid when the '--interactive' option is used.
@@ -649,6 +649,41 @@ consistent (they compile, pass the testsuite, etc.) you
should use
'git stash' to stash away the not-yet-committed changes
after each commit, test, and amend the commit if fixes are necessary.
+
+RECORDING ACKS
+
+
+Interactive mode with --autosquash can be used to concatenate
+commit log for several commits, which is useful to record
+extra information about the commit, such as ack signatures.
+This allows, for example, the following workflow:
+
+1. receive patches by mail and commit
+2. receive by mail ack signatures for the patches
+3. prepare a series for submission
+4. submit
+
+where point 2. consists of several instances of
+ i) create a (possibly empty) commit with signature
+ in the commit message
+
+Sometimes the ack signature added in i. cannot be amended to the
+commit it acks, because that commit is buried deeply in a
+patch series. That is exactly what rebase --autosquash
+option is for: use it
+after plenty of "i"s, to automaticlly rearrange
+commits, and squashing multiple sign-off commits into
+the commit that is signed.
+
+Start it with the last commit you want to retain as-is:
+
+ git rebase --autosquash -i
+
+An editor will be fired up with all the commits in your current branch
+which come after the given commit. Ack commits will be
+re-arranged to come after the commit that is acked,
+and the action will be automatically changed from `pick` to `ack`
+to cause them to be squashed into the acked commit.
RECOVERING FROM UPSTREAM REBASE
---
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] rebase: test ack
test ack! handling Signed-off-by: Michael S. Tsirkin --- t/t3415-rebase-autosquash.sh | 15 +++ 1 file changed, 15 insertions(+) diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh index 8f53e54..e78897d 100755 --- a/t/t3415-rebase-autosquash.sh +++ b/t/t3415-rebase-autosquash.sh @@ -74,6 +74,21 @@ test_expect_success 'auto squash (option)' ' test_auto_squash final-squash --autosquash ' +test_expect_success 'auto ack' ' + ack="Acked-by: xyz" && + msg=$(test_write_lines "ack! first commit" "" "$ack") && + git reset --hard base && + git commit --allow-empty -m "$msg" -- && + git tag ack && + test_tick && + git rebase --autosquash -i HEAD^^^ && + git log --oneline >actual && + git show -s first-commit | grep -v ^commit > expected-msg && + echo "$ack" >> expected-msg && + git show -s HEAD^ | grep -v ^commit > actual-msg && + diff actual-msg expected-msg +' + test_expect_success 'auto squash (config)' ' git config rebase.autosquash true && test_auto_squash final-squash-config-true && -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rebase: convert revert to squash on autosquash
On Fri, Apr 08, 2016 at 01:13:51PM +0200, Johannes Schindelin wrote: > Hi Michael, > > On Thu, 7 Apr 2016, Michael S. Tsirkin wrote: > > > On Thu, Apr 07, 2016 at 05:23:09PM +0200, Johannes Schindelin wrote: > > > > > > On Thu, 7 Apr 2016, Michael S. Tsirkin wrote: > > > > > > > Reverts can typically be treated like squash. Eliminating both the > > > > original commit and the revert would be even nicer, but this seems a bit > > > > harder to implement. > > > > > > Whoa. This rings a lot of alarm bells, very loudly. > > > > Whoa don't be alarmed. It's just a patch :). > > It's just a patch. Like every major breakage would be. So: no, there is > reason to be alarmed if it is likely to disrupt normal usage. > > > > It seems you intend to introduce a *major* change in behavior, > > > > Doing this automatically for all users might be a bit too drastic for > > the upstream git. > > That is a pretty safe thing to say, even without the subjunctive. > > > If there's a commit later followed by a revert, history can be > > simplified by squashing them, and if the result is empty, removing both. > > True. But that is not what the user told Git to do. If the user's > intention was to squash the reverting patch, she could have easily done > this: > > git revert -n deadbeef > git commit --squash deadbeef > > where "deadbeef" is the placeholder for the actual commit to revert. > > And indeed, I use exactly this song and dance quite frequently, *iff* my > intention is to drop a patch. Well then you have to decide whether you want to drop it when you commit. If *I* want do drop the patch when I commit, I just do git rebase. > A much better idea than co-opting the "Revert" commit message would be to > introduce a sibling to --fixup and --squash that you could call --drop. > > Ciao, > Johannes Maybe but it's a different usecase. What this addresses is a case where you first wanted to avoid rebases, so you reverted. But then you rebased after all. Now finding what was reverted automatically is helpful. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rebase: convert revert to squash on autosquash
On Fri, Apr 08, 2016 at 01:42:12PM +0200, Matthieu Moy wrote: > Johannes Schindelin writes: > > > git revert -n deadbeef > > git commit --squash deadbeef > > > > where "deadbeef" is the placeholder for the actual commit to revert. > > > > And indeed, I use exactly this song and dance quite frequently, *iff* my > > intention is to drop a patch. > > > > A much better idea than co-opting the "Revert" commit message would be to > > introduce a sibling to --fixup and --squash that you could call > > --drop. > > One could also add --fixup and --squash to "git revert", so the above > would become > > git revert --squash deadbeef > > In most cases, I find it simpler to just start a rebase -i and drop the > commit from rebase's todo-list. Absolutely, but moving rebase to near the commit to drop also makes it easier to spot where the commit is. Thoughts? > -- > Matthieu Moy > http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rebase: convert revert to squash on autosquash
On Fri, Apr 08, 2016 at 01:13:51PM +0200, Johannes Schindelin wrote: > Hi Michael, > > On Thu, 7 Apr 2016, Michael S. Tsirkin wrote: > > > On Thu, Apr 07, 2016 at 05:23:09PM +0200, Johannes Schindelin wrote: > > > > > > On Thu, 7 Apr 2016, Michael S. Tsirkin wrote: > > > > > > > Reverts can typically be treated like squash. Eliminating both the > > > > original commit and the revert would be even nicer, but this seems a bit > > > > harder to implement. > > > > > > Whoa. This rings a lot of alarm bells, very loudly. > > > > Whoa don't be alarmed. It's just a patch :). > > It's just a patch. Like every major breakage would be. So: no, there is > reason to be alarmed if it is likely to disrupt normal usage. > > > > It seems you intend to introduce a *major* change in behavior, > > > > Doing this automatically for all users might be a bit too drastic for > > the upstream git. > > That is a pretty safe thing to say, even without the subjunctive. > > > If there's a commit later followed by a revert, history can be > > simplified by squashing them, and if the result is empty, removing both. > > True. But that is not what the user told Git to do. If the user's > intention was to squash the reverting patch, she could have easily done > this: > > git revert -n deadbeef > git commit --squash deadbeef > > where "deadbeef" is the placeholder for the actual commit to revert. > > And indeed, I use exactly this song and dance quite frequently, *iff* my > intention is to drop a patch. > > A much better idea than co-opting the "Revert" commit message would be to > introduce a sibling to --fixup and --squash that you could call --drop. > > Ciao, > Johannes Sounds rather cool. Or alternatively git revert --squash deadbeef -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t
On Thu, Apr 07, 2016 at 10:30:02AM -0700, Junio C Hamano wrote: > "Michael S. Tsirkin" writes: > > > On Thu, Apr 07, 2016 at 09:55:29AM -0700, Junio C Hamano wrote: > >> "Michael S. Tsirkin" writes: > >> > >> > Allow -t as a short-cut for --trailer. > >> > > >> > Signed-off-by: Michael S. Tsirkin > >> > --- > >> > >> As I do not think interpret-trailers is meant to be end-user facing, > >> I am not sure I should be interested in this step. > >> > >> I am in principle OK with the later step that teaches a single > >> letter option to end-user facing "git am" that would be turned into > >> "--trailer" when it calls out to "interpret-trailers" (I haven't > >> checked if 't' is a sensible choice for that single letter option, > >> though). > > > > Does OPT_PASSTHRU_ARGV handle this transformation for me? > > As I wrote in my response to Matthieu, PASSTHRU_ARGV is one thing I > specifically do not want to see used in this codepath. It sounds like a general kind of thing, does it not? Aren't there other cases where a short option needs to be converted to a long one? -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t
On Thu, Apr 07, 2016 at 10:26:33AM -0700, Junio C Hamano wrote: > Matthieu Moy writes: > > >> I am in principle OK with the later step that teaches a single > >> letter option to end-user facing "git am" that would be turned into > >> "--trailer" when it calls out to "interpret-trailers" (I haven't > >> checked if 't' is a sensible choice for that single letter option, > >> though). > > > > If 'am' has -t == --trailer, I think it makes sense to have the same > > shortcut in interpret-trailers for consistency. > > It is the other way around. "git am" may be OK with "-t" (or it may > not--I do not know yet), but other commands that are currently > unaware of "interpret-trailers" (cherry-pick, revert, etc.) may have > better uses for a short-and-sweet 't'. > > In the ideal future, "interpret-trailers" should not have to exist > in the end-users' vocabulary, as all the front-line end-user facing > programs would be aware of it. But we are not there. > > Letting it reserve a short-and-sweet 't' that allows it to dictate > that its callers must have the same 't' is tail wagging the dog that > I want to avoid. It's mostly a short-cut I took by copying calls to applypatch. Are there examples of other commands doing such transformations on the fly? -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t
On Thu, Apr 07, 2016 at 09:55:29AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" writes:
>
> > Allow -t as a short-cut for --trailer.
> >
> > Signed-off-by: Michael S. Tsirkin
> > ---
>
> As I do not think interpret-trailers is meant to be end-user facing,
> I am not sure I should be interested in this step.
>
> I am in principle OK with the later step that teaches a single
> letter option to end-user facing "git am" that would be turned into
> "--trailer" when it calls out to "interpret-trailers" (I haven't
> checked if 't' is a sensible choice for that single letter option,
> though).
Does OPT_PASSTHRU_ARGV handle this transformation for me?
> > builtin/interpret-trailers.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> > index b99ae4b..18cf640 100644
> > --- a/builtin/interpret-trailers.c
> > +++ b/builtin/interpret-trailers.c
> > @@ -25,7 +25,7 @@ int cmd_interpret_trailers(int argc, const char **argv,
> > const char *prefix)
> > struct option options[] = {
> > OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
> > OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty
> > trailers")),
> > - OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
> > + OPT_STRING_LIST('t', "trailer", &trailers, N_("trailer"),
> > N_("trailer(s) to add")),
> > OPT_END()
> > };
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] builtin/interpret-trailers: suppress blank line
On Thu, Apr 07, 2016 at 10:00:37AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" writes:
>
> > it's sometimes useful to be able to pass output message of
> > git-mailinfo through git-interpret-trailers,
> > but that creates problems since that does not
> > include the subject and an empty line after that,
> > making interpret-trailers add an empty line.
> >
> > Add a flag to bypass adding the blank line.
>
> I think I understand what you are trying to do, but using output
> that comes from 'mailinfo' alone as the input to anything (including
> interpret-trailers) does not make much sense.
>
> If you use the mailinfo output in the way it is expected to be used,
> i.e. take the subject from the "info" that goes to its standard
> output and append the "msg" with a blank between them, and feed the
> result to interpret-trailers, do you still need this step in your
> series?
No - but then I will need to re-run mailinfo to parse the result,
will I not?
And unfortunately it appears that interpret-trailers can't
handle arbitrary mail - it wants data from commit.
> >
> > Signed-off-by: Michael S. Tsirkin
> > ---
> > trailer.h| 2 +-
> > builtin/interpret-trailers.c | 9 +++--
> > trailer.c| 10 +++---
> > 3 files changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/trailer.h b/trailer.h
> > index 36b40b8..afcf680 100644
> > --- a/trailer.h
> > +++ b/trailer.h
> > @@ -2,6 +2,6 @@
> > #define TRAILER_H
> >
> > void process_trailers(const char *file, int in_place, int trim_empty,
> > - struct string_list *trailers);
> > + int suppress_blank_line, struct string_list *trailers);
> >
> > #endif /* TRAILER_H */
> > diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> > index 18cf640..4a92788 100644
> > --- a/builtin/interpret-trailers.c
> > +++ b/builtin/interpret-trailers.c
> > @@ -18,11 +18,14 @@ static const char * const
> > git_interpret_trailers_usage[] = {
> >
> > int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
> > {
> > + int suppress_blank_line = 0;
> > int in_place = 0;
> > int trim_empty = 0;
> > struct string_list trailers = STRING_LIST_INIT_DUP;
> >
> > struct option options[] = {
> > + OPT_BOOL(0, "suppress-blank-line", &suppress_blank_line,
> > +N_("suppress prefixing tailer(s) with a blank line ")),
> > OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
> > OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty
> > trailers")),
> > OPT_STRING_LIST('t', "trailer", &trailers, N_("trailer"),
> > @@ -36,11 +39,13 @@ int cmd_interpret_trailers(int argc, const char **argv,
> > const char *prefix)
> > if (argc) {
> > int i;
> > for (i = 0; i < argc; i++)
> > - process_trailers(argv[i], in_place, trim_empty,
> > &trailers);
> > + process_trailers(argv[i], in_place, trim_empty,
> > +suppress_blank_line, &trailers);
> > } else {
> > if (in_place)
> > die(_("no input file given for in-place editing"));
> > - process_trailers(NULL, in_place, trim_empty, &trailers);
> > + process_trailers(NULL, in_place, trim_empty,
> > +suppress_blank_line, &trailers);
> > }
> >
> > string_list_clear(&trailers, 0);
> > diff --git a/trailer.c b/trailer.c
> > index 8e48a5c..8e5be91 100644
> > --- a/trailer.c
> > +++ b/trailer.c
> > @@ -805,6 +805,7 @@ static void print_lines(FILE *outfile, struct strbuf
> > **lines, int start, int end
> >
> > static int process_input_file(FILE *outfile,
> > struct strbuf **lines,
> > + int suppress_blank_line,
> > struct trailer_item **in_tok_first,
> > struct trailer_item **in_tok_last)
> > {
> > @@ -822,7 +823,8 @@ static int process_input_file(FILE *outfile,
> > /* Print lines before the trailers as is */
> > print_lines(outfile, lines, 0, trailer_start);
> >
> > - if (!has_blank_line_before(lines, trailer_start -
Re: [PATCH 3/4] builtin/am: read mailinfo from file
On Thu, Apr 07, 2016 at 10:08:37AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" writes:
>
> > Slightly slower, but will allow easy additional processing on it.
> >
> > Signed-off-by: Michael S. Tsirkin
> > ---
>
> I haven't read 4/4 yet, but can guess from what this patch does that
> the next step would let others futz with the contents of the message
> that is on disk (i.e. what mailinfo() wrote out, which is identical
> to what we have in mi.log_message at this point of the codeflow)
> before you do the new strbuf_read_file().
>
> It probably is better to do this as part of 4/4; it is easier to
> understand why this is a good and necessary thing to do. An obvious
> improvement is to omit this extra "read back from the filesystem"
> when we won't be making any interpret-trailer calls (i.e. no -t
> option from the command line), but if we stop at this step 3/4, then
> we'd end up wasting cycles without having any benefit.
Hmm - splitting it out was easy for development since I could verify all
tests pass. But if you do want the optimization, then sure, I'll have
to squash it in.
> > builtin/am.c | 9 -
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/am.c b/builtin/am.c
> > index d003939..4180b04 100644
> > --- a/builtin/am.c
> > +++ b/builtin/am.c
> > @@ -1246,6 +1246,7 @@ static int parse_mail(struct am_state *state, const
> > char *mail)
> > FILE *fp;
> > struct strbuf sb = STRBUF_INIT;
> > struct strbuf msg = STRBUF_INIT;
> > + struct strbuf log_msg = STRBUF_INIT;
> > struct strbuf author_name = STRBUF_INIT;
> > struct strbuf author_date = STRBUF_INIT;
> > struct strbuf author_email = STRBUF_INIT;
> > @@ -1330,7 +1331,12 @@ static int parse_mail(struct am_state *state, const
> > char *mail)
> > }
> >
> > strbuf_addstr(&msg, "\n\n");
> > - strbuf_addbuf(&msg, &mi.log_message);
> > +
> > + if (strbuf_read_file(&log_msg, am_path(state, "msg"), 0) < 0) {
> > + die_errno(_("could not read '%s'"), am_path(state, "msg"));
> > + }
>
> I do not think these {} serve any purpose; drop them?
>
> > +
> > + strbuf_addbuf(&msg, &log_msg);
> > strbuf_stripspace(&msg, 0);
> >
> > if (state->signoff)
> > @@ -1349,6 +1355,7 @@ static int parse_mail(struct am_state *state, const
> > char *mail)
> > state->msg = strbuf_detach(&msg, &state->msg_len);
> >
> > finish:
> > + strbuf_release(&log_msg);
> > strbuf_release(&msg);
> > strbuf_release(&author_date);
> > strbuf_release(&author_email);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rebase: convert revert to squash on autosquash
On Thu, Apr 07, 2016 at 05:23:09PM +0200, Johannes Schindelin wrote: > Hi, > > On Thu, 7 Apr 2016, Michael S. Tsirkin wrote: > > > Reverts can typically be treated like squash. Eliminating both the > > original commit and the revert would be even nicer, but this seems a bit > > harder to implement. > > Whoa. This rings a lot of alarm bells, very loudly. Whoa don't be alarmed. It's just a patch :). I've been using this patch for more than a year now, so I thought I'd post it in case it's useful for others. > It seems you intend to > introduce a *major* change in behavior, Doing this automatically for all users might be a bit too drastic for the upstream git. So there could be an option, or something - if there's interest I can add that. I thought I'd test the waters before I spend time on that. > and all we get to convince us that > this is a good change is this puny paragraph (which, by the way, does not > do half a good job of explaining to me what you think this patch is > supposed to do, let alone of convincing me that what you want is a good > change). > > So. What is it again that you want to achieve? Please use plain English, > e.g. explaining how exactly reverts are typically to be treated like > squashes. And please make it convincing, because so far, I am far from > convinced. > > Ciao, > Johannes It's rather simple. If there's a commit later followed by a revert, history can be simplified by squashing them, and if the result is empty, removing both. The removal part is not automatic with my patch. If someone wants to implement it, that would be nice and useful for me. squashing/fixing is similar in that they are also useful to keep history clean. -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] builtin/am: passthrough -t and --trailer flags
Pass -t and --trailer flags to git-reinterpret-trailers.
Signed-off-by: Michael S. Tsirkin
---
builtin/am.c | 48
1 file changed, 48 insertions(+)
diff --git a/builtin/am.c b/builtin/am.c
index 4180b04..480c4c2 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -122,6 +122,7 @@ struct am_state {
int message_id;
int scissors; /* enum scissors_type */
struct argv_array git_apply_opts;
+ struct argv_array git_interpret_trailers_opts;
const char *resolvemsg;
int committer_date_is_author_date;
int ignore_date;
@@ -157,6 +158,8 @@ static void am_state_init(struct am_state *state, const
char *dir)
if (!git_config_get_bool("commit.gpgsign", &gpgsign))
state->sign_commit = gpgsign ? "" : NULL;
+
+ argv_array_init(&state->git_interpret_trailers_opts);
}
/**
@@ -170,6 +173,7 @@ static void am_state_release(struct am_state *state)
free(state->author_date);
free(state->msg);
argv_array_clear(&state->git_apply_opts);
+ argv_array_clear(&state->git_interpret_trailers_opts);
}
/**
@@ -472,6 +476,11 @@ static void am_load(struct am_state *state)
if (sq_dequote_to_argv_array(sb.buf, &state->git_apply_opts) < 0)
die(_("could not parse %s"), am_path(state, "apply-opt"));
+ read_state_file(&sb, state, "interpret-trailers-opt", 1);
+ argv_array_clear(&state->git_interpret_trailers_opts);
+ if (sq_dequote_to_argv_array(sb.buf,
&state->git_interpret_trailers_opts) < 0)
+ die(_("could not parse %s"), am_path(state,
"interpret-trailers-opt"));
+
state->rebasing = !!file_exists(am_path(state, "rebasing"));
strbuf_release(&sb);
@@ -988,6 +997,7 @@ static void am_setup(struct am_state *state, enum
patch_format patch_format,
unsigned char curr_head[GIT_SHA1_RAWSZ];
const char *str;
struct strbuf sb = STRBUF_INIT;
+ struct strbuf tsb = STRBUF_INIT;
if (!patch_format)
patch_format = detect_patch_format(paths);
@@ -1048,6 +1058,9 @@ static void am_setup(struct am_state *state, enum
patch_format patch_format,
sq_quote_argv(&sb, state->git_apply_opts.argv, 0);
write_state_text(state, "apply-opt", sb.buf);
+ sq_quote_argv(&tsb, state->git_interpret_trailers_opts.argv, 0);
+ write_state_text(state, "interpret-trailers-opt", tsb.buf);
+
if (state->rebasing)
write_state_text(state, "rebasing", "");
else
@@ -1072,6 +1085,7 @@ static void am_setup(struct am_state *state, enum
patch_format patch_format,
write_state_count(state, "next", state->cur);
write_state_count(state, "last", state->last);
+ strbuf_release(&tsb);
strbuf_release(&sb);
}
@@ -1233,6 +1247,34 @@ static void am_append_signoff(struct am_state *state)
}
/**
+ * Processes the supplied message file in-place with git-interpret-trailers.
+ * Returns 0 on success, -1 otherwise.
+ */
+static int run_interpret_trailers(const struct am_state *state, const char
*msg)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+
+ if (!state->git_interpret_trailers_opts.argc)
+ return 0;
+
+ cp.git_cmd = 1;
+
+ argv_array_push(&cp.args, "interpret-trailers");
+
+ argv_array_push(&cp.args, "--in-place");
+ argv_array_push(&cp.args, "--suppress-blank-line");
+
+ argv_array_pushv(&cp.args, state->git_interpret_trailers_opts.argv);
+
+ argv_array_push(&cp.args, msg);
+
+ if (run_command(&cp))
+ return -1;
+
+ return 0;
+}
+
+/**
* Parses `mail` using git-mailinfo, extracting its patch and authorship info.
* state->msg will be set to the patch message. state->author_name,
* state->author_email and state->author_date will be set to the patch author's
@@ -1301,6 +1343,9 @@ static int parse_mail(struct am_state *state, const char
*mail)
fclose(mi.input);
fclose(mi.output);
+ if (run_interpret_trailers(state, am_path(state, "msg")) < 0)
+ die("could not interpret trailers");
+
/* Extract message and author information */
fp = xfopen(am_path(state, "info"), "r");
while (!strbuf_getline_lf(&sb, fp)) {
@@ -2299,6 +2344,9 @@ int cmd_am(int argc, const char **argv, const char
*prefix)
OPT_PASSTHRU_ARGV('p', NULL, &state.git_apply_opts, N_("num"),
N_("pass it through git-apply"),
0),
+
[PATCH 3/4] builtin/am: read mailinfo from file
Slightly slower, but will allow easy additional processing on it.
Signed-off-by: Michael S. Tsirkin
---
builtin/am.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/builtin/am.c b/builtin/am.c
index d003939..4180b04 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1246,6 +1246,7 @@ static int parse_mail(struct am_state *state, const char
*mail)
FILE *fp;
struct strbuf sb = STRBUF_INIT;
struct strbuf msg = STRBUF_INIT;
+ struct strbuf log_msg = STRBUF_INIT;
struct strbuf author_name = STRBUF_INIT;
struct strbuf author_date = STRBUF_INIT;
struct strbuf author_email = STRBUF_INIT;
@@ -1330,7 +1331,12 @@ static int parse_mail(struct am_state *state, const char
*mail)
}
strbuf_addstr(&msg, "\n\n");
- strbuf_addbuf(&msg, &mi.log_message);
+
+ if (strbuf_read_file(&log_msg, am_path(state, "msg"), 0) < 0) {
+ die_errno(_("could not read '%s'"), am_path(state, "msg"));
+ }
+
+ strbuf_addbuf(&msg, &log_msg);
strbuf_stripspace(&msg, 0);
if (state->signoff)
@@ -1349,6 +1355,7 @@ static int parse_mail(struct am_state *state, const char
*mail)
state->msg = strbuf_detach(&msg, &state->msg_len);
finish:
+ strbuf_release(&log_msg);
strbuf_release(&msg);
strbuf_release(&author_date);
strbuf_release(&author_email);
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] builtin/interpret-trailers.c: allow -t
Allow -t as a short-cut for --trailer.
Signed-off-by: Michael S. Tsirkin
---
builtin/interpret-trailers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index b99ae4b..18cf640 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -25,7 +25,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const
char *prefix)
struct option options[] = {
OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty
trailers")),
- OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
+ OPT_STRING_LIST('t', "trailer", &trailers, N_("trailer"),
N_("trailer(s) to add")),
OPT_END()
};
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/4] git-am: use trailers to add extra signatures
I'm using git am to apply patches, and I like the ability
to add arbitrary trailers instead of the standard Signed-off-by
one.
To this end, I have extended git am to call git interpret-trailers
internally. This way I can add arbitrary signatures.
For example, I have:
[trailer "t"]
key = Tested-by
command = "echo \"Michael S. Tsirkin \""
[trailer "r"]
key = Reviewed-by
command = "echo \"Michael S. Tsirkin \""
[trailer "a"]
key = Acked-by
command = "echo \"Michael S. Tsirkin \""
[trailer "s"]
key = Signed-off-by
command = "echo \"Michael S. Tsirkin \""
And now:
git am -t t -t r -t s
adds all of:
Tested-by: Michael S. Tsirkin
Reviewed-by: Michael S. Tsirkin
Signed-off-by: Michael S. Tsirkin
This was originally suggested by Junio (a long time ago).
Documentation and tests are still TBD.
Michael S. Tsirkin (4):
builtin/interpret-trailers.c: allow -t
builtin/interpret-trailers: suppress blank line
builtin/am: read mailinfo from file
builtin/am: passthrough -t and --trailer flags
trailer.h| 2 +-
builtin/am.c | 57 +++-
builtin/interpret-trailers.c | 11 ++---
trailer.c| 10 +---
4 files changed, 72 insertions(+), 8 deletions(-)
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] builtin/interpret-trailers: suppress blank line
it's sometimes useful to be able to pass output message of
git-mailinfo through git-interpret-trailers,
but that creates problems since that does not
include the subject and an empty line after that,
making interpret-trailers add an empty line.
Add a flag to bypass adding the blank line.
Signed-off-by: Michael S. Tsirkin
---
trailer.h| 2 +-
builtin/interpret-trailers.c | 9 +++--
trailer.c| 10 +++---
3 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/trailer.h b/trailer.h
index 36b40b8..afcf680 100644
--- a/trailer.h
+++ b/trailer.h
@@ -2,6 +2,6 @@
#define TRAILER_H
void process_trailers(const char *file, int in_place, int trim_empty,
- struct string_list *trailers);
+ int suppress_blank_line, struct string_list *trailers);
#endif /* TRAILER_H */
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 18cf640..4a92788 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -18,11 +18,14 @@ static const char * const git_interpret_trailers_usage[] = {
int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
{
+ int suppress_blank_line = 0;
int in_place = 0;
int trim_empty = 0;
struct string_list trailers = STRING_LIST_INIT_DUP;
struct option options[] = {
+ OPT_BOOL(0, "suppress-blank-line", &suppress_blank_line,
+N_("suppress prefixing tailer(s) with a blank line ")),
OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty
trailers")),
OPT_STRING_LIST('t', "trailer", &trailers, N_("trailer"),
@@ -36,11 +39,13 @@ int cmd_interpret_trailers(int argc, const char **argv,
const char *prefix)
if (argc) {
int i;
for (i = 0; i < argc; i++)
- process_trailers(argv[i], in_place, trim_empty,
&trailers);
+ process_trailers(argv[i], in_place, trim_empty,
+suppress_blank_line, &trailers);
} else {
if (in_place)
die(_("no input file given for in-place editing"));
- process_trailers(NULL, in_place, trim_empty, &trailers);
+ process_trailers(NULL, in_place, trim_empty,
+suppress_blank_line, &trailers);
}
string_list_clear(&trailers, 0);
diff --git a/trailer.c b/trailer.c
index 8e48a5c..8e5be91 100644
--- a/trailer.c
+++ b/trailer.c
@@ -805,6 +805,7 @@ static void print_lines(FILE *outfile, struct strbuf
**lines, int start, int end
static int process_input_file(FILE *outfile,
struct strbuf **lines,
+ int suppress_blank_line,
struct trailer_item **in_tok_first,
struct trailer_item **in_tok_last)
{
@@ -822,7 +823,8 @@ static int process_input_file(FILE *outfile,
/* Print lines before the trailers as is */
print_lines(outfile, lines, 0, trailer_start);
- if (!has_blank_line_before(lines, trailer_start - 1))
+ if (!suppress_blank_line &&
+ !has_blank_line_before(lines, trailer_start - 1))
fprintf(outfile, "\n");
/* Parse trailer lines */
@@ -875,7 +877,8 @@ static FILE *create_in_place_tempfile(const char *file)
return outfile;
}
-void process_trailers(const char *file, int in_place, int trim_empty, struct
string_list *trailers)
+void process_trailers(const char *file, int in_place, int trim_empty,
+ int suppress_blank_line, struct string_list *trailers)
{
struct trailer_item *in_tok_first = NULL;
struct trailer_item *in_tok_last = NULL;
@@ -894,7 +897,8 @@ void process_trailers(const char *file, int in_place, int
trim_empty, struct str
outfile = create_in_place_tempfile(file);
/* Print the lines before the trailers */
- trailer_end = process_input_file(outfile, lines, &in_tok_first,
&in_tok_last);
+ trailer_end = process_input_file(outfile, lines, suppress_blank_line,
+&in_tok_first, &in_tok_last);
arg_tok_first = process_command_line_args(trailers);
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] rebase: convert revert to squash on autosquash
Reverts can typically be treated like squash. Eliminating both the
original commit and the revert would be even nicer, but this seems a bit
harder to implement.
Signed-off-by: Michael S. Tsirkin
---
git-rebase--interactive.sh | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 6a766ca..6fc1935 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -777,7 +777,7 @@ rearrange_squash () {
do
test -z "${format}" || message=$(git log -n 1 --format="%s"
${sha1})
case "$message" in
- "squash! "*|"fixup! "*|"ack! "*)
+ "squash! "*|"fixup! "*|"ack! "*|"Revert \""*)
action="${message%%!*}"
rest=$message
prefix=
@@ -789,6 +789,12 @@ rearrange_squash () {
prefix="$prefix${rest%%!*},"
rest="${rest#*! }"
;;
+ "Revert \""*\")
+ action="squash"
+ prefix="Revert,"
+ rest="${rest#Revert \"}"
+ rest="${rest%%\"}"
+ ;;
*)
break
;;
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git interpret-trailers with multiple keys
On Wed, Apr 06, 2016 at 10:42:42AM -0700, Junio C Hamano wrote: > "Michael S. Tsirkin" writes: > > > On Wed, Apr 06, 2016 at 06:58:30PM +0200, Matthieu Moy wrote: > >> "Michael S. Tsirkin" writes: > >> > >> > I have this in .git/config > >> > > >> > [trailer "r"] > >> > key = Reviewed-by > >> > command = "echo \"Michael S. Tsirkin >> > [trailer "s"] > >> > key = Signed-off-by > >> > command = "echo \"Michael S. Tsirkin >> > > >> > whenever I run git interpret-trailers -t r I see these lines added: > >> > > >> > Reviewed-by: Michael S. Tsirkin >> > Signed-off-by: Michael S. Tsirkin >> > Reviewed-by: Michael S. Tsirkin >> > > >> > Why is Reviewed-by repeated? Bug or feature? > >> > >> The first two lines are added unconditionally: > >> > >> $ echo | git interpret-trailers > >> > >> Reviewed-by: Michael S. Tsirkin >> Signed-off-by: Michael S. Tsirkin >> > >> The last line is added because you've asked for it with --trailer r. > >> > >> I don't think it's currently possible to get the behavior you seem to > >> expect, ie. to define trailer tokens fully (key and value) in your > >> config file but use them only on request. > >> > >> (BTW, I think you wanted a closing > at the end) > > > > Is this worth fixing? It doesn't look like a behaviour anyone > > would want... > > CC'ing Christian who's done the "trailers" thing. > > Personally, I do not think adding any configured trailers without > being asked is a sensible behaviour, but it is likely that people > already depend on it, as we seem to see "How do I configure to > always add this and that trailer?" from time to time. I do not > think it is unreasonable to disable the "automatically add > everything that is configured" when the command line arguments ask > for some specific trailer, but I haven't thought deeply about it. > > An additional (uninformed) observation is that the 'echo' looks like > an ugly workaround for the lack of "always use this string as the > value" configuration. Or at least a default. > Perhaps next to trailer..command, we > would need trailer..value? -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] builtin/trailers: don't always run all commands
If an explicit -t trailer is used, only parse
trailers from command line.
Signed-off-by: Michael S. Tsirkin
---
trailer.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/trailer.c b/trailer.c
index 8e5be91..34654fc 100644
--- a/trailer.c
+++ b/trailer.c
@@ -676,10 +676,14 @@ static struct trailer_item
*process_command_line_args(struct string_list *traile
struct trailer_item *item;
/* Add a trailer item for each configured trailer with a command */
- for (item = first_conf_item; item; item = item->next) {
- if (item->conf.command) {
- struct trailer_item *new = new_trailer_item(item, NULL,
NULL);
- add_trailer_item(&arg_tok_first, &arg_tok_last, new);
+ if (!trailers->nr) {
+ for (item = first_conf_item; item; item = item->next) {
+ if (item->conf.command) {
+ struct trailer_item *new =
+ new_trailer_item(item, NULL, NULL);
+ add_trailer_item(&arg_tok_first,
+&arg_tok_last, new);
+ }
}
}
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git interpret-trailers with multiple keys
On Wed, Apr 06, 2016 at 06:58:30PM +0200, Matthieu Moy wrote: > "Michael S. Tsirkin" writes: > > > I have this in .git/config > > > > [trailer "r"] > > key = Reviewed-by > > command = "echo \"Michael S. Tsirkin > [trailer "s"] > > key = Signed-off-by > > command = "echo \"Michael S. Tsirkin > > > whenever I run git interpret-trailers -t r I see these lines added: > > > > Reviewed-by: Michael S. Tsirkin > Signed-off-by: Michael S. Tsirkin > Reviewed-by: Michael S. Tsirkin > > > Why is Reviewed-by repeated? Bug or feature? > > The first two lines are added unconditionally: > > $ echo | git interpret-trailers > > Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin > The last line is added because you've asked for it with --trailer r. > > I don't think it's currently possible to get the behavior you seem to > expect, ie. to define trailer tokens fully (key and value) in your > config file but use them only on request. > > (BTW, I think you wanted a closing > at the end) Is this worth fixing? It doesn't look like a behaviour anyone would want... > -- > Matthieu Moy > http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
git interpret-trailers with multiple keys
I have this in .git/config [trailer "r"] key = Reviewed-by command = "echo \"Michael S. Tsirkin http://vger.kernel.org/majordomo-info.html
[PATCH] rebase: convert revert to squash on autosquash
Reverts can typically be treated like squash. Eliminating both the
original commit and the revert would be even nicer, but this seems a bit
harder to implement.
Signed-off-by: Michael S. Tsirkin
---
git-rebase--interactive.sh | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 86edac7..a82bbdf 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -745,7 +745,7 @@ rearrange_squash () {
while read -r pick sha1 message
do
case "$message" in
- "squash! "*|"fixup! "*|"ack! "*)
+ "squash! "*|"fixup! "*|"ack! "*|"Revert \""*)
action="${message%%!*}"
rest=$message
prefix=
@@ -757,6 +757,12 @@ rearrange_squash () {
prefix="$prefix${rest%%!*},"
rest="${rest#*! }"
;;
+ "Revert \""*\")
+ action="squash"
+ prefix="Revert,"
+ rest="${rest#Revert \"}"
+ rest="${rest%%\"}"
+ ;;
*)
break
;;
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] git-am: support any number of signatures
On Wed, Sep 24, 2014 at 12:00:40PM +0200, Christian Couder wrote: > On Tue, Sep 23, 2014 at 10:07 AM, Michael S. Tsirkin wrote: > > On Tue, Sep 23, 2014 at 09:45:50AM +0200, Christian Couder wrote: > >> This is probably not as simple as you would like but it works with > >> something like: > >> > >> $ git interpret-trailers --trailer "Acked-by: Michael S. Tsirkin > >> " --trailer "Reviewed-by: Michael S. Tsirkin > >> " --trailer "Tested-by: Michael S. Tsirkin > >> " 0001-foo.patch >to_apply/0001-foo.patch > >> > >> and then: > >> > >> $ git am to_apply/*.patch > >> > >> Also by using something like: > >> > >> $ git config trailer.a.key Acked-by > >> $ git config trailer.r.key Reviewed-by > >> $ git config trailer.t.key Tested-by > > > > I would like multiple keys to match a specific > > letter, e.g. as a maintainer I need > > both reviewed by and signed off by when I > > apply a patch, I like applying them with > > a single "-s m". > > That's different from what you implemented in your patch. > And franckly I think that for this kind of specific use cases, you > could create your own aliases, either Git aliases or just shell > aliases. > > For example if we implement default values and make git am call git > interpret-trailers, a shell alias could simply be: > > alias gamsm='git am --trailer r --trailer s' > > I use "git log --oneline --decorate --graph" very often, so I made my > own alias for it, and I suppose a lot of other people have done so. > > The number of people who will use trailers will probably be much > smaller than the number of people using git log, so if we don't make > shortcuts for "git log --oneline --decorate --graph", I see no ground > to ask for a specific shortcut that adds both a reviewed by and a > signed off by. I've been thinking: how about a generic ability to add option shortcuts for commands in .config? For example: [am "-z"] command = "--trailer foobar" would replace any -z in git am command line with --trailer foobar. Does this sound useful? > >> the first command could be simplified to: > >> > >> $ git interpret-trailers --trailer "a: Michael S. Tsirkin > >> " --trailer "r: Michael S. Tsirkin " > >> --trailer "t: Michael S. Tsirkin " 0001-foo.patch > >> >to_apply/0001-foo.patch > >> > >> And if you use an env variable: > >> > >> $ ME="Michael S. Tsirkin " > >> $ git interpret-trailers --trailer "a: $ME" --trailer "r: $ME" > >> --trailer "t: $ME" 0001-foo.patch >to_apply/0001-foo.patch > >> > >> Maybe later we will integrate git interpret-trailers with git commit, > >> git am and other commands, so that you can do directly: > >> > >> git am --trailer "a: $ME" --trailer "r: $ME" --trailer "t: $ME" > >> 0001-foo.patch > >> > >> Maybe we wil also assign a one letter shortcut to --trailer, for > >> example "z", so that could be: > >> > >> git am -z "a: $ME" -z "r: $ME" -z "t: $ME" 0001-foo.patch > > > > -s could apply here, right? > > I don't know what we will do with -s. Maybe if we use -z, we don't need -s. > > > It doesn't have a parameter at the moment. > > We will have to discuss that kind of thing when we make it possible > for git commit, git am and maybe other commands to accept trailers > arguments and pass them to git interpret-trailers. > > In his email Junio seems to say that we don't need a shortcut like -z, > we could only have --trailer. > And I think that it is indeed sound to at least wait a little before > using up one shortcut like -z in many commands. > > >> We could also allow many separators in the same -z argument as long as > >> they are separated by say "~", > > > > I think -z a -z r -z t is enough. > > Great! I think you will likely have at least "--trailer a --trailer r > --trailer t", but I don't think it is too bad as you can use aliases > to make it shorter to type. > > >> so you could have: > >> > >> git am -z "a: $ME~r: $ME~t: $ME" 0001-foo.patch > >> > >> And then we could also allow people to define default values for > >> trailers with something like: > >> > >> $ git config trailer.
Re: [PATCH RFC] git-am: support any number of signatures
On Tue, Sep 23, 2014 at 10:15:47AM -0700, Junio C Hamano wrote:
> Christian Couder writes:
>
> > On Mon, Sep 22, 2014 at 4:01 PM, Michael S. Tsirkin wrote:
> >> ...
> >> As a reminder, this old patchset (that I replied to) enhanced git am -s
> >> with an option to add different signatures depending on
> >> the option passed to the -s flag.
> >> E.g. I have
> >> [am "a"]
> >> signoff = "Acked-by: Michael S. Tsirkin "
> >>
> >> [am "r"]
> >> signoff = "Reviewed-by: Michael S. Tsirkin "
> >>
> >> [am "t"]
> >> signoff = "Tested-by: Michael S. Tsirkin "
> >>
> >> and now:
> >> git am -s art
> >> adds all 3 signatures when applying the patch.
> >
> > This is probably not as simple as you would like but it works with
> > something like:
> >
> > $ git interpret-trailers --trailer "Acked-by: Michael S. Tsirkin
> > " --trailer "Reviewed-by: Michael S. Tsirkin
> > " --trailer "Tested-by: Michael S. Tsirkin
> > " 0001-foo.patch >to_apply/0001-foo.patch
> >
> > and then:
> >
> > $ git am to_apply/*.patch
>
> If I understand it correctly, Michael is envisioning to implement
> his "git am -s art" (I would recommend against reusing -s for this,
> though. "git am --trailer art" is fine) and doing so by using
> interpret-trailers as an internal implementation detail, so I would
> say the above is a perfectly fine way to do so. An equivalent of
> that command line is synthesized and run internally in his version
> of "git am" when his "git am" sees "--trailer art" option using
> those am.{"a","r","t"}.trailer configuration variables.
Hmm I wonder why do you dislike reusing -s with a parameter for this.
To me, this looks like a superset of the default -s functionality: -s
adds the default signature, -s "x" adds signature "x" ... Users don't
really care that one is implemented as a trailer and another isn't. In
fact, default -s can be implemented as a trailer too, right?
Could you clarify please?
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] git-am: support any number of signatures
On Tue, Sep 23, 2014 at 09:45:50AM +0200, Christian Couder wrote: > Hi Michael, > > On Mon, Sep 22, 2014 at 4:01 PM, Michael S. Tsirkin wrote: > > > > Hi Junio, Christian, > > it's been a while. > > I see that the work on trailers is going on. > > I tried going over the documentation but I could not figure > > out how would one implement multiple signatures using the > > trailers mechanism. > > > > As a reminder, this old patchset (that I replied to) enhanced git am -s > > with an option to add different signatures depending on > > the option passed to the -s flag. > > E.g. I have > > [am "a"] > > signoff = "Acked-by: Michael S. Tsirkin " > > > > [am "r"] > > signoff = "Reviewed-by: Michael S. Tsirkin " > > > > [am "t"] > > signoff = "Tested-by: Michael S. Tsirkin " > > > > and now: > > git am -s art > > adds all 3 signatures when applying the patch. > > This is probably not as simple as you would like but it works with > something like: > > $ git interpret-trailers --trailer "Acked-by: Michael S. Tsirkin > " --trailer "Reviewed-by: Michael S. Tsirkin > " --trailer "Tested-by: Michael S. Tsirkin > " 0001-foo.patch >to_apply/0001-foo.patch > > and then: > > $ git am to_apply/*.patch > > Also by using something like: > > $ git config trailer.a.key Acked-by > $ git config trailer.r.key Reviewed-by > $ git config trailer.t.key Tested-by I would like multiple keys to match a specific letter, e.g. as a maintainer I need both reviewed by and signed off by when I apply a patch, I like applying them with a single "-s m". > the first command could be simplified to: > > $ git interpret-trailers --trailer "a: Michael S. Tsirkin > " --trailer "r: Michael S. Tsirkin " > --trailer "t: Michael S. Tsirkin " 0001-foo.patch > >to_apply/0001-foo.patch > > And if you use an env variable: > > $ ME="Michael S. Tsirkin " > $ git interpret-trailers --trailer "a: $ME" --trailer "r: $ME" > --trailer "t: $ME" 0001-foo.patch >to_apply/0001-foo.patch > > Maybe later we will integrate git interpret-trailers with git commit, > git am and other commands, so that you can do directly: > > git am --trailer "a: $ME" --trailer "r: $ME" --trailer "t: $ME" > 0001-foo.patch > > Maybe we wil also assign a one letter shortcut to --trailer, for > example "z", so that could be: > > git am -z "a: $ME" -z "r: $ME" -z "t: $ME" 0001-foo.patch -s could apply here, right? It doesn't have a parameter at the moment. > We could also allow many separators in the same -z argument as long as > they are separated by say "~", I think -z a -z r -z t is enough. > so you could have: > > git am -z "a: $ME~r: $ME~t: $ME" 0001-foo.patch > > And then we could also allow people to define default values for > trailers with something like: > > $ git config trailer.a.defaultvalue "Michael S. Tsirkin " > $ git config trailer.r.defaultvalue "Michael S. Tsirkin " > $ git config trailer.t.defaultvalue "Michael S. Tsirkin " I'm kind of confused by the key/value concept. Can't I define the whole 'Acked-by: Michael S. Tsirkin ' string as the key? > So that in the end you could have: > > git am -z a~r~t 0001-foo.patch > > which is very close to "git am -s art". > > Best, > Christian. If I figure out the defaultvalue thing, I might find the time to work on git am integration. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] git-am: support any number of signatures
On Wed, Jun 18, 2014 at 10:51:04AM -0700, Junio C Hamano wrote: > Junio C Hamano writes: > > > On Tue, Jun 17, 2014 at 8:09 PM, Michael S. Tsirkin wrote: > >> > >> OK, after looking into this for a while, I realize > >> this is a special property of the Signed-off-by footer. > >> For now I think it's reasonable to just avoid de-duplicating > >> other footers if any. Agree? > > > > Not really. I'd rather see "git am" hardcode as little such policy as > > possible. > > We do need to support S-o-b footer and the policy we defined for it long > > time > > ago, if only for backward compatiblity, but for any other footers, > > policy decision > > such as "dedup by default" isn't something "am" should know about. > > By the way, "append without looking for dups" is a policy decision > that is as bad to have as "append with dedup". > > I'd rather not to see "am.signoff", or any name that implies what > the "-s" option to the command is about for that matter, to be used > in futzing with the trailers other than S-o-b in any way. As I > understand it, our longer term goal is to defer that task, including > the user-programmable policy decisions, to something like the > 'trailer' Christian started. > > I suspect that it may add unnecessary work later if we overloaded > "signoff" with a similar feature with the change under discussion. > I would feel safer to see it outlined how we envision to transition > to a more generic 'trailer' solution later if we were to enhance > "am" with "am.signoff" now. > > Thanks. Hi Junio, Christian, it's been a while. I see that the work on trailers is going on. I tried going over the documentation but I could not figure out how would one implement multiple signatures using the trailers mechanism. As a reminder, this old patchset (that I replied to) enhanced git am -s with an option to add different signatures depending on the option passed to the -s flag. E.g. I have [am "a"] signoff = "Acked-by: Michael S. Tsirkin " [am "r"] signoff = "Reviewed-by: Michael S. Tsirkin " [am "t"] signoff = "Tested-by: Michael S. Tsirkin " and now: git am -s art adds all 3 signatures when applying the patch. Any help will be appreciated. -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: autopacking twice?
On Sun, Jun 22, 2014 at 05:22:32PM +0200, Matthieu Moy wrote: > "Michael S. Tsirkin" writes: > > > Guess: auto-packing was started in background, did not > > complete in time, and was restarted for the second time? > > Probably once for fetch, and another after rebase. > > > If true, some kind of lock file would be useful > > to prevent this. > > It is the case with recent versions of Git (>= 1.8.5). Which version are > you running? Sorry, forgot to mention this: $ git --version git version 2.0.0.542.g41d13ef this is git master with some patches by my own. > -- > Matthieu Moy > http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
autopacking twice?
I noticed this: remote: Counting objects: 302, done. remote: Compressing objects: 100% (195/195), done. remote: Total 209 (delta 169), reused 15 (delta 14) Receiving objects: 100% (209/209), 42.83 KiB | 0 bytes/s, done. Resolving deltas: 100% (169/169), completed with 67 local objects. >From git://git.qemu.org/qemu 6baa963..427e175 master -> origin/master Auto packing the repository in background for optimum performance. See "git help gc" for manual housekeeping. First, rewinding head to replay your work on top of it... Applying: vhost: block migration if backend does not log memory Applying: vhost: fix resource leak in error handling Applying: qapi/hmp: use 'backend' instead of 'device' with memory backend Applying: libqemustub: add more stubs for qemu-char Applying: qtest: fix qtest for vhost-user Applying: qtest: fix vhost-user-test unbalanced mutex locks Applying: e1000: emulate auto-negotiation during external link status change Applying: e1000: improve auto-negotiation reporting via mii-tool Applying: e1000: signal guest on successful link auto-negotiation Applying: e1000: move e1000_autoneg_timer() to after set_ics() Applying: e1000: factor out checking for auto-negotiation availability Applying: fixup! libqemustub: add more stubs for qemu-char Applying: qapi/string-output-visitor: fix human output Applying: tests: add human format test for string output visitor Applying: Revert "fixup! libqemustub: add more stubs for qemu-char" Applying: fixup! libqemustub: add more stubs for qemu-char warning: notes ref refs/notes/commits is invalid Auto packing the repository in background for optimum performance. See "git help gc" for manual housekeeping. Why did it auto-pack twice in a single pull? None of the changes applied are very large. Guess: auto-packing was started in background, did not complete in time, and was restarted for the second time? If true, some kind of lock file would be useful to prevent this. -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] git-am: support any number of signatures
On Wed, Jun 18, 2014 at 10:51:04AM -0700, Junio C Hamano wrote: > Junio C Hamano writes: > > > On Tue, Jun 17, 2014 at 8:09 PM, Michael S. Tsirkin wrote: > >> > >> OK, after looking into this for a while, I realize > >> this is a special property of the Signed-off-by footer. > >> For now I think it's reasonable to just avoid de-duplicating > >> other footers if any. Agree? > > > > Not really. I'd rather see "git am" hardcode as little such policy as > > possible. > > We do need to support S-o-b footer and the policy we defined for it long > > time > > ago, if only for backward compatiblity, but for any other footers, > > policy decision > > such as "dedup by default" isn't something "am" should know about. > > By the way, "append without looking for dups" is a policy decision > that is as bad to have as "append with dedup". > > I'd rather not to see "am.signoff", or any name that implies what > the "-s" option to the command is about for that matter, to be used > in futzing with the trailers other than S-o-b in any way. As I > understand it, our longer term goal is to defer that task, including > the user-programmable policy decisions, to something like the > 'trailer' Christian started. > > I suspect that it may add unnecessary work later if we overloaded > "signoff" with a similar feature with the change under discussion. > I would feel safer to see it outlined how we envision to transition > to a more generic 'trailer' solution later if we were to enhance > "am" with "am.signoff" now. > > Thanks. I'll need to look at trailers, if indeed they are a superset of this functionality, I will transition to trailers when they land on master. In this case seems easier for me to keep this out tree patch for now, Good thing I didn't spend time writing docs and tests :) -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] git-am: support any number of signatures
On Tue, Jun 17, 2014 at 11:49:11PM -0700, Junio C Hamano wrote: > On Tue, Jun 17, 2014 at 8:09 PM, Michael S. Tsirkin wrote: > > > > OK, after looking into this for a while, I realize > > this is a special property of the Signed-off-by footer. > > For now I think it's reasonable to just avoid de-duplicating > > other footers if any. Agree? > > Not really. I'd rather see "git am" hardcode as little such policy as > possible. > We do need to support S-o-b footer and the policy we defined for it long time > ago, if only for backward compatiblity, but for any other footers, > policy decision > such as "dedup by default" isn't something "am" should know about. OK happily that's exactly what v2 that I just posted does. Default S-o-b footer gets the existing policy. Any other footers are added on top without any tricky deduplication. -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RFC v2] git-am: support any number of signatures
I'm using different signature tags for git am depending on the patch,
project and other factors.
Sometimes I add multiple tags as well, e.g. QEMU
wants both Reviewed-by and Signed-off-by tags.
This patch makes it easy to do so:
1. new parameter am.signoff can be used any number
of times:
[am]
signoff = "Reviewed-by: Michael S. Tsirkin "
Will add reviewed by tag in addition to the S.O.B.
if set all signatures are picked up when git am -s is used.
2. Any number of alternative signatures
[am "a"]
signoff = "Acked-by: Michael S. Tsirkin "
[am "t"]
signoff = "Tested-by: Michael S. Tsirkin "
if set the signature type can be specified by passing
a parameter to the -s flag:
git am -sa
A combination is supported:
git am -sa -st
No docs or tests, sorry, so not yet ready for master, but I'm using this
all the time without any issues so maybe ok for pu.
Early flames/feedback/help welcome.
Changes from v1:
Address Junio's feedback:
Default signature is always applied.
This is to make it a no-brainer for people to track using DCO.
De-duplication fixed (works for default signature only
as other signatures might make sense multiple times).
Signed-off-by: Michael S. Tsirkin
---
git-am.sh | 28 +++-
1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/git-am.sh b/git-am.sh
index ee61a77..c1246e6 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -13,7 +13,7 @@ i,interactive run interactively
b,binary* (historical option -- no-op)
3,3way allow fall back on 3way merging if needed
q,quiet be quiet
-s,signoff add a Signed-off-by line to the commit message
+s,signoff? add a Signed-off-by line to the commit message
u,utf8 recode into utf8 (default)
k,keep pass -k flag to git-mailinfo
keep-non-patch pass -b flag to git-mailinfo
@@ -383,6 +383,7 @@ then
keepcr=t
fi
+signoffs=
while test $# != 0
do
case "$1" in
@@ -394,8 +395,15 @@ it will be removed. Please do not use it anymore."
;;
-3|--3way)
threeway=t ;;
- -s|--signoff)
- sign=t ;;
+ --signoff)
+ sign=t
+ s=$(git config --get-all am.signoff)
+ signoffs=("${signoffs[@]}" "${s[@]}") ;;
+ --signoff=*)
+ sign=t
+ a="${1#--signoff=}"
+ s=$(git config --get-all am."${a}".signoff)
+ signoffs=("${signoffs[@]}" "${s[@]}") ;;
-u|--utf8)
utf8=t ;; # this is now default
--no-utf8)
@@ -642,6 +650,16 @@ then
threeway=t
fi
git_apply_opt=$(cat "$dotest/apply-opt")
+EXTRA_SIGNOFFS=
+for ack in "${signoffs[@]}"; do
+ if test "$EXTRA_SIGNOFFS"
+ then
+ EXTRA_SIGNOFFS=$(printf "%s\n%s" "$SIGNOFF" "$ack")
+ else
+ EXTRA_SIGNOFFS="$ack"
+ fi
+done
+
if test "$(cat "$dotest/sign")" = t
then
SIGNOFF=$(git var GIT_COMMITTER_IDENT | sed -e '
@@ -744,13 +762,13 @@ To restore the original branch and stop patching run
\"\$cmdline --abort\"."
"$dotest/msg-clean" |
sed -ne '$p'
)
- ADD_SIGNOFF=$(
+ ADD_SIGNOFF="$EXTRA_SIGNOFFS"$(
test "$LAST_SIGNED_OFF_BY" = "$SIGNOFF" || {
test '' = "$LAST_SIGNED_OFF_BY" && echo
echo "$SIGNOFF"
})
else
- ADD_SIGNOFF=
+ ADD_SIGNOFF="$EXTRA_SIGNOFFS"
fi
{
if test -s "$dotest/msg-clean"
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] git-am: support any number of signatures
On Mon, Jun 16, 2014 at 11:06:20AM -0700, Junio C Hamano wrote: > "Michael S. Tsirkin" writes: > > > Now A wants to sign this patch. > > > > I think there are two reasonable ways to behave: > > 1. What you describe above: > > A > > B > > A > > That is the only sensible thing to do for Signed-off-by footers. OK, after looking into this for a while, I realize this is a special property of the Signed-off-by footer. For now I think it's reasonable to just avoid de-duplicating other footers if any. Agree? To have it apply to other footers, will have to implement ^[A-Z]*-by: logic that I have implemented for sendemail, in am as well. I'll get back to that, but I think it's separate from this feature. -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] git-am: support any number of signatures
On Fri, Jun 13, 2014 at 10:32:09AM -0700, Junio C Hamano wrote: > "Michael S. Tsirkin" writes: > > > On Thu, Jun 12, 2014 at 12:07:03PM -0700, Junio C Hamano wrote: > >> "Michael S. Tsirkin" writes: > >> ... > >> > 1. new parameter am.signoff can be used any number > >> > of times: > >> > > >> > [am] > >> > signoff = "Reviewed-by: Michael S. Tsirkin " > >> > signoff = "Signed-off-by: Michael S. Tsirkin " > >> > > >> > if set all signatures are picked up when git am -s is used. > >> > >> How does this interact with the logic to avoid appending the same > >> Signed-off-by: line as the last one the incoming message already > >> has? > > > > Not handled if you have multiple signatures. > > That will have to be fixed. > > Do we only care about the last line? > > > > Signed-off-by: A > > Signed-off-by: B > > > > do we want to add > > > > Signed-off-by: A > > > > or would it be better to replace with > > Signed-off-by: B > > Signed-off-by: A > > > > ? > > > > Current git am will add A twice, I wonder if this is > > a feature or a bug. > > This is very much deliberate. > > Appending A after existing A and B is meant to record that the patch > originated from A, passed thru B possibly with changes by B, came > back to A who wants to assert that the result is still under DCO. > > The only case we can safely omit appending A's sign-off is when the > last one in the chain is by A. Imagine that you had a patch signed > off by B, which A may have tweaked and forwarded under DCO with A's > sign-off. Such a patch would have sign-off chain B-A. > > Now A makes further changes to the patch and says "the further > change is also something I am authorized to release as open source" > with the "-s" option or some other way. It would not change that A > can contribute under DCO if we did not add an extra A after existing > B-A sign-off chain in that case. OK imagine we have signatures: A B Now A wants to sign this patch. I think there are two reasonable ways to behave: 1. What you describe above: A B A 2. For things like Tested-by: tags, removing tag from where it was and adding it at the bottom: B A This probably calls for a separate feature: maybe adding "acks" along with "signoffs"? acks would be unique, re-adding ack removes it from the message and adds at the bottom. -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] git-am: support any number of signatures
On Thu, Jun 12, 2014 at 09:25:54PM +0200, René Scharfe wrote:
> Am 12.06.2014 18:12, schrieb Michael S. Tsirkin:
> >@@ -136,7 +136,7 @@ fall_back_3way () {
> > eval "$cmd" &&
> > GIT_INDEX_FILE="$dotest/patch-merge-tmp-index" \
> > git write-tree >"$dotest/patch-merge-base+" ||
> >-cannot_fallback "$(gettext "Repository lacks necessary blobs to fall
> >back on 3-way merge.")"
> >+cannot_fallback "$(gettext "Repository lsignoffs necessary blobs to
> >fall back on 3-way merge.")"
>
> lsignoffs?
Heh good catch, I'll fix this up.
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] git-am: support any number of signatures
On Thu, Jun 12, 2014 at 12:07:03PM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" writes:
>
> > I'm using different signature tags for git am depending on the patch,
> > project and other factors.
> >
> > Sometimes I add multiple tags as well, e.g. QEMU
> > wants both Reviewed-by and Signed-off-by tags.
> >
> > This patch makes it easy to do so:
> > 1. new parameter am.signoff can be used any number
> > of times:
> >
> > [am]
> > signoff = "Reviewed-by: Michael S. Tsirkin "
> > signoff = "Signed-off-by: Michael S. Tsirkin "
> >
> > if set all signatures are picked up when git am -s is used.
>
> How does this interact with the logic to avoid appending the same
> Signed-off-by: line as the last one the incoming message already
> has?
Not handled if you have multiple signatures.
That will have to be fixed.
Do we only care about the last line?
Signed-off-by: A
Signed-off-by: B
do we want to add
Signed-off-by: A
or would it be better to replace with
Signed-off-by: B
Signed-off-by: A
?
Current git am will add A twice, I wonder if this is
a feature or a bug.
> > 2. Any number of alternative signatures
> >
> > [am "a"]
> > signoff = "Acked-by: Michael S. Tsirkin "
> >
> > if set the signature type can be specified by passing
> > a parameter to the -s flag:
> >
> > git am -sa
> >
> > No docs or tests, sorry, so not yet ready for master, but I'm using this
> > all the time without any issues so maybe ok for pu.
> > Early flames/feedback/help welcome.
>
> How does that "a" in [am "a"] work? If it defines some kind of
> scope (i.e. use am.a.* instead of am.* when I specify I am using "a"
> set somehow), that might be something interesting, but if it applies
> only to sign-off and other things, then I am not sure if I like it,
> as that would invite confusions from end users.
>
> > + signoffs=("${signoffs[@]}" "${s[@]}") ;;
>
> Is this a shell array? It won't fly in our codebase if that is the
> case.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RFC] git-am: support any number of signatures
I'm using different signature tags for git am depending on the patch,
project and other factors.
Sometimes I add multiple tags as well, e.g. QEMU
wants both Reviewed-by and Signed-off-by tags.
This patch makes it easy to do so:
1. new parameter am.signoff can be used any number
of times:
[am]
signoff = "Reviewed-by: Michael S. Tsirkin "
signoff = "Signed-off-by: Michael S. Tsirkin "
if set all signatures are picked up when git am -s is used.
2. Any number of alternative signatures
[am "a"]
signoff = "Acked-by: Michael S. Tsirkin "
if set the signature type can be specified by passing
a parameter to the -s flag:
git am -sa
No docs or tests, sorry, so not yet ready for master, but I'm using this
all the time without any issues so maybe ok for pu.
Early flames/feedback/help welcome.
Signed-off-by: Michael S. Tsirkin
---
git-am.sh | 36
1 file changed, 28 insertions(+), 8 deletions(-)
diff --git a/git-am.sh b/git-am.sh
index ee61a77..e992c34 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -13,7 +13,7 @@ i,interactive run interactively
b,binary* (historical option -- no-op)
3,3way allow fall back on 3way merging if needed
q,quiet be quiet
-s,signoff add a Signed-off-by line to the commit message
+s,signoff? add a Signed-off-by line to the commit message
u,utf8 recode into utf8 (default)
k,keep pass -k flag to git-mailinfo
keep-non-patch pass -b flag to git-mailinfo
@@ -136,7 +136,7 @@ fall_back_3way () {
eval "$cmd" &&
GIT_INDEX_FILE="$dotest/patch-merge-tmp-index" \
git write-tree >"$dotest/patch-merge-base+" ||
-cannot_fallback "$(gettext "Repository lacks necessary blobs to fall back
on 3-way merge.")"
+cannot_fallback "$(gettext "Repository lsignoffs necessary blobs to fall
back on 3-way merge.")"
say "$(gettext "Using index info to reconstruct a base tree...")"
@@ -383,6 +383,7 @@ then
keepcr=t
fi
+signoffs=
while test $# != 0
do
case "$1" in
@@ -394,8 +395,15 @@ it will be removed. Please do not use it anymore."
;;
-3|--3way)
threeway=t ;;
- -s|--signoff)
- sign=t ;;
+ --signoff)
+ sign=t
+ s=$(git config --get-all am.signoff)
+ signoffs=("${signoffs[@]}" "${s[@]}") ;;
+ --signoff=*)
+ sign=t
+ a="${1#--signoff=}"
+ s=$(git config --get-all am."${a}".signoff)
+ signoffs=("${signoffs[@]}" "${s[@]}") ;;
-u|--utf8)
utf8=t ;; # this is now default
--no-utf8)
@@ -644,14 +652,25 @@ fi
git_apply_opt=$(cat "$dotest/apply-opt")
if test "$(cat "$dotest/sign")" = t
then
- SIGNOFF=$(git var GIT_COMMITTER_IDENT | sed -e '
- s/>.*/>/
- s/^/Signed-off-by: /'
- )
+ for ack in "${signoffs[@]}"; do
+ if test "$SIGNOFF"
+ then
+ SIGNOFF="$SIGNOFF\n$ack"
+ else
+ SIGNOFF="$ack"
+ fi
+ done
+ if test -z "$SIGNOFF"
+ then
+ SIGNOFF=$(git var GIT_COMMITTER_IDENT | sed -e '
+ s/>.*/>/
+ s/^/Signed-off-by: /'
+ )
+ fi
else
SIGNOFF=
fi
last=$(cat "$dotest/last")
this=$(cat "$dotest/next")
if test "$skip" = t
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] ack recoding in commit log
On Wed, Jun 11, 2014 at 10:05:46AM +0200, Fabian Ruch wrote: > Hi Michael, > > On 05/18/2014 11:17 PM, Michael S. Tsirkin wrote: > > As a maintainer, I often get patches by mail, then > > acked-by,reviewed-by etc responses are sent by separate > > mail. > > > > I like making acks commits, > > this way they are easy to keep track of > > as part of git history. > > In order to fully understand your additions, I think, I need some > clarification on the term "ack commit". I think the two additions should be judged separately, they aren't related. So I am not sure we need to spend time discussing what is an ack commit. What I mean is that I prefer using git history for recording activity, and git rebase for modifying history. I think I am not alone in this. To record acks, I wrote this tool to create empty commits, that only record log changes. They are named specially such that rebase knows not to discard them, and to automate the editing stage. I added some features to the tool since. Did not yet address all comments I got here, but just in case you are curious it's attached at the end. > What is an ack commit exactly? > Suppose our principal commit has the commit message > > Some changes > > The changes are... > > Signed-off-by: A U Thor > > and we receive an email from Somebody saying > > > Some changes > > > > The changes are... > > > > Signed-off-by: A U Thor > > Reviewed-by: Somebody Other possible cases I handle: people reply with: For the series: Reviewed-by: Somebody This uses the save/restore flags. or an informal Ack the series (this one I handle by creating ACKS file manually, then applying it with git ack restore) Someone sends a fixup! patch, and someone else replies to the fixup with Reviewed-by: tag. (this was not handled correctly in the version you reviewed, I attach latest one below where it is handled correctly). > . Now, if I understand correctly, we create an empty commit on top of > the principal commit using the following commit message. > > Some changes > > Reviewed-by: Somebody > > Is this commit then called an ack commit? git ack creates commits like this: ack! Some changes Reviewed-by: Somebody --- Other work-flows > Can an ack commit be non-empty? At the moment git ack does not create non empty commits. > Is a commit still an ack if its description mentions additional text > between the subject and the tag lines? At the moment git ack does not create such commits. > Maybe the ack command for todo lists and ack commits have little to do > with one another. I use them together but ack command is more powerful than git ack. Same as commit --fixup, that is not the only way to use fixup!. > If we stick to the term "ack commit", then the command > name suggests that it takes the tags from some commit b and appends them > to the list of tags in the previous commit's (a) message: > > pick a A commit > ack b The next commit > > However, this obviously does not work by just appending messages. For > instance, there could be additional text before or after some tag line > in either commit message. If we treat the workflow you described as a > very specific use case of the ack command instead, it seems reasonable > to add such a todo list functionality for melding commits by silently > appending messages. However, we might consider parametrizing a single > squash command instead of defining just another name that one has to > keep in mind for melding commits: Hmm, I don't see why is flag any easier to remember than a separate command frankly, and there's help text at the end to remind you. > > pick a A commit > squash --no-edit b The next commit That's too much typing I think, I just want to use a single letter. You could argue the same thing for fixup right? It's really squash but discards the log, and there is no requirement that it fixes anything, it can add functionality or whatever. So why not squash --discardlog b The next commit I think that would be bad usability, and so would --no-edit. --no-edit also does not convey the fact that it allows empty commits. I don't mind renaming the command to something else but I could not come up with a better name so far. > > Since response mail happens to have appropriate > > subject matching the patch, it's a natural fit to > > then use git rebase mechanics if we want to smash > > these acks into the original commit. > > > > I have been using these patches without any problems > > for a while now, and find the approach very convenient. > > > > Included: >
Re: 2.0.0 regression? request pull does not seem to find head
On Mon, Jun 02, 2014 at 02:36:01PM -0700, Linus Torvalds wrote: > On Mon, Jun 2, 2014 at 2:01 PM, Michael S. Tsirkin wrote: > > > > [mst@robin linux]$ git request-pull net-next/master > > git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git net-next > > warn: No match for commit 2ae76693b8bcabf370b981cd00c36cd41d33fabc found at > > git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git > > warn: Are you sure you pushed 'net-next' there? > > git request-pull is clearly correct. There is no "net-next" in that > public repository. OK, I see that it's correct. It used to match commit and go from there, but it does not anymore, and I didn't know this. However, it does not tell me this. It tells me there's no match for commit 2ae76693b8bcabf370b981cd00c36cd41d33fabc: that commit is there. Also "match" implies some matching still going on, might be best to drop this. > It *used* to be that request-pull then magically tried to fix it up > for you, which in turn resulted in the guess not being right, like > pointing to the wrong branch that just happened to have the same SHA1, > or pointing to a branch when it _should_ have pointed to a tag. Why not just put the SHA1 in there? In fact it would be a bit more robust in case of non-signed pull requests, won't it? > Now, if you pushed your local "net-next" branch to another branch name > (I can find a branch name called "vhost-next" at that repository, then > you can *tell* git that, using the same syntax you must have used for > the push. > > So something like > > git request-pull net-next/master > git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git > net-next:vhost-next > > should work so that git doesn't end up having to guess (and > potentially guessing wrong). > > But it may actually be a simpler driver error, and you wanted to use > "vhost-next", and that "net-next" was actually incorrect? > >Linus Yes: net-next is a local name, on the remote it's called vhost-next. I realize now that with git request-pull [-p] [] actually is a name of commit in the *remote* tree, not the local one. Documentation could be improved a bit: Commit to end at (defaults to HEAD). This names the commit at the tip of the history you are asking to be pulled. When the repository named by has the commit at a tip of a ref that is different from the ref you have locally, you can use the : syntax, to have its local name, a colon :, and its remote name. It does not have to be commit (could be signed tag), and that text does not make it very clear what is different from what until you re-read it a couple of times. How about: Object (commit or tag) to end at (defaults to HEAD). This names the object at the tip of the history you are asking to be pulled. The name must refer to the same object in both the local tree and the remote tree pointed at by . If the object that you want pulled has a different name in the local and the remote trees, you can use the : syntax, to have its local name, a colon :, and its remote name. The error message could be improved too, it asks me whether I pushed net-next which I did. Would be nicer if it asked "Pushed net-next to net-next there?" Also, how is it supposed to work without ? git request-pull net-next/master git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git warn: No match for commit 2ae76693b8bcabf370b981cd00c36cd41d33fabc found at git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git warn: Are you sure you pushed 'HEAD' there? Where should I push HEAD, and how? In fact does git let you push to HEAD? Finally, the output even with a warning could be better: git request-pull net-next/master git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git warn: No match for commit 2ae76693b8bcabf370b981cd00c36cd41d33fabc found at git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git warn: Are you sure you pushed 'HEAD' there? The following changes since commit 96b2e73c5471542cb9c622c4360716684f8797ed: Revert "net/mlx4_en: Use affinity hint" (2014-06-02 00:18:48 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git If someone does not notice the warning (e.g. the warning is on stderr and script could only redirect stdout) then pull request is actually wrong. It would be better to find the commit on both sides and if it's there, just use the hash name. -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 2.0.0 regression? request pull does not seem to find head
On Mon, Jun 02, 2014 at 02:27:25PM -0700, Junio C Hamano wrote: > "Michael S. Tsirkin" writes: > > > Looks like pull requests no longer work for me on linux. > > Wasn't "does not seem to find head" was very much deliberate? I'm sorry I don't understand what you are asking here. Same thing happens if I use a branch name explicitly, not just HEAD. > Linus's patch wanted the users to explicitly tell the tool, without > tool trying to be too helpful and risking to guess incorrectly. So this is an intentional behaviour change? Which patch do you refer to? > > Some other trees (non-linux) work fine but I didn't yet > > check whether it's the local or the remote tree that's > > at issue. > > > > Or maybe it's a configuration change that I missed? > > > > Note: I have > > [push] > > default = matching > > configured in .gitconfig. > > This should not affect anything in request-pull, I think. I just thought I'd mention this. push behaviour is the only big incompatible change I'm aware of between 1.8 which works for me and 2.0 which doesn't. -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2.0.0 regression? request pull does not seem to find head
Looks like pull requests no longer work for me on linux. Some other trees (non-linux) work fine but I didn't yet check whether it's the local or the remote tree that's at issue. Or maybe it's a configuration change that I missed? Note: I have [push] default = matching configured in .gitconfig. [mst@robin linux]$ git request-pull net-next/master git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git net-next warn: No match for commit 2ae76693b8bcabf370b981cd00c36cd41d33fabc found at git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git warn: Are you sure you pushed 'net-next' there? The following changes since commit 96b2e73c5471542cb9c622c4360716684f8797ed: Revert "net/mlx4_en: Use affinity hint" (2014-06-02 00:18:48 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git net-next for you to fetch changes up to 2ae76693b8bcabf370b981cd00c36cd41d33fabc: vhost: replace rcu with mutex (2014-06-02 23:47:59 +0300) ------------ Michael S. Tsirkin (2): vhost-net: extend device allocation to vmalloc vhost: replace rcu with mutex drivers/vhost/net.c | 23 ++- drivers/vhost/vhost.c | 10 +- 2 files changed, 27 insertions(+), 6 deletions(-) [mst@robin linux]$ git request-pull net-next/master git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git net-next warn: No match for commit 2ae76693b8bcabf370b981cd00c36cd41d33fabc found at git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git warn: Are you sure you pushed 'net-next' there? The following changes since commit 96b2e73c5471542cb9c622c4360716684f8797ed: Revert "net/mlx4_en: Use affinity hint" (2014-06-02 00:18:48 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git net-next for you to fetch changes up to 2ae76693b8bcabf370b981cd00c36cd41d33fabc: vhost: replace rcu with mutex (2014-06-02 23:47:59 +0300) ------------ Michael S. Tsirkin (2): vhost-net: extend device allocation to vmalloc vhost: replace rcu with mutex drivers/vhost/net.c | 23 ++- drivers/vhost/vhost.c | 10 +- 2 files changed, 27 insertions(+), 6 deletions(-) [mst@robin linux]$ git --version git version 2.0.0.538.gb6dd70f [mst@robin linux]$ /usr/bin/git request-pull net-next/master git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git net-next The following changes since commit 96b2e73c5471542cb9c622c4360716684f8797ed: Revert "net/mlx4_en: Use affinity hint" (2014-06-02 00:18:48 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-next for you to fetch changes up to 2ae76693b8bcabf370b981cd00c36cd41d33fabc: vhost: replace rcu with mutex (2014-06-02 23:47:59 +0300) ------------ Michael S. Tsirkin (2): vhost-net: extend device allocation to vmalloc vhost: replace rcu with mutex drivers/vhost/net.c | 23 ++- drivers/vhost/vhost.c | 10 +- 2 files changed, 27 insertions(+), 6 deletions(-) [mst@robin linux]$ /usr/bin/git --version git version 1.8.3.1 -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] rebase: test ack
On Wed, May 21, 2014 at 09:54:47AM -0700, Junio C Hamano wrote: > "Michael S. Tsirkin" writes: > > > On Tue, May 20, 2014 at 08:13:27AM -0700, Junio C Hamano wrote: > >> "Michael S. Tsirkin" writes: > >> > >> > Just to clarify I can post v2 of 4/4 without reposting 1-3 since they > >> > are queued? > >> > >> If you need to update anything queued only on 'pu' but not yet in > >> 'next', it is customary to resend the whole for everybody to see > >> (what is already in 'next' should only be built upon incrementally > >> and not updated with replacement patches), while noting which ones > >> are the same as before. Christian Couder has been doing it nicely > >> in his recent rerolls (if the series were not in 'next', that is). > >> > >> Thanks. > > > > Actually I don't see anything like it in pu. > > The way I usually work is to apply a non-fix (i.e. enhancement) > series on a topic branch forked from 'master' (or the last tagged > version contained in its tip) and see if it makes sense, and then > try-merge the result to 'next' to see if it is free of potential > funny interactions with other topics that are already in flight. > After that happens, the topic branch is merged to somewhere in 'pu'. > > It is possible that I did not have time to go through all the steps > above (after all, I had to make another -rc release and there was an > unexpected last-minute change of plans in the morning that blew a > few hours of work). Or there may have been some merge conflicts > that I didn't feel like resolving for various reasons (e.g. if I > knew the series would be rerolled anyway, it can wait; if the other > topic that interacts with your series has been cooking sufficiently > long in 'next' and if it is very close to the final release of this > cycle, it may be easier to wait for the other topic to graduate to > 'master', which would happen soon after this cycle finishes, and ask > you to rebase your series). > > I don't remember which ;-) > Oh sorry, didn't mean to try to pressure you. I was just surprised not to see it there. I know this applies cleanly to next so I'll just wait for 2.0 to be out. -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] rebase: test ack
On Tue, May 20, 2014 at 08:13:27AM -0700, Junio C Hamano wrote: > "Michael S. Tsirkin" writes: > > > Just to clarify I can post v2 of 4/4 without reposting 1-3 since they > > are queued? > > If you need to update anything queued only on 'pu' but not yet in > 'next', it is customary to resend the whole for everybody to see > (what is already in 'next' should only be built upon incrementally > and not updated with replacement patches), while noting which ones > are the same as before. Christian Couder has been doing it nicely > in his recent rerolls (if the series were not in 'next', that is). > > Thanks. Actually I don't see anything like it in pu. What I would like is for 1-3 to be in pu, 4/4 was for illustrative purposes it's not yet ready for that, and 1-3 are useful by themselves. I could then iterate on 4/4 without reposting 1-3. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] rebase: test ack
On Mon, May 19, 2014 at 02:34:26PM -0700, Junio C Hamano wrote: > "Michael S. Tsirkin" writes: > > > test ack! handling > > > > Signed-off-by: Michael S. Tsirkin > > Will queue with this squashed in. > > 4/4 seems to have some style issues as well, but I didn't look very > closely. > > Thanks. Just to clarify I can post v2 of 4/4 without reposting 1-3 since they are queued? > t/t3415-rebase-autosquash.sh | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh > index 9d7db13..dcdba6f 100755 > --- a/t/t3415-rebase-autosquash.sh > +++ b/t/t3415-rebase-autosquash.sh > @@ -75,18 +75,18 @@ test_expect_success 'auto squash (option)' ' > ' > > test_expect_success 'auto ack' ' > - ack="Acked-by: xyz" > - msg=$(test_write_lines "ack! first commit" "" "$ack") > + ack="Acked-by: xyz" && > + msg=$(test_write_lines "ack! first commit" "" "$ack") && > git reset --hard base && > git commit --allow-empty -m "$msg" -- && > git tag ack && > test_tick && > git rebase --autosquash -i HEAD^^^ && > git log --oneline >actual && > - git show -s first-commit | grep -v ^commit > expected-msg && > - echo "$ack" >> expected-msg && > - git show -s HEAD^ | grep -v ^commit > actual-msg && > - diff actual-msg expected-msg > + git show -s first-commit | grep -v ^commit >expected-msg && > + echo "$ack" >>expected-msg && > + git show -s HEAD^ | grep -v ^commit >actual-msg && > + test_cmp actual-msg expected-msg > ' > > test_expect_success 'auto squash (config)' ' -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] rebase: test ack
On Mon, May 19, 2014 at 02:34:26PM -0700, Junio C Hamano wrote: > "Michael S. Tsirkin" writes: > > > test ack! handling > > > > Signed-off-by: Michael S. Tsirkin > > Will queue with this squashed in. Thanks! And sorry about the style issues. > 4/4 seems to have some style issues as well, but I didn't look very > closely. I'll try to clean it for the next submission. I'll be glad to hear about them as well. Thanks! > Thanks. > > t/t3415-rebase-autosquash.sh | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh > index 9d7db13..dcdba6f 100755 > --- a/t/t3415-rebase-autosquash.sh > +++ b/t/t3415-rebase-autosquash.sh > @@ -75,18 +75,18 @@ test_expect_success 'auto squash (option)' ' > ' > > test_expect_success 'auto ack' ' > - ack="Acked-by: xyz" > - msg=$(test_write_lines "ack! first commit" "" "$ack") > + ack="Acked-by: xyz" && > + msg=$(test_write_lines "ack! first commit" "" "$ack") && > git reset --hard base && > git commit --allow-empty -m "$msg" -- && > git tag ack && > test_tick && > git rebase --autosquash -i HEAD^^^ && > git log --oneline >actual && > - git show -s first-commit | grep -v ^commit > expected-msg && > - echo "$ack" >> expected-msg && > - git show -s HEAD^ | grep -v ^commit > actual-msg && > - diff actual-msg expected-msg > + git show -s first-commit | grep -v ^commit >expected-msg && > + echo "$ack" >>expected-msg && > + git show -s HEAD^ | grep -v ^commit >actual-msg && > + test_cmp actual-msg expected-msg > ' > > test_expect_success 'auto squash (config)' ' -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
format-patch crashes with a huge patchset
I tried to fump the whole history of qemu with format-patch. It crashes both with v2.0.0-rc2-21-g6087111 and with git 1.8.3.1: ~/opt/libexec/git-core/git-format-patch --follow -o patches/all e63c3dc74bfb90e4522d075d0d5a7600c5145745.. Backtrace: Program received signal SIGSEGV, Segmentation fault. 0x0814d9d5 in try_to_follow_renames (opt=0xc8e4, base=base@entry=0x816e4fe "", t2=0xbdf0, t1=0xbddc) at tree-diff.c:227 227 diff_opts.single_follow = opt->pathspec.items[0].match; Missing separate debuginfos, use: debuginfo-install openssl-libs-1.0.1e-37.fc19.1.i686 (gdb) p opt $1 = (struct diff_options *) 0xc8e4 (gdb) where #0 0x0814d9d5 in try_to_follow_renames (opt=0xc8e4, base=base@entry=0x816e4fe "", t2=0xbdf0, t1=0xbddc) at tree-diff.c:227 #1 diff_tree_sha1 (old=0x97469b4 "\372\022\366\336k\345\236\362\062K\021\236\300\227\036\302\217\251\202f", new=new@entry=0x9746994 "$\305H\250)\237\203\266ya\311W\n\274 \n\027^*\221", base=base@entry=0x816e4fe "", opt=opt@entry=0xc8e4) at tree-diff.c:305 #2 0x080fb83d in log_tree_diff (log=0xbf28, commit=0x9734730, opt=0xc618) at log-tree.c:780 #3 log_tree_commit (opt=opt@entry=0xc618, commit=commit@entry=0x9734730) at log-tree.c:810 #4 0x08088406 in cmd_format_patch (argc=, argv=0xccc4, prefix=0x0) at builtin/log.c:1510 #5 0x0804c666 in run_builtin (argv=0xccc4, argc=5, p=0x81cb524 ) at git.c:314 #6 handle_builtin (argc=5, argv=0xccc4) at git.c:487 #7 0x0804bc22 in main (argc=5, av=0xccc4) at git.c:584 (gdb) p opt->pathspec.items $2 = (struct pathspec_item *) 0x0 Did not debug further: could be related to the fact swap is disabled on my box, so attempts to allocate huge amounts of RAM might fail. Still should not segv I think ... -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] rebase -i: add ack action
This implements a new ack! action for git rebase -i
It is essentially a middle ground between fixup! and squash!:
- commits are squashed silently without editor being started
- commit logs are concatenated (with action line being discarded)
- because of the above, empty commits aren't discarded,
their log is also included.
I am using it as follows:
git am -s < mailbox #creates first commit
hack ...
get mail with Ack
git commit --allow-empty -m `cat <<-EOF
ack! first
Acked-by: maintainer
EOF`
repeat cycle
git rebase --autosquash -i origin/master
before public branch push
The "cat" command above is actually a script that
parses the Ack mail to create the empty commit -
to be submitted separately.
Signed-off-by: Michael S. Tsirkin
---
git-rebase--interactive.sh | 34 +++---
1 file changed, 27 insertions(+), 7 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 6ec9d3c..821872c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -140,6 +140,7 @@ Commands:
r, reword = use commit, but edit the commit message
e, edit = use commit, but stop for amending
s, squash = use commit, but meld into previous commit
+ a, ack = like "squash", but append commit body only to previous commit
f, fixup = like "squash", but discard this commit's log message
x, exec = run command (the rest of the line) using shell
@@ -412,6 +413,15 @@ update_squash_messages () {
echo
commit_message $2
;;
+ ack)
+ if test -f "$fixup_msg"
+ then
+ commit_message $2 | git stripspace --strip-comments |
sed -e '1,2d' >> "$fixup_msg"
+ fi
+ printf '%s\n' "$comment_char This is the $(nth_string $count)
commit message:"
+ echo
+ commit_message $2
+ ;;
fixup)
echo
printf '%s\n' "$comment_char The $(nth_string $count) commit
message will be skipped:"
@@ -453,7 +463,7 @@ record_in_rewritten() {
echo "$oldsha1" >> "$rewritten_pending"
case "$(peek_next_command)" in
- squash|s|fixup|f)
+ squash|s|fixup|f|ack|a)
;;
*)
flush_rewritten_pending
@@ -521,8 +531,11 @@ do_next () {
warn "Stopped at $sha1... $rest"
exit_with_patch $sha1 0
;;
- squash|s|fixup|f)
+ squash|s|fixup|f|ack|a)
case "$command" in
+ ack|a)
+ squash_style=ack
+ ;;
squash|s)
squash_style=squash
;;
@@ -546,7 +559,7 @@ do_next () {
die_failed_squash $sha1 "$rest"
fi
case "$(peek_next_command)" in
- squash|s|fixup|f)
+ squash|s|fixup|f|ack|a)
# This is an intermediate commit; its message will only
be
# used in case of trouble. So use the long version:
do_with_author output git commit --amend --no-verify -F
"$squash_msg" \
@@ -557,7 +570,7 @@ do_next () {
# This is the final command of this squash/fixup group
if test -f "$fixup_msg"
then
- do_with_author git commit --amend --no-verify
-F "$fixup_msg" \
+ do_with_author git commit --quiet --amend
--no-verify -F "$fixup_msg" \
${gpg_sign_opt:+"$gpg_sign_opt"} ||
die_failed_squash $sha1 "$rest"
else
@@ -690,7 +703,7 @@ skip_unnecessary_picks () {
done <"$todo" >"$todo.new" 3>>"$done" &&
mv -f "$todo".new "$todo" &&
case "$(peek_next_command)" in
- squash|s|fixup|f)
+ squash|s|fixup|f|ack|a)
record_in_rewritten "$onto"
;;
esac ||
@@ -732,7 +745,7 @@ rearrange_squash () {
while read -r pick sha1 message
do
case "$message" in
- "squash! "*|"fixup! "*)
+ "squash! "*|"fixup! "*|"ack! "*)
action="${message%%!*}"
rest=$message
prefix=
@@ -740,7 +753,7 @@ rearrange_squash
[PATCH 0/4] ack recoding in commit log
As a maintainer, I often get patches by mail, then acked-by,reviewed-by etc responses are sent by separate mail. I like making acks commits, this way they are easy to keep track of as part of git history. Since response mail happens to have appropriate subject matching the patch, it's a natural fit to then use git rebase mechanics if we want to smash these acks into the original commit. I have been using these patches without any problems for a while now, and find the approach very convenient. Included: rebase: new ack! action to handle ack commits this part seems ready for merge to me, please review and comment git-ack: new tool to record an ack this does not have proper documentation and tests yet, I definitely intend to do this but wanted to see whether people like the UI first. posting for early review and feedback Note: yes, I think notes+git replace could be used for this too, my workflow always includes a rebase with --autosquash before publishing anyway, so dealing with ack as with any other commit is nicer. Michael S. Tsirkin (4): rebase -i: add ack action git-rebase: document ack rebase: test ack git-ack: record an ack Documentation/git-rebase.txt | 45 ++--- contrib/git-ack | 79 git-rebase--interactive.sh | 34 +++ t/t3415-rebase-autosquash.sh | 15 + 4 files changed, 161 insertions(+), 12 deletions(-) create mode 100755 contrib/git-ack -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] git-ack: record an ack
This is a simple script that I use by piping
incoming mail with an ack to it.
It produces an empty ack commit suitable for
squshing with git rebase -i -autosquash.
Works best if people ack individual commits: you simply
pipe each ack to git ack, before pushing your branch,
rebase.
Some people ack series by responding to cover letter
or to commit 1.
To address this usecase, there are two additional
flags: -s saves the ack signature in a file (you can
save several in a row), -r creates an ack for
a given patch using the saved signature.
Thus: pipe ack(s) to git ack -s, then select and pipe
each individual patch to git ack -r.
I don't use these flags much so they likely work
less well.
If it's found useful, this script can either
become a first-class command (with documentation
and tests) or be integrated as a flag into git am.
Limitations: requires that index is clean, this is
so we can create an empty commit recording the ack.
Signed-off-by: Michael S. Tsirkin
---
contrib/git-ack | 84 +
1 file changed, 84 insertions(+)
create mode 100755 contrib/git-ack
diff --git a/contrib/git-ack b/contrib/git-ack
new file mode 100755
index 000..4aeb16a
--- /dev/null
+++ b/contrib/git-ack
@@ -0,0 +1,84 @@
+msg=`mktemp`
+patch=`mktemp`
+info=`git mailinfo $msg $patch`
+subj=`echo "$info"|sed -n 's/^Subject: //p'`
+author=`echo "$info"|sed -n 's/^Author: //p'`
+email=`echo "$info"|sed -n 's/^Email: //p'`
+auth="$author <$email>"
+date=`echo "$info"|sed -n 's/^Date: //p'`
+sign=`mktemp`
+echo "ack! $subj" > $sign
+echo "" >> $sign
+if
+git diff --cached HEAD
+then
+nop < /dev/null
+else
+echo "DIFF in cache. Not acked, reset or commit!"
+exit 1
+fi
+GIT_DIR=`pwd`/${GIT_DIR}
+
+usage () {
+ echo "Usage: git ack " \
+"[-s|--save|-a|--append|-r|--restore |-c|--clear]\n" >& 2;
+exit 1;
+}
+
+append=
+save=
+clear=
+
+while test $# != 0
+do
+ case "$1" in
+ -a|--append)
+ append="y"
+ ;;
+ -s|--s)
+ save="y"
+ ;;
+ -r|--restore)
+ restore="y"
+ ;;
+ -c|--clear)
+ clear="y"
+;;
+ *)
+ usage ;;
+ esac
+ shift
+done
+
+if
+test "$clear"
+then
+rm -f "${GIT_DIR}/ACKS"
+fi
+
+if
+test "$save"
+then
+if
+test "$append"
+then
+cat $msg >> "${GIT_DIR}/ACKS"
+else
+cat $msg > "${GIT_DIR}/ACKS"
+fi
+fi
+
+if
+test "$restore"
+then
+msg = ${GIT_DIR}/ACKS
+fi
+
+if
+grep '^[A-Z][A-Za-z-]*-by:' $msg >> $sign
+then
+git commit --allow-empty -F $sign --author="$auth" --date="$date"
+else
+echo "No signature found!"
+exit 2
+fi
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] git-rebase: document ack
document ack! behaviour and use
Signed-off-by: Michael S. Tsirkin
---
Documentation/git-rebase.txt | 45 +++-
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 2a93c64..c27aef4 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -384,7 +384,7 @@ or by giving more than one `--exec`:
+
If `--autosquash` is used, "exec" lines will not be appended for
the intermediate commits, and will only appear at the end of each
-squash/fixup series.
+squash/fixup/ack series.
--root::
Rebase all commits reachable from , instead of
@@ -398,13 +398,13 @@ squash/fixup series.
--autosquash::
--no-autosquash::
- When the commit log message begins with "squash! ..." (or
- "fixup! ..."), and there is a commit whose title begins with
+ When the commit log message begins with "squash! ..." ("fixup! ..."
+ or "ack! ..."), and there is a commit whose title begins with
the same ..., automatically modify the todo list of rebase -i
so that the commit marked for squashing comes right after the
commit to be modified, and change the action of the moved
- commit from `pick` to `squash` (or `fixup`). Ignores subsequent
- "fixup! " or "squash! " after the first, in case you referred to an
+ commit from `pick` to `squash` (`fixup` or `ack`). Ignores subsequent
+ "ack! ", "fixup! " or "squash! " after the first, in case you referred
to an
earlier fixup/squash with `git commit --fixup/--squash`.
+
This option is only valid when the '--interactive' option is used.
@@ -624,6 +624,41 @@ consistent (they compile, pass the testsuite, etc.) you
should use
'git stash' to stash away the not-yet-committed changes
after each commit, test, and amend the commit if fixes are necessary.
+
+RECORDING ACKS
+
+
+Interactive mode with --autosquash can be used to concatenate
+commit log for several commits, which is useful to record
+extra information about the commit, such as ack signatures.
+This allows, for example, the following workflow:
+
+1. receive patches by mail and commit
+2. receive by mail ack signatures for the patches
+3. prepare a series for submission
+4. submit
+
+where point 2. consists of several instances of
+ i) create a (possibly empty) commit with signature
+ in the commit message
+
+Sometimes the ack signature added in i. cannot be amended to the
+commit it acks, because that commit is buried deeply in a
+patch series. That is exactly what rebase --autosquash
+option is for: use it
+after plenty of "i"s, to automaticlly rearrange
+commits, and squashing multiple sign-off commits into
+the commit that is signed.
+
+Start it with the last commit you want to retain as-is:
+
+ git rebase --autosquash -i
+
+An editor will be fired up with all the commits in your current branch
+which come after the given commit. Ack commits will be
+re-arranged to come after the commit that is acked,
+and the action will be utomticlly changed from `pick` to `ack`
+to cause them to be squashed into the acked commit.
RECOVERING FROM UPSTREAM REBASE
---
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] rebase: test ack
test ack! handling Signed-off-by: Michael S. Tsirkin --- t/t3415-rebase-autosquash.sh | 15 +++ 1 file changed, 15 insertions(+) diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh index 41370ab..9d7db13 100755 --- a/t/t3415-rebase-autosquash.sh +++ b/t/t3415-rebase-autosquash.sh @@ -74,6 +74,21 @@ test_expect_success 'auto squash (option)' ' test_auto_squash final-squash --autosquash ' +test_expect_success 'auto ack' ' + ack="Acked-by: xyz" + msg=$(test_write_lines "ack! first commit" "" "$ack") + git reset --hard base && + git commit --allow-empty -m "$msg" -- && + git tag ack && + test_tick && + git rebase --autosquash -i HEAD^^^ && + git log --oneline >actual && + git show -s first-commit | grep -v ^commit > expected-msg && + echo "$ack" >> expected-msg && + git show -s HEAD^ | grep -v ^commit > actual-msg && + diff actual-msg expected-msg +' + test_expect_success 'auto squash (config)' ' git config rebase.autosquash true && test_auto_squash final-squash-config-true && -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] rebase --keep-empty -i: add test
There's some special code in rebase -i to deal with --keep-empty. Add test for this combination. Signed-off-by: Michael S. Tsirkin --- t/t3404-rebase-interactive.sh | 9 + 1 file changed, 9 insertions(+) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index c0023a5..3b1b863 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -67,6 +67,14 @@ test_expect_success 'setup' ' SHELL= export SHELL +test_expect_success 'rebase --keep-empty' ' + git checkout -b emptybranch master && + git commit --allow-empty -m "empty" && + git rebase --keep-empty -i HEAD~2 && + git log --oneline >actual && + test_line_count = 6 actual +' + test_expect_success 'rebase -i with the exec command' ' git checkout master && ( -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] request-pull: resurrect for-linus -> tags/for-linus DWIM
On Fri, May 16, 2014 at 10:57:50AM -0700, Junio C Hamano wrote: > Older versions of Git before v1.7.10 did not DWIM > > $ git pull $URL for-linus > > to the tag "tags/for-linus" and the users were required to say > > $ git pull $URL tags/for-linus > > instead. Because newer versions of Git works either way, > request-pull used to show tags/for-linus when asked > > $ git request-pull origin/master $URL for-linus > > The recent updates broke this and in the output we see "for-linus" > without the "tags/" prefix. > > As v1.7.10 is more than 2 years old, this should matter very little > in practice, but resurrecting it is very simple. Well RHEL6 apparently comes with git 1.7.1, there are probably others. The problem isn't theorectical actually, the reason I noticed the change in behaviour is because one of my pull requests got bounced because of it: http://article.gmane.org/gmane.comp.emulators.qemu/273121 > Signed-off-by: Junio C Hamano Tested-by: Michael S. Tsirkin > --- > > * I _think_ the fix, without breaking the spirit of Linus's "I do >not want the thing DWIM based on what the remote end has" >original, would be as simple as this patch. We can queue it as a >regression fix and do another round of -rc4 if those who depend >on request-pull heavily feel strongly about it. > > git-request-pull.sh | 6 ++ > t/t5150-request-pull.sh | 8 +++- > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/git-request-pull.sh b/git-request-pull.sh > index b67513a..d6648b2 100755 > --- a/git-request-pull.sh > +++ b/git-request-pull.sh > @@ -118,6 +118,12 @@ then > status=1 > fi > > +# Special case: turn "for_linus" to "tags/for_linus" when it is correct > +if test "$ref" = "refs/tags/$pretty_remote" > +then > + pretty_remote=tags/$pretty_remote > +fi > + > url=$(git ls-remote --get-url "$url") > > git show -s --format='The following changes since commit %H: > diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh > index 75d6b38..93e2c65 100755 > --- a/t/t5150-request-pull.sh > +++ b/t/t5150-request-pull.sh > @@ -223,7 +223,13 @@ test_expect_success 'pull request format' ' > git request-pull initial "$downstream_url" > tags/full:refs/tags/full > ) >request && > sed -nf fuzz.sed request.fuzzy && > - test_i18ncmp expect request.fuzzy > + test_i18ncmp expect request.fuzzy && > + > + ( > + cd local && > + git request-pull initial "$downstream_url" full > + ) >request && > + grep ' tags/full$' > ' > > test_expect_success 'request-pull ignores OPTIONS_KEEPDASHDASH poison' ' > -- > 2.0.0-rc3-434-g1ba2fe8 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: regression: request-pull with signed tag lacks tags/ in master
On Thu, May 15, 2014 at 12:13:18PM -0700, Junio C Hamano wrote: > Junio C Hamano writes: > > > "Michael S. Tsirkin" writes: > > > >> looks like pull requests with signed git got broken in git master: > >> > >> [mst@robin qemu]$ /usr/bin/git --version > >> git version 1.8.3.1 > >> [mst@robin qemu]$ git --version > >> git version 2.0.0.rc1.18.gac53fc6.dirty > >> [mst@robin qemu]$ > >> [mst@robin qemu]$ /usr/bin/git request-pull origin/master > >> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream |grep > >> git.kernel.org > >> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream > >> > >> > >> [mst@robin qemu]$ git request-pull origin/master > >> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream |grep > >> git.kernel.org > >> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream > >> > >> this for_upstream syntax is a problem because it does not work > >> for older git clients who might get this request. > >> > >> Didn't bisect yet - anyone knows what broke this? > > > > Linus ;-) The series that ends with ec44507, I think. > > My reading of the earlier parts of the series is that Linus wanted > us never dwim "for-upstream" to "tags/for-upstream" or any other ref > that happens to point at the same commit as for-upstream you have. > The changes done for that purpose covered various cases a bit too > widely, and "request-pull ... tags/for_upstream" were incorrectly > stripped to a request to pull "for_upstream", which was fixed by > 5aae66bd (request-pull: resurrect "pretty refname" feature, > 2014-02-25). > > But that fix does not resurrect the dwimming forbid by the earlier > parts of the series to turn "for_upstream" into "tags/for_upstream". > > What would you get if you do this? > > $ git request-pull origin/master \ > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git \ > tags/for_upstream | grep git.kernel.org I get git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
regression: request-pull with signed tag lacks tags/ in master
looks like pull requests with signed git got broken in git master: [mst@robin qemu]$ /usr/bin/git --version git version 1.8.3.1 [mst@robin qemu]$ git --version git version 2.0.0.rc1.18.gac53fc6.dirty [mst@robin qemu]$ [mst@robin qemu]$ /usr/bin/git request-pull origin/master git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream |grep git.kernel.org git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream [mst@robin qemu]$ git request-pull origin/master git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream |grep git.kernel.org git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream this for_upstream syntax is a problem because it does not work for older git clients who might get this request. Didn't bisect yet - anyone knows what broke this? -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test doc: test_write_lines does not split its arguments
On Mon, May 05, 2014 at 04:51:43PM -0700, Jonathan Nieder wrote:
> test_write_lines carefully quotes its arguments as "$@", so
>
> test_write_lines "a b" c
>
> writes two lines as requested, not three.
>
> Signed-off-by: Jonathan Nieder
Acked-by: Michael S. Tsirkin
> ---
> Hi,
>
> Michael S. Tsirkin wrote:
>
> > +++ b/t/README
> > @@ -596,6 +596,28 @@ library for your script to use.
> > + test_write_lines "a b c d e f g" >foo
> > +
> > + Is a more compact equivalent of:
> > + cat >foo <<-EOF
> > + a
> > + b
> [...]
> > +++ b/t/test-lib-functions.sh
> > @@ -717,6 +717,11 @@ test_ln_s_add () {
> > fi
> > }
> >
> > +# This function writes out its parameters, one per line
> > +test_write_lines () {
> > + printf "%s\n" "$@"
> > +}
>
> How about this patch?
>
> Thanks,
> Jonathan
>
> t/README | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/t/README b/t/README
> index 2d6232f..8a9d499 100644
> --- a/t/README
> +++ b/t/README
> @@ -596,15 +596,14 @@ library for your script to use.
> ...
> '
>
> - - test_write_lines
> + - test_write_lines
>
> - Split to white-space separated words and write it out on standard
> - output, one word per line.
> + Write on standard output, one line per argument.
> Useful to prepare multi-line files in a compact form.
>
> Example:
>
> - test_write_lines "a b c d e f g" >foo
> + test_write_lines a b c d e f g >foo
>
> Is a more compact equivalent of:
> cat >foo <<-EOF
> --
> 1.9.1.423.g4596e3a
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] test/send-email: to-cover, cc-cover tests
On Tue, Apr 29, 2014 at 12:01:10PM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" writes:
>
> > Add tests for the new feature.
> >
> > Signed-off-by: Michael S. Tsirkin
> > ---
> > t/t9001-send-email.sh | 45 +
> > 1 file changed, 45 insertions(+)
> >
> > diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> > index 1ecdacb..97cc094 100755
> > --- a/t/t9001-send-email.sh
> > +++ b/t/t9001-send-email.sh
> > @@ -1334,6 +1334,51 @@ test_expect_success $PREREQ '--force sends cover
> > letter template anyway' '
> > test -n "$(ls msgtxt*)"
> > '
> >
> > +test_cover_addresses () {
> > + header="$1"
> > + shift
> > + clean_fake_sendmail &&
> > + rm -fr outdir &&
> > + git format-patch --cover-letter -2 -o outdir &&
> > + cover=`echo outdir/-*.patch` &&
> > + mv $cover cover-to-edit.patch &&
> > + sed "s/^From:/$header: ex...@address.com\nFrom:/" cover-to-edit.patch >
> > $cover &&
>
> Please do the redirection like this:
>
> sed "s/^From:/$header: ex...@address.com\nFrom:/" cover-to-edit.patch
> >"$cover" &&
>
> in your later patches (I'll tweak this patch myself, so no need to
> resend). We know >$cover should be the same as >"$cover", but it
> was reported that some version of bash does not know it and
> complains instead (see Documentation/CodingGuidelines).
I'll try to remember this, thanks.
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 2/2] test/send-email: to-cover, cc-cover tests
Add tests for the new feature.
Signed-off-by: Michael S. Tsirkin
---
t/t9001-send-email.sh | 45 +
1 file changed, 45 insertions(+)
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 1ecdacb..97cc094 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1334,6 +1334,51 @@ test_expect_success $PREREQ '--force sends cover letter
template anyway' '
test -n "$(ls msgtxt*)"
'
+test_cover_addresses () {
+ header="$1"
+ shift
+ clean_fake_sendmail &&
+ rm -fr outdir &&
+ git format-patch --cover-letter -2 -o outdir &&
+ cover=`echo outdir/-*.patch` &&
+ mv $cover cover-to-edit.patch &&
+ sed "s/^From:/$header: ex...@address.com\nFrom:/" cover-to-edit.patch >
$cover &&
+ git send-email \
+ --force \
+ --from="Example " \
+ --no-to --no-cc \
+ "$@" \
+ --smtp-server="$(pwd)/fake.sendmail" \
+ outdir/-*.patch \
+ outdir/0001-*.patch \
+ outdir/0002-*.patch \
+ 2>errors >out &&
+ grep "^$header: ex...@address.com" msgtxt1 > to1 &&
+ grep "^$header: ex...@address.com" msgtxt2 > to2 &&
+ grep "^$header: ex...@address.com" msgtxt3 > to3 &&
+ test_line_count = 1 to1 &&
+ test_line_count = 1 to2 &&
+ test_line_count = 1 to3
+}
+
+test_expect_success $PREREQ 'to-cover adds To to all mail' '
+ test_cover_addresses "To" --to-cover
+'
+
+test_expect_success $PREREQ 'cc-cover adds Cc to all mail' '
+ test_cover_addresses "Cc" --cc-cover
+'
+
+test_expect_success $PREREQ 'tocover adds To to all mail' '
+ test_config sendemail.tocover true &&
+ test_cover_addresses "To"
+'
+
+test_expect_success $PREREQ 'cccover adds Cc to all mail' '
+ test_config sendemail.cccover true &&
+ test_cover_addresses "Cc"
+'
+
test_expect_success $PREREQ 'sendemail.aliasfiletype=mailrc' '
clean_fake_sendmail &&
echo "alias sbd someb...@example.org" >.mailrc &&
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 1/2] git-send-email: two new options: to-cover, cc-cover
Allow extracting To/Cc addresses from the first patch
(typically the cover letter), and use them as To/Cc addresses of the
remainder of the series.
Signed-off-by: Michael S. Tsirkin
---
Documentation/git-send-email.txt | 12
git-send-email.perl | 16
2 files changed, 28 insertions(+)
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index f0e57a5..b983053 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -248,6 +248,18 @@ Automating
cc list. Default is the value of 'sendemail.signedoffbycc' configuration
value; if that is unspecified, default to --signed-off-by-cc.
+--[no-]cc-cover::
+ If this is set, emails found in Cc: headers in the first patch of
+ the series (typically the cover letter) are added to the cc list
+ for each email set. Default is the value of 'sendemail.cccover'
+ configuration value; if that is unspecified, default to --no-cc-cover.
+
+--[no-]to-cover::
+ If this is set, emails found in To: headers in the first patch of
+ the series (typically the cover letter) are added to the to list
+ for each email set. Default is the value of 'sendemail.tocover'
+ configuration value; if that is unspecified, default to --no-to-cover.
+
--suppress-cc=::
Specify an additional category of recipients to suppress the
auto-cc of:
diff --git a/git-send-email.perl b/git-send-email.perl
index 4c138a2..0084cf4 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -80,6 +80,8 @@ git send-email [options]
--to-cmd * Email To: via ` \$patch_path`
--cc-cmd * Email Cc: via ` \$patch_path`
--suppress-cc * author, self, sob, cc, cccmd, body,
bodycc, all.
+--[no-]cc-cover* Email Cc: addresses in the cover letter.
+--[no-]to-cover* Email To: addresses in the cover letter.
--[no-]signed-off-by-cc* Send to Signed-off-by: addresses. Default
on.
--[no-]suppress-from * Send to self. Default off.
--[no-]chain-reply-to * Chain In-Reply-To: fields. Default off.
@@ -195,6 +197,7 @@ sub do_edit {
# Variables with corresponding config settings
my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc);
+my ($cover_cc, $cover_to);
my ($to_cmd, $cc_cmd);
my ($smtp_server, $smtp_server_port, @smtp_server_options);
my ($smtp_authuser, $smtp_encryption, $smtp_ssl_cert_path);
@@ -211,6 +214,8 @@ my %config_bool_settings = (
"chainreplyto" => [\$chain_reply_to, 0],
"suppressfrom" => [\$suppress_from, undef],
"signedoffbycc" => [\$signed_off_by_cc, undef],
+"cccover" => [\$cover_cc, undef],
+"tocover" => [\$cover_to, undef],
"signedoffcc" => [\$signed_off_by_cc, undef], # Deprecated
"validate" => [\$validate, 1],
"multiedit" => [\$multiedit, undef],
@@ -302,6 +307,8 @@ my $rc = GetOptions("h" => \$help,
"suppress-from!" => \$suppress_from,
"suppress-cc=s" => \@suppress_cc,
"signed-off-cc|signed-off-by-cc!" => \$signed_off_by_cc,
+ "cc-cover|cc-cover!" => \$cover_cc,
+ "to-cover|to-cover!" => \$cover_to,
"confirm=s" => \$confirm,
"dry-run" => \$dry_run,
"envelope-sender=s" => \$envelope_sender,
@@ -1469,6 +1476,15 @@ foreach my $t (@files) {
@to = (@initial_to, @to);
@cc = (@initial_cc, @cc);
+ if ($message_num == 1) {
+ if (defined $cover_cc and $cover_cc) {
+ @initial_cc = @cc;
+ }
+ if (defined $cover_to and $cover_to) {
+ @initial_to = @to;
+ }
+ }
+
my $message_was_sent = send_message();
# set up for the next message
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] git-send-email: two new options: to-cover, cc-cover
On Thu, Apr 03, 2014 at 11:31:51AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" writes:
>
> > Allow extracting To/Cc addresses from cover letter.
>
> Please say what you are doing with what you extract, which is the
> more important part of the objective. Extracting is merely a step
> to achieve that.
>
> s/.$/, to be used as To/Cc addresses of the remainder of the series./
>
> or something.
>
thanks, I did that in the new version.
> I think this will be a very handy feature.
>
> If you have a series *and* you bothered to add To/Cc to the cover
> letter, it is likely that you want all the messages read by these
> people [*1*].
>
> > @@ -1468,6 +1475,15 @@ foreach my $t (@files) {
> > @to = (@initial_to, @to);
> > @cc = (@initial_cc, @cc);
> >
> > + if ($message_num == 1) {
> > + if (defined $cover_cc and $cover_cc) {
> > + @initial_cc = @cc;
> > + }
> > + if (defined $cover_to and $cover_to) {
> > + @initial_to = @to;
> > + }
> > + }
> > +
>
> What is stored away with this code to @initial_cc/to includes:
>
> - what was given to @initial_cc/to before ll.1468-1469
> - what was in @cc/to before ll.1468-1469
>
> when we see the first message [*2*]. The former come from the
> command line --to/--cc, and the latter comes from the header lines
> of the first message. Am I reading the code correctly?
Exactly.
> If that is the case, I think the updated code makes sense.
>
> Thanks.
>
>
> [Footnote]
>
> *1* Allowing this to be disabled is also a good thing this patch
> does. A 100 patch series that does a tree-wide clean-up may
> have different set of people on To/Cc of individual patches, and
> you may want the union of them on To/Cc on the cover letter, so
> that a person may get the cover letter and a single patch that
> relates to his area of expertise without having to see the
> remainder.
>
> *2* The first message may not necessarily be the cover letter. Is
> there a reliable way to detect that?
> The user may want to send
> out a series with only a few patches without any cover, and
> taking To/Cc from the [PATCH 1/3] and propagating them to the
> rest does not match what the documentation and the option name
> claim to do.
Two things that come to mind:
- check that subject has /
Needs some manual parsing, I don't like this much
- check that there's no patch
We could try running git mailinfo but it might give
false negatives if cover letter happens to have
---
diff a/foo b/bar
within it.
Worth worrying about?
For now I simply updated the documentation.
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/2] git-send-email: two new options: to-cover, cc-cover
Allow extracting To/Cc addresses from cover letter.
Signed-off-by: Michael S. Tsirkin
---
Documentation/git-send-email.txt | 12
git-send-email.perl | 16
2 files changed, 28 insertions(+)
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index f0e57a5..1733664 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -248,6 +248,18 @@ Automating
cc list. Default is the value of 'sendemail.signedoffbycc' configuration
value; if that is unspecified, default to --signed-off-by-cc.
+--[no-]cc-cover::
+ If this is set, emails found in Cc: headers in the cover letter are
+ added to the cc list for each email set. Default is the value of
+ 'sendemail.cccover' configuration value; if that is unspecified,
+ default to --no-cc-cover.
+
+--[no-]to-cover::
+ If this is set, emails found in To: headers in the cover letter are
+ added to the to list for each email set. Default is the value of
+ 'sendemail.tocover' configuration value; if that is unspecified,
+ default to --no-to-cover.
+
--suppress-cc=::
Specify an additional category of recipients to suppress the
auto-cc of:
diff --git a/git-send-email.perl b/git-send-email.perl
index 4c138a2..0084cf4 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -80,6 +80,8 @@ git send-email [options]
--to-cmd * Email To: via ` \$patch_path`
--cc-cmd * Email Cc: via ` \$patch_path`
--suppress-cc * author, self, sob, cc, cccmd, body,
bodycc, all.
+--[no-]cc-cover* Email Cc: addresses in the cover letter.
+--[no-]to-cover* Email To: addresses in the cover letter.
--[no-]signed-off-by-cc* Send to Signed-off-by: addresses. Default
on.
--[no-]suppress-from * Send to self. Default off.
--[no-]chain-reply-to * Chain In-Reply-To: fields. Default off.
@@ -195,6 +197,7 @@ sub do_edit {
# Variables with corresponding config settings
my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc);
+my ($cover_cc, $cover_to);
my ($to_cmd, $cc_cmd);
my ($smtp_server, $smtp_server_port, @smtp_server_options);
my ($smtp_authuser, $smtp_encryption, $smtp_ssl_cert_path);
@@ -211,6 +214,8 @@ my %config_bool_settings = (
"chainreplyto" => [\$chain_reply_to, 0],
"suppressfrom" => [\$suppress_from, undef],
"signedoffbycc" => [\$signed_off_by_cc, undef],
+"cccover" => [\$cover_cc, undef],
+"tocover" => [\$cover_to, undef],
"signedoffcc" => [\$signed_off_by_cc, undef], # Deprecated
"validate" => [\$validate, 1],
"multiedit" => [\$multiedit, undef],
@@ -302,6 +307,8 @@ my $rc = GetOptions("h" => \$help,
"suppress-from!" => \$suppress_from,
"suppress-cc=s" => \@suppress_cc,
"signed-off-cc|signed-off-by-cc!" => \$signed_off_by_cc,
+ "cc-cover|cc-cover!" => \$cover_cc,
+ "to-cover|to-cover!" => \$cover_to,
"confirm=s" => \$confirm,
"dry-run" => \$dry_run,
"envelope-sender=s" => \$envelope_sender,
@@ -1469,6 +1476,15 @@ foreach my $t (@files) {
@to = (@initial_to, @to);
@cc = (@initial_cc, @cc);
+ if ($message_num == 1) {
+ if (defined $cover_cc and $cover_cc) {
+ @initial_cc = @cc;
+ }
+ if (defined $cover_to and $cover_to) {
+ @initial_to = @to;
+ }
+ }
+
my $message_was_sent = send_message();
# set up for the next message
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/2] test/send-email: to-cover, cc-cover tests
Add tests for the new feature.
Signed-off-by: Michael S. Tsirkin
---
t/t9001-send-email.sh | 45 +
1 file changed, 45 insertions(+)
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 1ecdacb..97cc094 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1334,6 +1334,51 @@ test_expect_success $PREREQ '--force sends cover letter
template anyway' '
test -n "$(ls msgtxt*)"
'
+test_cover_addresses () {
+ header="$1"
+ shift
+ clean_fake_sendmail &&
+ rm -fr outdir &&
+ git format-patch --cover-letter -2 -o outdir &&
+ cover=`echo outdir/-*.patch` &&
+ mv $cover cover-to-edit.patch &&
+ sed "s/^From:/$header: ex...@address.com\nFrom:/" cover-to-edit.patch >
$cover &&
+ git send-email \
+ --force \
+ --from="Example " \
+ --no-to --no-cc \
+ "$@" \
+ --smtp-server="$(pwd)/fake.sendmail" \
+ outdir/-*.patch \
+ outdir/0001-*.patch \
+ outdir/0002-*.patch \
+ 2>errors >out &&
+ grep "^$header: ex...@address.com" msgtxt1 > to1 &&
+ grep "^$header: ex...@address.com" msgtxt2 > to2 &&
+ grep "^$header: ex...@address.com" msgtxt3 > to3 &&
+ test_line_count = 1 to1 &&
+ test_line_count = 1 to2 &&
+ test_line_count = 1 to3
+}
+
+test_expect_success $PREREQ 'to-cover adds To to all mail' '
+ test_cover_addresses "To" --to-cover
+'
+
+test_expect_success $PREREQ 'cc-cover adds Cc to all mail' '
+ test_cover_addresses "Cc" --cc-cover
+'
+
+test_expect_success $PREREQ 'tocover adds To to all mail' '
+ test_config sendemail.tocover true &&
+ test_cover_addresses "To"
+'
+
+test_expect_success $PREREQ 'cccover adds Cc to all mail' '
+ test_config sendemail.cccover true &&
+ test_cover_addresses "Cc"
+'
+
test_expect_success $PREREQ 'sendemail.aliasfiletype=mailrc' '
clean_fake_sendmail &&
echo "alias sbd someb...@example.org" >.mailrc &&
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 5/9] patch-id: document new behaviour
On Thu, Apr 24, 2014 at 03:12:14PM -0700, Junio C Hamano wrote: > "Michael S. Tsirkin" writes: > > >> > +--unstable:: > >> > +Use a non-symmetrical sum of hashes, such that reordering > >> > >> What is a non-symmetrical sum? > > > > Non-symmetrical combination function is better? > > I do not think either is very good X-<. > > The primary points to convey for "--stable" are: > > - Two patches produced by comparing the same two trees with two >different settings for "-O" will result in the same >patchc signature, thereby allowing the computed result to be used >as a key to index some metainformation about the change between >the two trees; > > - It will produce a result different from the plain vanilla >patch-id has always produced even when used on a diff output >taken without any use of "-O", thereby making existing >databases keyed by patch-ids unusable. > > The fact that we happened to use a patch-id that catches that > somebody reordered the same patch into different file order and > declares that they are two different changes is a more historical > accident than a designed goal. > > I would even say that we would have used the "stable" version from > the beginning if we thought that "-O" would be widely > used when these two features both appeared. Even though I was the > guilty one who introduced it, I'd admit that "-O" has > merely been a curiosity from its inception and has been a failed > experiment, not in the sense that the feature does not work as > adverertised (it does), but in the sense that it is not widely used > (evidenced by the lack of complaints on missing diff.orderfile for a > long time) at all. With "-O" being a failed experiment, > the "unstability" did not matter, so it has stuck. > > The only two things worth mentioning about "--unstable", if our > future direction is to see diff.orderfile and "--stable" a lot more > widely used, are: > > (1) it keeps producing the same patch-id as existing versions of > Git, so users with existing databases (who do not deal with > reordered patches) may want to use it; and perhaps > > (2) it will not consider a patch taken with "-O" and > another without it from the same source the same patches. > > Mathmatically speaking, mentioning "non-symmetrial" might be one way > of expressing the latter point (2), but stressing on that alone > without mentioning (1) misses the point. (2) is _not_ a designed > feature, so it is not very interesting. Unless you have an existing > database, there is no reason to use "--unstable". > > On the other hand (1) is a very relevant thing to mention, as we are > talking about a feature that, if unused, may break existing users' > data. OK I did just that, pls take a look. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 5/5] t4204-patch-id.sh: default is now stable
update test to match behaviour change Signed-off-by: Michael S. Tsirkin --- t/t4204-patch-id.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh index 7732370..a8b0c2c 100755 --- a/t/t4204-patch-id.sh +++ b/t/t4204-patch-id.sh @@ -112,8 +112,8 @@ test_expect_success 'file order is relevant with --unstable' ' ' #Now test various option combinations. -test_expect_success 'default is unstable' ' - test_patch_id relevant default +test_expect_success 'default is stable' ' + test_patch_id irrelevant default ' test_expect_success 'patchid.stable = true is stable' ' -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 3/5] patch-id-test: test stable and unstable behaviour
Verify that patch ID supports an algorithm
that is stable against diff split and reordering.
Signed-off-by: Michael S. Tsirkin
---
t/t4204-patch-id.sh | 102 ++--
1 file changed, 91 insertions(+), 11 deletions(-)
diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index d2c930d..7732370 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -5,27 +5,44 @@ test_description='git patch-id'
. ./test-lib.sh
test_expect_success 'setup' '
- test_commit initial foo a &&
- test_commit first foo b &&
- git checkout -b same HEAD^ &&
- test_commit same-msg foo b &&
- git checkout -b notsame HEAD^ &&
- test_commit notsame-msg foo c
+ as="a a a a a a a a" && # eight a
+ test_write_lines $as >foo &&
+ test_write_lines $as >bar &&
+ git add foo bar &&
+ git commit -a -m initial &&
+ test_write_lines $as b >foo &&
+ test_write_lines $as b >bar &&
+ git commit -a -m first &&
+ git checkout -b same master &&
+ git commit --amend -m same-msg &&
+ git checkout -b notsame master &&
+ echo c >foo &&
+ echo c >bar &&
+ git commit --amend -a -m notsame-msg &&
+ test_write_lines bar foo >bar-then-foo &&
+ test_write_lines foo bar >foo-then-bar
'
test_expect_success 'patch-id output is well-formed' '
- git log -p -1 | git patch-id > output &&
+ git log -p -1 | git patch-id >output &&
grep "^[a-f0-9]\{40\} $(git rev-parse HEAD)$" output
'
+#calculate patch id. Make sure output is not empty.
calc_patch_id () {
- git patch-id |
- sed "s# .*##" > patch-id_"$1"
+ name="$1"
+ shift
+ git patch-id "$@" |
+ sed "s/ .*//" >patch-id_"$name" &&
+ test_line_count -gt 0 patch-id_"$name"
+}
+
+get_top_diff () {
+ git log -p -1 "$@" -O bar-then-foo --
}
get_patch_id () {
- git log -p -1 "$1" | git patch-id |
- sed "s# .*##" > patch-id_"$1"
+ get_top_diff "$1" | calc_patch_id "$@"
}
test_expect_success 'patch-id detects equality' '
@@ -56,6 +73,69 @@ test_expect_success 'whitespace is irrelevant in footer' '
test_cmp patch-id_master patch-id_same
'
+cmp_patch_id () {
+ if
+ test "$1" = "relevant"
+ then
+ ! test_cmp patch-id_"$2" patch-id_"$3"
+ else
+ test_cmp patch-id_"$2" patch-id_"$3"
+ fi
+}
+
+test_patch_id_file_order () {
+ relevant="$1"
+ shift
+ name="order-${1}-$relevant"
+ shift
+ get_top_diff "master" | calc_patch_id "$name" "$@" &&
+ git checkout same &&
+ git format-patch -1 --stdout -O foo-then-bar |
+ calc_patch_id "ordered-$name" "$@" &&
+ cmp_patch_id $relevant "$name" "ordered-$name"
+
+}
+
+# combined test for options: add more tests here to make them
+# run with all options
+test_patch_id () {
+ test_patch_id_file_order "$@"
+}
+
+# small tests with detailed diagnostic for basic options.
+test_expect_success 'file order is irrelevant with --stable' '
+ test_patch_id_file_order irrelevant --stable --stable
+'
+
+test_expect_success 'file order is relevant with --unstable' '
+ test_patch_id_file_order relevant --unstable --unstable
+'
+
+#Now test various option combinations.
+test_expect_success 'default is unstable' '
+ test_patch_id relevant default
+'
+
+test_expect_success 'patchid.stable = true is stable' '
+ test_config patchid.stable true &&
+ test_patch_id irrelevant patchid.stable=true
+'
+
+test_expect_success 'patchid.stable = false is unstable' '
+ test_config patchid.stable false &&
+ test_patch_id relevant patchid.stable=false
+'
+
+test_expect_success '--unstable overrides patchid.stable = true' '
+ test_config patchid.stable true &&
+ test_patch_id relevant patchid.stable=true--unstable --unstable
+'
+
+test_expect_success '--stable overrides patchid.stable = false' '
+ test_config patchid.stable false &&
+ test_patch_id irrelevant patchid.stable=false--stable --stable
+'
+
test_expect_success 'patch-id supports git-format-patch MIME output' '
get_patch_id master &&
git checkout same &&
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 4/5] patch-id: change default to stable
--stable has been the default in 'next' for a few weeks with no ill effects. Change the default to that so that users don't have to remember to enable it. Update documentation to match behaviour change. Signed-off-by: Michael S. Tsirkin --- builtin/patch-id.c | 4 ++-- Documentation/git-patch-id.txt | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/patch-id.c b/builtin/patch-id.c index 77db873..e11a6a7 100644 --- a/builtin/patch-id.c +++ b/builtin/patch-id.c @@ -185,9 +185,9 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix) git_config(git_patch_id_config, &stable); - /* If nothing is set, default to unstable. */ + /* If nothing is set, default to stable. */ if (stable < 0) - stable = 0; + stable = 1; if (argc == 2 && !strcmp(argv[1], "--stable")) stable = 1; diff --git a/Documentation/git-patch-id.txt b/Documentation/git-patch-id.txt index fa562d3..1e2ca75 100644 --- a/Documentation/git-patch-id.txt +++ b/Documentation/git-patch-id.txt @@ -43,7 +43,7 @@ OPTIONS of "-O", thereby making existing databases storing such "unstable" or historical patch-ids unusable. - This is the default if patchid.stable is set to true. + This is the default. --unstable:: Use an "unstable" hash as the patch ID. With this option, @@ -52,7 +52,7 @@ OPTIONS patch-ids produced by git 1.9 and older (who do not deal with reordered patches) may want to use this option. - This is the default. + This is the default if patchid.stable is set to false. :: The diff to create the ID of. -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 2/5] test: add test_write_lines helper
API and implementation as suggested by Junio.
Signed-off-by: Michael S. Tsirkin
---
t/README| 22 ++
t/test-lib-functions.sh | 5 +
2 files changed, 27 insertions(+)
diff --git a/t/README b/t/README
index caeeb9d..2d6232f 100644
--- a/t/README
+++ b/t/README
@@ -596,6 +596,28 @@ library for your script to use.
...
'
+ - test_write_lines
+
+ Split to white-space separated words and write it out on standard
+ output, one word per line.
+ Useful to prepare multi-line files in a compact form.
+
+ Example:
+
+ test_write_lines "a b c d e f g" >foo
+
+ Is a more compact equivalent of:
+ cat >foo <<-EOF
+ a
+ b
+ c
+ d
+ e
+ f
+ g
+ EOF
+
+
- test_pause
This command is useful for writing and debugging tests and must be
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 158e10a..f581535 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -717,6 +717,11 @@ test_ln_s_add () {
fi
}
+# This function writes out its parameters, one per line
+test_write_lines () {
+ printf "%s\n" "$@"
+}
+
perl () {
command "$PERL_PATH" "$@"
}
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 1/5] patch-id: make it stable against hunk reordering
Patch id changes if users reorder file diffs that make up a patch.
As the result is functionally equivalent, a different patch id is
surprising to many users.
In particular, reordering files using diff -O is helpful to make patches
more readable (e.g. API header diff before implementation diff).
Add an option to change patch-id behaviour making it stable against
these kinds of patch change:
calculate SHA1 hash for each hunk separately and sum all hashes
(using a symmetrical sum) to get patch id
We use a 20byte sum and not xor - since xor would give 0 output
for patches that have two identical diffs, which isn't all that
unlikely (e.g. append the same line in two places).
The new behaviour is enabled
- when patchid.stable is true
- when --stable flag is present
Using a new flag --unstable or setting patchid.stable to false force
the historical behaviour.
In the documentation, clarify that patch ID can now be a sum of hashes,
not a hash.
Document how command line and config options affect the
behaviour.
Signed-off-by: Michael S. Tsirkin
---
builtin/patch-id.c | 74 +-
Documentation/git-patch-id.txt | 37 ++---
2 files changed, 91 insertions(+), 20 deletions(-)
diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 3cfe02d..77db873 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -1,17 +1,14 @@
#include "builtin.h"
-static void flush_current_id(int patchlen, unsigned char *id, git_SHA_CTX *c)
+static void flush_current_id(int patchlen, unsigned char *id, unsigned char
*result)
{
- unsigned char result[20];
char name[50];
if (!patchlen)
return;
- git_SHA1_Final(result, c);
memcpy(name, sha1_to_hex(id), 41);
printf("%s %s\n", sha1_to_hex(result), name);
- git_SHA1_Init(c);
}
static int remove_space(char *line)
@@ -56,10 +53,31 @@ static int scan_hunk_header(const char *p, int *p_before,
int *p_after)
return 1;
}
-static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct
strbuf *line_buf)
+static void flush_one_hunk(unsigned char *result, git_SHA_CTX *ctx)
+{
+ unsigned char hash[20];
+ unsigned short carry = 0;
+ int i;
+
+ git_SHA1_Final(hash, ctx);
+ git_SHA1_Init(ctx);
+ /* 20-byte sum, with carry */
+ for (i = 0; i < 20; ++i) {
+ carry += result[i] + hash[i];
+ result[i] = carry;
+ carry >>= 8;
+ }
+}
+
+static int get_one_patchid(unsigned char *next_sha1, unsigned char *result,
+ struct strbuf *line_buf, int stable)
{
int patchlen = 0, found_next = 0;
int before = -1, after = -1;
+ git_SHA_CTX ctx;
+
+ git_SHA1_Init(&ctx);
+ hashclr(result);
while (strbuf_getwholeline(line_buf, stdin, '\n') != EOF) {
char *line = line_buf->buf;
@@ -107,6 +125,8 @@ static int get_one_patchid(unsigned char *next_sha1,
git_SHA_CTX *ctx, struct st
break;
/* Else we're parsing another header. */
+ if (stable)
+ flush_one_hunk(result, &ctx);
before = after = -1;
}
@@ -119,39 +139,63 @@ static int get_one_patchid(unsigned char *next_sha1,
git_SHA_CTX *ctx, struct st
/* Compute the sha without whitespace */
len = remove_space(line);
patchlen += len;
- git_SHA1_Update(ctx, line, len);
+ git_SHA1_Update(&ctx, line, len);
}
if (!found_next)
hashclr(next_sha1);
+ flush_one_hunk(result, &ctx);
+
return patchlen;
}
-static void generate_id_list(void)
+static void generate_id_list(int stable)
{
- unsigned char sha1[20], n[20];
- git_SHA_CTX ctx;
+ unsigned char sha1[20], n[20], result[20];
int patchlen;
struct strbuf line_buf = STRBUF_INIT;
- git_SHA1_Init(&ctx);
hashclr(sha1);
while (!feof(stdin)) {
- patchlen = get_one_patchid(n, &ctx, &line_buf);
- flush_current_id(patchlen, sha1, &ctx);
+ patchlen = get_one_patchid(n, result, &line_buf, stable);
+ flush_current_id(patchlen, sha1, result);
hashcpy(sha1, n);
}
strbuf_release(&line_buf);
}
-static const char patch_id_usage[] = "git patch-id < patch";
+static const char patch_id_usage[] = "git patch-id [--stable | --unstable] <
patch";
+
+static int git_patch_id_config(const char *var, const char *value, void *cb)
+{
+ int *stable = cb;
+
+ if (!strcmp(var, "patchid.stable")) {
+ *stable = git_config_bool(var, value);
+
Re: [PATCH v5 3/9] tests: new test for orderfile options
On Thu, Apr 24, 2014 at 11:45:35AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" writes:
>
> > The test is very basic and can be extended.
> > Couldn't find a good existing place to put it,
> > so created a new file.
> >
> > Signed-off-by: Michael S. Tsirkin
> > ---
> > t/t4056-diff-order.sh | 63
> > +++
> > 1 file changed, 63 insertions(+)
> > create mode 100755 t/t4056-diff-order.sh
> >
> > diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
> > new file mode 100755
> > index 000..0404f50
>
> Huh? What codebase is this based on?
>
> I think we had t4056 since b5277730 (t4056: add new tests for "git
> diff -O", 2013-12-18).
>
> Puzzled...
Yes I didn't rebase in a while: 7794a680e63a2a11b73cb1194653662f2769a792
was the tip.
I'll rebase, sorry.
> > --- /dev/null
> > +++ b/t/t4056-diff-order.sh
> > @@ -0,0 +1,63 @@
> > +#!/bin/sh
> > +
> > +test_description='diff orderfile'
> > +
> > +. ./test-lib.sh
> > +
> > +test_expect_success 'setup' '
> > + as="a a a a a a a a" && # eight a
> > + test_write_lines $as >foo &&
> > + test_write_lines $as >bar &&
> > + git add foo bar &&
> > + git commit -a -m initial &&
> > + test_write_lines $as b >foo &&
> > + test_write_lines $as b >bar &&
> > + git commit -a -m first &&
> > + test_write_lines bar foo >bar-then-foo &&
> > + test_write_lines foo bar >foo-then-bar &&
> > + git diff -Ofoo-then-bar HEAD~1..HEAD >diff-foo-then-bar &&
> > + git diff -Obar-then-foo HEAD~1..HEAD >diff-bar-then-foo
> > +'
> > +
> > +test_diff_well_formed () {
> > + grep ^+b "$1" >added
> > + grep ^-b "$1" >removed
> > + grep ^+++ "$1" >oldfiles
> > + grep ^--- "$1" >newfiles
> > + test_line_count = 2 added &&
> > + test_line_count = 0 removed &&
> > + test_line_count = 2 oldfiles &&
> > + test_line_count = 2 newfiles
> > +}
> > +
> > +test_expect_success 'diff output with -O is well-formed' '
> > + test_diff_well_formed diff-foo-then-bar &&
> > + test_diff_well_formed diff-bar-then-foo
> > +'
> > +
> > +test_expect_success 'flag -O affects diff output' '
> > + ! test_cmp diff-foo-then-bar diff-bar-then-foo
> > +'
> > +
> > +test_expect_success 'orderfile is same as -O' '
> > + test_config diff.orderfile foo-then-bar &&
> > + git diff HEAD~1..HEAD >diff-foo-then-bar-config &&
> > + test_config diff.orderfile bar-then-foo &&
> > + git diff HEAD~1..HEAD >diff-bar-then-foo-config &&
> > + test_cmp diff-foo-then-bar diff-foo-then-bar-config &&
> > + test_cmp diff-bar-then-foo diff-bar-then-foo-config
> > +'
> > +
> > +test_expect_success '-O overrides orderfile' '
> > + test_config diff.orderfile foo-then-bar &&
> > + git diff -Obar-then-foo HEAD~1..HEAD >diff-bar-then-foo-flag &&
> > + test_cmp diff-bar-then-foo diff-bar-then-foo-flag
> > +'
> > +
> > +test_expect_success '/dev/null is same as no orderfile' '
> > + git diff -O/dev/null HEAD~1..HEAD>diff-null-orderfile &&
> > + git diff HEAD~1..HEAD >diff-default &&
> > + test_cmp diff-null-orderfile diff-default
> > +'
> > +
> > +test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/9] patch-id: make it stable against hunk reordering
On Thu, Apr 24, 2014 at 10:30:44AM -0700, Jonathan Nieder wrote: > Hi, > > Michael S. Tsirkin wrote: > > > Patch id changes if users > > 1. reorder file diffs that make up a patch > > or > > 2. split a patch up to multiple diffs that touch the same path > > (keeping hunks within a single diff ordered to make patch valid). > > > > As the result is functionally equivalent, a different patch id is > > surprising to many users. > > Hm. > > If the goal is that functionally equivalent patches are guaranteed to > produce the same patch-id, I wonder if we should be doing something > like the following: > > 1. apply the patch in memory > 2. generate a new diff > 3. use that new diff to produce a patch-id > > Otherwise issues like --diff-algorithm=patience versus =myers will > create trouble too. I don't think that avoiding false negatives for > patch comparison without doing something like that is really possible. > > On the other hand if someone reorders file diffs within a patch, that > is a potentially very common thing to do and something worth fixing. > In other words, while your (1) makes perfect sense to me, case (2) > seems less convincing. I agree it's less convincing: one would have to edit patch by hand (which I used to sometimes do to make important parts more prominent, but stopped doing in favor of splitting a patch). I'm not 100% sure whether it's worth supporting or not. > The downside of allowing reordering hunks is that it can potentially > make different patches to be treated the same (for example if they > were making similar changes to different functions) when the ordering > previously caused them to be distinguished. But that wasn't something > people could count on anyway, so I don't mind. I think this example convinces me. I'll drop this support in the next version. > Should the internal patch-id computation used by commands like 'git > cherry' (see diff.c::diff_get_patch_id) get the same change? (Not a > rhetorical question --- I don't know what the right choice would be > there.) > > [...] > > The new behaviour is enabled > > - when patchid.stable is true > > - when --stable flag is present > > > > Using a new flag --unstable or setting patchid.stable to false force > > the historical behaviour. > > Which is the default? > > [...] > > builtin/patch-id.c | 89 > > -- > > 1 file changed, 73 insertions(+), 16 deletions(-) > > Documentation? Tests? > > Thanks, > Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 5/9] patch-id: document new behaviour
On Thu, Apr 24, 2014 at 10:33:25AM -0700, Jonathan Nieder wrote: > Michael S. Tsirkin wrote: > > > Documentation/git-patch-id.txt | 23 ++- > > 1 file changed, 18 insertions(+), 5 deletions(-) > > Ah, there's the documentation. Please squash this with the patch that > introduces the new behavior so they can be reviewed together more > easily (both now and later when people do archeology). > > [...] > > +--stable:: > > + Use a symmetrical sum of hashes as the patch ID. > > + With this option, reordering file diffs that make up a patch or > > + splitting a diff up to multiple diffs that touch the same path > > + does not affect the ID. > > + This is the default if patchid.stable is set to true. > > This doesn't explain to me why I would want to use --stable versus > --unstable. Maybe an EXAMPLES section would help? > > The only reason I can think of to use --unstable is for compatibility > with historical patch-ids. Is there any other reason? > > At this point in the series there is no patchid.stable configuration. > > > +--unstable:: > > + Use a non-symmetrical sum of hashes, such that reordering > > What is a non-symmetrical sum? Non-symmetrical combination function is better? > Thanks, > Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 3/9] tests: new test for orderfile options
The test is very basic and can be extended.
Couldn't find a good existing place to put it,
so created a new file.
Signed-off-by: Michael S. Tsirkin
---
t/t4056-diff-order.sh | 63 +++
1 file changed, 63 insertions(+)
create mode 100755 t/t4056-diff-order.sh
diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
new file mode 100755
index 000..0404f50
--- /dev/null
+++ b/t/t4056-diff-order.sh
@@ -0,0 +1,63 @@
+#!/bin/sh
+
+test_description='diff orderfile'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ as="a a a a a a a a" && # eight a
+ test_write_lines $as >foo &&
+ test_write_lines $as >bar &&
+ git add foo bar &&
+ git commit -a -m initial &&
+ test_write_lines $as b >foo &&
+ test_write_lines $as b >bar &&
+ git commit -a -m first &&
+ test_write_lines bar foo >bar-then-foo &&
+ test_write_lines foo bar >foo-then-bar &&
+ git diff -Ofoo-then-bar HEAD~1..HEAD >diff-foo-then-bar &&
+ git diff -Obar-then-foo HEAD~1..HEAD >diff-bar-then-foo
+'
+
+test_diff_well_formed () {
+ grep ^+b "$1" >added
+ grep ^-b "$1" >removed
+ grep ^+++ "$1" >oldfiles
+ grep ^--- "$1" >newfiles
+ test_line_count = 2 added &&
+ test_line_count = 0 removed &&
+ test_line_count = 2 oldfiles &&
+ test_line_count = 2 newfiles
+}
+
+test_expect_success 'diff output with -O is well-formed' '
+ test_diff_well_formed diff-foo-then-bar &&
+ test_diff_well_formed diff-bar-then-foo
+'
+
+test_expect_success 'flag -O affects diff output' '
+ ! test_cmp diff-foo-then-bar diff-bar-then-foo
+'
+
+test_expect_success 'orderfile is same as -O' '
+ test_config diff.orderfile foo-then-bar &&
+ git diff HEAD~1..HEAD >diff-foo-then-bar-config &&
+ test_config diff.orderfile bar-then-foo &&
+ git diff HEAD~1..HEAD >diff-bar-then-foo-config &&
+ test_cmp diff-foo-then-bar diff-foo-then-bar-config &&
+ test_cmp diff-bar-then-foo diff-bar-then-foo-config
+'
+
+test_expect_success '-O overrides orderfile' '
+ test_config diff.orderfile foo-then-bar &&
+ git diff -Obar-then-foo HEAD~1..HEAD >diff-bar-then-foo-flag &&
+ test_cmp diff-bar-then-foo diff-bar-then-foo-flag
+'
+
+test_expect_success '/dev/null is same as no orderfile' '
+ git diff -O/dev/null HEAD~1..HEAD>diff-null-orderfile &&
+ git diff HEAD~1..HEAD >diff-default &&
+ test_cmp diff-null-orderfile diff-default
+'
+
+test_done
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 1/9] diff: add a config option to control orderfile
I always want my diffs to show header files first, then .c files, then the rest. Make it possible to set orderfile though a config option to achieve this. Signed-off-by: Michael S. Tsirkin --- diff.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/diff.c b/diff.c index b79432b..6bcb26b 100644 --- a/diff.c +++ b/diff.c @@ -264,6 +264,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb) return 0; } + if (!strcmp(var, "diff.orderfile")) + return git_config_string(&default_diff_options.orderfile, var, value); + if (starts_with(var, "submodule.")) return parse_submodule_config_option(var, value); -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 7/9] patch-id: change default to stable
--stable has been the default in 'next' for a few weeks with no ill effects. Change the default to that so that users don't have to remember to enable it. Signed-off-by: Michael S. Tsirkin --- builtin/patch-id.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/patch-id.c b/builtin/patch-id.c index 037cf2f..0b267af 100644 --- a/builtin/patch-id.c +++ b/builtin/patch-id.c @@ -198,9 +198,9 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix) git_config(git_patch_id_config, &stable); - /* If nothing is set, default to unstable. */ + /* If nothing is set, default to stable. */ if (stable < 0) - stable = 0; + stable = 1; if (argc == 2 && !strcmp(argv[1], "--stable")) stable = 1; -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 2/9] test: add test_write_lines helper
API and implementation as suggested by Junio.
Signed-off-by: Michael S. Tsirkin
---
t/test-lib-functions.sh | 5 +
1 file changed, 5 insertions(+)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index aeae3ca..0e21275 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -712,6 +712,11 @@ test_ln_s_add () {
fi
}
+# This function writes out its parameters, one per line
+test_write_lines () {
+ printf "%s\n" "$@";
+}
+
perl () {
command "$PERL_PATH" "$@"
}
--
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 4/9] patch-id: make it stable against hunk reordering
Patch id changes if users
1. reorder file diffs that make up a patch
or
2. split a patch up to multiple diffs that touch the same path
(keeping hunks within a single diff ordered to make patch valid).
As the result is functionally equivalent, a different patch id is
surprising to many users.
In particular, reordering files using diff -O is helpful to make patches
more readable (e.g. API header diff before implementation diff).
Add an option to change patch-id behaviour making it stable against
these two kinds of patch change:
1. calculate SHA1 hash for each hunk separately and sum all hashes
(using a symmetrical sum) to get patch id
2. hash the file-level headers together with each hunk (not just the
first hunk)
We use a 20byte sum and not xor - since xor would give 0 output
for patches that have two identical diffs, which isn't all that
unlikely (e.g. append the same line in two places).
The new behaviour is enabled
- when patchid.stable is true
- when --stable flag is present
Using a new flag --unstable or setting patchid.stable to false force
the historical behaviour.
Signed-off-by: Michael S. Tsirkin
---
builtin/patch-id.c | 89 --
1 file changed, 73 insertions(+), 16 deletions(-)
diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 3cfe02d..037cf2f 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -1,17 +1,14 @@
#include "builtin.h"
-static void flush_current_id(int patchlen, unsigned char *id, git_SHA_CTX *c)
+static void flush_current_id(int patchlen, unsigned char *id, unsigned char
*result)
{
- unsigned char result[20];
char name[50];
if (!patchlen)
return;
- git_SHA1_Final(result, c);
memcpy(name, sha1_to_hex(id), 41);
printf("%s %s\n", sha1_to_hex(result), name);
- git_SHA1_Init(c);
}
static int remove_space(char *line)
@@ -56,10 +53,31 @@ static int scan_hunk_header(const char *p, int *p_before,
int *p_after)
return 1;
}
-static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct
strbuf *line_buf)
+static void flush_one_hunk(unsigned char *result, git_SHA_CTX *ctx)
{
- int patchlen = 0, found_next = 0;
+ unsigned char hash[20];
+ unsigned short carry = 0;
+ int i;
+
+ git_SHA1_Final(hash, ctx);
+ git_SHA1_Init(ctx);
+ /* 20-byte sum, with carry */
+ for (i = 0; i < 20; ++i) {
+ carry += result[i] + hash[i];
+ result[i] = carry;
+ carry >>= 8;
+ }
+}
+
+static int get_one_patchid(unsigned char *next_sha1, unsigned char *result,
+ struct strbuf *line_buf, int stable)
+{
+ int patchlen = 0, found_next = 0, hunks = 0;
int before = -1, after = -1;
+ git_SHA_CTX ctx, header_ctx;
+
+ git_SHA1_Init(&ctx);
+ hashclr(result);
while (strbuf_getwholeline(line_buf, stdin, '\n') != EOF) {
char *line = line_buf->buf;
@@ -98,7 +116,19 @@ static int get_one_patchid(unsigned char *next_sha1,
git_SHA_CTX *ctx, struct st
if (before == 0 && after == 0) {
if (!memcmp(line, "@@ -", 4)) {
/* Parse next hunk, but ignore line numbers. */
+ if (stable) {
+ /* Hash the file-level headers together
with each hunk. */
+ if (hunks) {
+ flush_one_hunk(result, &ctx);
+ /* Prepend saved header ctx for
next hunk. */
+ memcpy(&ctx, &header_ctx,
sizeof(ctx));
+ } else {
+ /* Save header ctx for next
hunk. */
+ memcpy(&header_ctx, &ctx,
sizeof(ctx));
+ }
+ }
scan_hunk_header(line, &before, &after);
+ hunks++;
continue;
}
@@ -107,7 +137,10 @@ static int get_one_patchid(unsigned char *next_sha1,
git_SHA_CTX *ctx, struct st
break;
/* Else we're parsing another header. */
+ if (stable && hunks)
+ flush_one_hunk(result, &ctx);
before = after = -1;
+ hunks = 0;
}
/* If we get here, we're inside a hunk. */
@@ -119,39 +152,63 @@ static int get_one_patchid(unsigned char *next_sha1,
git_SHA_CTX *ctx, struct st
[PATCH v5 6/9] patch-id-test: test stable and unstable behaviour
Verify that patch ID supports an algorithm
that is stable against diff split and reordering.
Signed-off-by: Michael S. Tsirkin
---
t/t4204-patch-id.sh | 128 +++-
1 file changed, 117 insertions(+), 11 deletions(-)
diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index d2c930d..cd13e8e 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -5,27 +5,51 @@ test_description='git patch-id'
. ./test-lib.sh
test_expect_success 'setup' '
- test_commit initial foo a &&
- test_commit first foo b &&
- git checkout -b same HEAD^ &&
- test_commit same-msg foo b &&
- git checkout -b notsame HEAD^ &&
- test_commit notsame-msg foo c
+ as="a a a a a a a a" && # eight a
+ test_write_lines $as >foo &&
+ test_write_lines $as >bar &&
+ git add foo bar &&
+ git commit -a -m initial &&
+ test_write_lines $as b >foo &&
+ test_write_lines $as b >bar &&
+ git commit -a -m first &&
+ git checkout -b same master &&
+ git commit --amend -m same-msg &&
+ git checkout -b notsame master &&
+ echo c >foo &&
+ echo c >bar &&
+ git commit --amend -a -m notsame-msg &&
+ git checkout -b split master &&
+ test_write_lines d $as b >foo &&
+ test_write_lines d $as b >bar &&
+ git commit -a -m split &&
+ git checkout -b merged master &&
+ git checkout split -- foo bar &&
+ git commit --amend -a -m merged &&
+ test_write_lines bar foo >bar-then-foo &&
+ test_write_lines foo bar >foo-then-bar
'
test_expect_success 'patch-id output is well-formed' '
- git log -p -1 | git patch-id > output &&
+ git log -p -1 | git patch-id >output &&
grep "^[a-f0-9]\{40\} $(git rev-parse HEAD)$" output
'
+#calculate patch id. Make sure output is not empty.
calc_patch_id () {
- git patch-id |
- sed "s# .*##" > patch-id_"$1"
+ name="$1"
+ shift
+ git patch-id "$@" |
+ sed "s/ .*//" >patch-id_"$name" &&
+ test_line_count -gt 0 patch-id_"$name"
+}
+
+get_top_diff () {
+ git log -p -1 "$@" -O bar-then-foo --
}
get_patch_id () {
- git log -p -1 "$1" | git patch-id |
- sed "s# .*##" > patch-id_"$1"
+ get_top_diff "$1" | calc_patch_id "$@"
}
test_expect_success 'patch-id detects equality' '
@@ -56,6 +80,88 @@ test_expect_success 'whitespace is irrelevant in footer' '
test_cmp patch-id_master patch-id_same
'
+cmp_patch_id () {
+ if
+ test "$1" = "relevant"
+ then
+ ! test_cmp patch-id_"$2" patch-id_"$3"
+ else
+ test_cmp patch-id_"$2" patch-id_"$3"
+ fi
+}
+
+test_patch_id_file_order () {
+ relevant="$1"
+ shift
+ name="order-${1}-$relevant"
+ shift
+ get_top_diff "master" | calc_patch_id "$name" "$@" &&
+ git checkout same &&
+ git format-patch -1 --stdout -O foo-then-bar |
+ calc_patch_id "ordered-$name" "$@" &&
+ cmp_patch_id $relevant "$name" "ordered-$name"
+
+}
+
+test_patch_id_split () {
+ relevant="$1"
+ shift
+ name="split-${1}-$relevant"
+ shift
+ get_top_diff merged | calc_patch_id "$name" "$@" &&
+ (git log -p -1 -O foo-then-bar split~1; git diff split~1..split) |
+ calc_patch_id "split-$name" "$@" &&
+ cmp_patch_id "$relevant" "$name" "split-$name"
+}
+
+# combined test for options
+test_patch_id () {
+ test_patch_id_file_order "$@" &&
+ test_patch_id_split "$@"
+}
+
+# small tests with detailed diagnostic for basic options.
+test_expect_success 'file order is irrelevant with --stable' '
+ test_patch_id_file_order irrelevant --stable --stable
+'
+
+test_expect_success 'file order is relevant with --unstable' '
+ test_patch_id_file_order relevant --unstable --unstable
+'
+
+test_expect_success 'splitting patch is irrelevant with --stable' '
+ test_patch_id_split irrelevant --stable --stab
[PATCH v5 5/9] patch-id: document new behaviour
Clarify that patch ID can now be a sum of hashes, not a hash. Document how command line and config options affect the behaviour. Signed-off-by: Michael S. Tsirkin --- Documentation/git-patch-id.txt | 23 ++- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/Documentation/git-patch-id.txt b/Documentation/git-patch-id.txt index 312c3b1..e21b79b 100644 --- a/Documentation/git-patch-id.txt +++ b/Documentation/git-patch-id.txt @@ -8,14 +8,14 @@ git-patch-id - Compute unique ID for a patch SYNOPSIS [verse] -'git patch-id' < +'git patch-id' [--stable | --unstable] < DESCRIPTION --- -A "patch ID" is nothing but a SHA-1 of the diff associated with a patch, with -whitespace and line numbers ignored. As such, it's "reasonably stable", but at -the same time also reasonably unique, i.e., two patches that have the same "patch -ID" are almost guaranteed to be the same thing. +A "patch ID" is nothing but a sum of SHA-1 of the diff hunks associated with a +patch, with whitespace and line numbers ignored. As such, it's "reasonably +stable", but at the same time also reasonably unique, i.e., two patches that +have the same "patch ID" are almost guaranteed to be the same thing. IOW, you can use this thing to look for likely duplicate commits. @@ -27,6 +27,19 @@ This can be used to make a mapping from patch ID to commit ID. OPTIONS --- + +--stable:: + Use a symmetrical sum of hashes as the patch ID. + With this option, reordering file diffs that make up a patch or + splitting a diff up to multiple diffs that touch the same path + does not affect the ID. + This is the default if patchid.stable is set to true. + +--unstable:: + Use a non-symmetrical sum of hashes, such that reordering + or splitting the patch does affect the ID. + This is the default. + :: The diff to create the ID of. -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 9/9] Documentation/git-patch-id.txt: default is stable
Update documentation to match behaviour change. Signed-off-by: Michael S. Tsirkin --- Documentation/git-patch-id.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/git-patch-id.txt b/Documentation/git-patch-id.txt index e21b79b..9299b90 100644 --- a/Documentation/git-patch-id.txt +++ b/Documentation/git-patch-id.txt @@ -33,12 +33,13 @@ OPTIONS With this option, reordering file diffs that make up a patch or splitting a diff up to multiple diffs that touch the same path does not affect the ID. - This is the default if patchid.stable is set to true. + This is the default. --unstable:: Use a non-symmetrical sum of hashes, such that reordering or splitting the patch does affect the ID. - This is the default. + This is the default if patchid.stable is set to false. + This was the default value for git 1.9 and older. :: The diff to create the ID of. -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 8/9] t4204-patch-id.sh: default is now stable
update test to match behaviour change Signed-off-by: Michael S. Tsirkin --- t/t4204-patch-id.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh index cd13e8e..03f91ce 100755 --- a/t/t4204-patch-id.sh +++ b/t/t4204-patch-id.sh @@ -138,8 +138,8 @@ test_expect_success 'splitting patch affects id with --unstable' ' ' #Now test various option combinations. -test_expect_success 'default is unstable' ' - test_patch_id relevant default +test_expect_success 'default is stable' ' + test_patch_id irrelevant default ' test_expect_success 'patchid.stable = true is stable' ' -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
