* [PATCH] Handle "git show" output correctly. @ 2012-09-12 15:26 Peter Jones 2012-09-12 15:30 ` Peter Jones 2012-09-12 15:40 ` Matthieu Moy 0 siblings, 2 replies; 21+ messages in thread From: Peter Jones @ 2012-09-12 15:26 UTC (permalink / raw) To: git; +Cc: Peter Jones Signed-off-by: Peter Jones <pjones@redhat.com> --- git-am.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/git-am.sh b/git-am.sh index c682d34..ebcbff7 100755 --- a/git-am.sh +++ b/git-am.sh @@ -216,6 +216,21 @@ check_patch_format () { read l2 read l3 case "$l1" in + "commit "*) + case "$l2" in + "Author: "*) + case "$l3" in + "Date: "*) + patch_format=gitshow + ;; + *) + ;; + esac + ;; + *) + ;; + esac + ;; "From "* | "From: "*) patch_format=mbox ;; @@ -321,6 +336,37 @@ split_patches () { this= msgnum= ;; + gitshow) + this=0 + for stgit in "$@" + do + this=`expr "$this" + 1` + msgnum=`printf "%0${prec}d" $this` + # Perl version of The first nonemptyline after an + # empty line is the subject, and the body starts with + # the next nonempty line. + perl -ne 'BEGIN { $subject = 0 } + if ($subject > 1) { print ; } + elsif (/^\s+$/) { next ; } + elsif (/^Author:/) { s/Author/From/ ; print ;} + elsif (/^(From|Date)/) { print ; } + elsif (/^commit/) { next ; } + elsif ($subject) { + $subject = 2 ; + print "\n" ; + s/^ // ; + print ; + } else { + print "Subject: ", $_ ; + $subject = 1; + } + ' < "$stgit" > "$dotest/$msgnum" || clean_abort + done + echo "$this" > "$dotest/last" + this= + msgnum= + ;; + hg) this=0 for hg in "$@" -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH] Handle "git show" output correctly. 2012-09-12 15:26 [PATCH] Handle "git show" output correctly Peter Jones @ 2012-09-12 15:30 ` Peter Jones 2012-09-12 15:41 ` Matthieu Moy 2012-09-12 15:42 ` [PATCH] " Peter Jones 2012-09-12 15:40 ` Matthieu Moy 1 sibling, 2 replies; 21+ messages in thread From: Peter Jones @ 2012-09-12 15:30 UTC (permalink / raw) To: git; +Cc: Peter Jones (this version with fixed tabs) Signed-off-by: Peter Jones <pjones@redhat.com> --- git-am.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/git-am.sh b/git-am.sh index c682d34..4a1a768 100755 --- a/git-am.sh +++ b/git-am.sh @@ -216,6 +216,21 @@ check_patch_format () { read l2 read l3 case "$l1" in + "commit "*) + case "$l2" in + "Author: "*) + case "$l3" in + "Date: "*) + patch_format=gitshow + ;; + *) + ;; + esac + ;; + *) + ;; + esac + ;; "From "* | "From: "*) patch_format=mbox ;; @@ -321,6 +336,36 @@ split_patches () { this= msgnum= ;; + gitshow) + this=0 + for stgit in "$@" + do + this=`expr "$this" + 1` + msgnum=`printf "%0${prec}d" $this` + # Perl version of The first nonemptyline after an + # empty line is the subject, and the body starts with + # the next nonempty line. + perl -ne 'BEGIN { $subject = 0 } + if ($subject > 1) { print ; } + elsif (/^\s+$/) { next ; } + elsif (/^Author:/) { s/Author/From/ ; print ;} + elsif (/^(From|Date)/) { print ; } + elsif (/^commit/) { next ; } + elsif ($subject) { + $subject = 2 ; + print "\n" ; + s/^ // ; + print ; + } else { + print "Subject: ", $_ ; + $subject = 1; + } + ' < "$stgit" > "$dotest/$msgnum" || clean_abort + done + echo "$this" > "$dotest/last" + this= + msgnum= + ;; hg) this=0 for hg in "$@" -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] Handle "git show" output correctly. 2012-09-12 15:30 ` Peter Jones @ 2012-09-12 15:41 ` Matthieu Moy 2012-09-12 15:49 ` [PATCH] [git-am] " Peter Jones 2012-09-12 15:42 ` [PATCH] " Peter Jones 1 sibling, 1 reply; 21+ messages in thread From: Matthieu Moy @ 2012-09-12 15:41 UTC (permalink / raw) To: Peter Jones; +Cc: git Peter Jones <pjones@redhat.com> writes: > (this version with fixed tabs) This will end up being the commit message. If you add text here, then the maintainer will have to manually fix it when applying (or reject your patch). Please, be nice with him and put your comments below the --- : > Signed-off-by: Peter Jones <pjones@redhat.com> > --- (ie. here) > git-am.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++ -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] [git-am] Handle "git show" output correctly 2012-09-12 15:41 ` Matthieu Moy @ 2012-09-12 15:49 ` Peter Jones 2012-09-12 15:57 ` Matthieu Moy 0 siblings, 1 reply; 21+ messages in thread From: Peter Jones @ 2012-09-12 15:49 UTC (permalink / raw) To: git; +Cc: Peter Jones This patch adds the ability for "git am" to accept patches in the format generated by "git show". Some people erroneously use "git show" instead of "git format-patch", and it's nice as a maintainer to be able to easily take their patch rather than going back and forth with them to get a "correctly" formatted patch containing exactly the same actual information. Signed-off-by: Peter Jones <pjones@redhat.com> --- git-am.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/git-am.sh b/git-am.sh index c682d34..cfd7b09 100755 --- a/git-am.sh +++ b/git-am.sh @@ -216,6 +216,21 @@ check_patch_format () { read l2 read l3 case "$l1" in + "commit "*) + case "$l2" in + "Author: "*) + case "$l3" in + "Date: "*) + patch_format=gitshow + ;; + *) + ;; + esac + ;; + *) + ;; + esac + ;; "From "* | "From: "*) patch_format=mbox ;; @@ -321,6 +336,36 @@ split_patches () { this= msgnum= ;; + gitshow) + this=0 + for stgit in "$@" + do + this=`expr "$this" + 1` + msgnum=`printf "%0${prec}d" $this` + # The first nonemptyline after an empty line is the + # subject, and the body starts with the next nonempty + # line. + perl -ne 'BEGIN { $subject = 0 } + if ($subject > 1) { print ; } + elsif (/^\s+$/) { next ; } + elsif (/^Author:/) { s/Author/From/ ; print ;} + elsif (/^(From|Date)/) { print ; } + elsif (/^commit/) { next ; } + elsif ($subject) { + $subject = 2 ; + print "\n" ; + s/^ // ; + print ; + } else { + print "Subject: ", $_ ; + $subject = 1; + } + ' < "$stgit" > "$dotest/$msgnum" || clean_abort + done + echo "$this" > "$dotest/last" + this= + msgnum= + ;; hg) this=0 for hg in "$@" -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] [git-am] Handle "git show" output correctly 2012-09-12 15:49 ` [PATCH] [git-am] " Peter Jones @ 2012-09-12 15:57 ` Matthieu Moy 2012-09-12 17:32 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Matthieu Moy @ 2012-09-12 15:57 UTC (permalink / raw) To: Peter Jones; +Cc: git Peter Jones <pjones@redhat.com> writes: > Subject: [PATCH] [git-am] Handle "git show" output correctly The convention in Git is "<subsystem>: <summary of change>" (i.e. no brackets around git-am, just am: and no capital for Handle). My other concerns (name of stgit, multi-lines subject lines and lack of documentation) still hold. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] [git-am] Handle "git show" output correctly 2012-09-12 15:57 ` Matthieu Moy @ 2012-09-12 17:32 ` Junio C Hamano 2012-09-12 18:05 ` Peter Jones 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2012-09-12 17:32 UTC (permalink / raw) To: Matthieu Moy; +Cc: Peter Jones, git Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes: > Peter Jones <pjones@redhat.com> writes: > >> Subject: [PATCH] [git-am] Handle "git show" output correctly > > The convention in Git is "<subsystem>: <summary of change>" (i.e. no > brackets around git-am, just am: and no capital for Handle). > > My other concerns (name of stgit, multi-lines subject lines and lack of > documentation) still hold. We do not want to apply "git show" output that munges the log message, period. If you want to give patches to somebody (or to yourself) via e-mail or via sneaker-net, "git format-patch" is there for you. Do not butcher "am" to accept a format that is not meant for patch transport in the first place. If you want to screw something in to your shelf, you would use a screw and a screwdriver. You do not try to hammer a nail using your screwdriver, find that the screwdriver is not very useful as a hammer and modify the screwdriver to hit your nail. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] [git-am] Handle "git show" output correctly 2012-09-12 17:32 ` Junio C Hamano @ 2012-09-12 18:05 ` Peter Jones 2012-09-12 19:07 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Peter Jones @ 2012-09-12 18:05 UTC (permalink / raw) To: Junio C Hamano; +Cc: Matthieu Moy, git On Wed, 2012-09-12 at 10:32 -0700, Junio C Hamano wrote: > We do not want to apply "git show" output that munges the log > message, period. > > If you want to give patches to somebody (or to yourself) via e-mail > or via sneaker-net, "git format-patch" is there for you. Do not > butcher "am" to accept a format that is not meant for patch > transport in the first place. > > If you want to screw something in to your shelf, you would use a > screw and a screwdriver. You do not try to hammer a nail using your > screwdriver, find that the screwdriver is not very useful as a > hammer and modify the screwdriver to hit your nail. That seems to be completely missing the point - people /send/ them without knowing, and as a maintainer of several projects, it's /hostile/ to people who are trying to help by sending patches to go around in circles with them about the fact that they typed the wrong command. I'd rather just take the patch, but right now the tools won't let me, and for completely arbitrary reasons. Let me put it a different way - if you won't accept git-am handling "git show" output because "git show" has output that wasn't designed to be parsed ever, would you be opposed to a patch that switches the "git show" output to be something usable? -- Peter ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] [git-am] Handle "git show" output correctly 2012-09-12 18:05 ` Peter Jones @ 2012-09-12 19:07 ` Junio C Hamano 0 siblings, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2012-09-12 19:07 UTC (permalink / raw) To: Peter Jones; +Cc: Matthieu Moy, git Peter Jones <pjones@redhat.com> writes: > Let me put it a different way - if you won't accept git-am handling "git > show" output because "git show" has output that wasn't designed to be > parsed ever, would you be opposed to a patch that switches the "git > show" output to be something usable? The output from the command is optimized for humans, but you could invoke "git show --pretty=email" if you want to, so I do not think you need any patch to do that. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] Handle "git show" output correctly. 2012-09-12 15:30 ` Peter Jones 2012-09-12 15:41 ` Matthieu Moy @ 2012-09-12 15:42 ` Peter Jones 1 sibling, 0 replies; 21+ messages in thread From: Peter Jones @ 2012-09-12 15:42 UTC (permalink / raw) To: git; +Cc: Peter Jones (this version with fixed tabs and the comment fixed to be actual English) Signed-off-by: Peter Jones <pjones@redhat.com> --- git-am.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/git-am.sh b/git-am.sh index c682d34..cfd7b09 100755 --- a/git-am.sh +++ b/git-am.sh @@ -216,6 +216,21 @@ check_patch_format () { read l2 read l3 case "$l1" in + "commit "*) + case "$l2" in + "Author: "*) + case "$l3" in + "Date: "*) + patch_format=gitshow + ;; + *) + ;; + esac + ;; + *) + ;; + esac + ;; "From "* | "From: "*) patch_format=mbox ;; @@ -321,6 +336,36 @@ split_patches () { this= msgnum= ;; + gitshow) + this=0 + for stgit in "$@" + do + this=`expr "$this" + 1` + msgnum=`printf "%0${prec}d" $this` + # The first nonemptyline after an empty line is the + # subject, and the body starts with the next nonempty + # line. + perl -ne 'BEGIN { $subject = 0 } + if ($subject > 1) { print ; } + elsif (/^\s+$/) { next ; } + elsif (/^Author:/) { s/Author/From/ ; print ;} + elsif (/^(From|Date)/) { print ; } + elsif (/^commit/) { next ; } + elsif ($subject) { + $subject = 2 ; + print "\n" ; + s/^ // ; + print ; + } else { + print "Subject: ", $_ ; + $subject = 1; + } + ' < "$stgit" > "$dotest/$msgnum" || clean_abort + done + echo "$this" > "$dotest/last" + this= + msgnum= + ;; hg) this=0 for hg in "$@" -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] Handle "git show" output correctly. 2012-09-12 15:26 [PATCH] Handle "git show" output correctly Peter Jones 2012-09-12 15:30 ` Peter Jones @ 2012-09-12 15:40 ` Matthieu Moy 2012-09-12 18:00 ` Peter Jones 1 sibling, 1 reply; 21+ messages in thread From: Matthieu Moy @ 2012-09-12 15:40 UTC (permalink / raw) To: Peter Jones; +Cc: git > Subject: Re: [PATCH] Handle "git show" output correctly. No final period please. This does not say which part of git is made to handle "git show". What about [PATCH] am: handle "git show" output correctly Peter Jones <pjones@redhat.com> writes: This lacks a proper commit message, i.e. an answer to the "why is this change good?" question. > Signed-off-by: Peter Jones <pjones@redhat.com> > --- > git-am.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++ Documentation? > --- a/git-am.sh > +++ b/git-am.sh > @@ -216,6 +216,21 @@ check_patch_format () { > read l2 > read l3 > case "$l1" in > + "commit "*) > + case "$l2" in > + "Author: "*) > + case "$l3" in > + "Date: "*) > + patch_format=gitshow > + ;; > + *) > + ;; > + esac > + ;; > + *) > + ;; > + esac > + ;; Your code is indented with space, Git indents with tabs. Please fix this in your next version. > patch_format=mbox > ;; > @@ -321,6 +336,37 @@ split_patches () { > this= > msgnum= > ;; > + gitshow) > + this=0 > + for stgit in "$@" Probably a cut-and-paste from the stgit version, but your variable naming doesn't make sense here. > + do > + this=`expr "$this" + 1` > + msgnum=`printf "%0${prec}d" $this` > + # Perl version of The first nonemptyline after an Wrong cut-and-paste again, the sentense doesn't parse. > + # empty line is the subject, and the body starts with > + # the next nonempty line. > + perl -ne 'BEGIN { $subject = 0 } > + if ($subject > 1) { print ; } > + elsif (/^\s+$/) { next ; } > + elsif (/^Author:/) { s/Author/From/ ; print ;} > + elsif (/^(From|Date)/) { print ; } > + elsif (/^commit/) { next ; } > + elsif ($subject) { > + $subject = 2 ; > + print "\n" ; > + s/^ // ; > + print ; > + } else { > + print "Subject: ", $_ ; > + $subject = 1; > + } How does this react to multi-line subject, e.g This should be the subject line. And this is the body. ? git format-patch will merge the lines in a single Subject: header, and your version seems to take only the first line. A test showing this would be welcome. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Handle "git show" output correctly. 2012-09-12 15:40 ` Matthieu Moy @ 2012-09-12 18:00 ` Peter Jones 2012-09-12 18:08 ` [PATCH] git-am: " Peter Jones 0 siblings, 1 reply; 21+ messages in thread From: Peter Jones @ 2012-09-12 18:00 UTC (permalink / raw) To: Matthieu Moy; +Cc: git [-- Attachment #1: Type: text/plain, Size: 981 bytes --] On Wed, 2012-09-12 at 17:40 +0200, Matthieu Moy wrote: > > How does this react to multi-line subject, e.g > > This should be the > subject line. > > And this is the body. > > ? > > git format-patch will merge the lines in a single Subject: header, and > your version seems to take only the first line. > > A test showing this would be welcome. An updated patch to fix this will be my next mail. It's not as succinct as it once was, but such is life. The two attached commits to this message can be used as a test case. Basically, do (in any repo) git am 0001* 0002* git show > foo.patch git reset HEAD^ --hard git am foo.patch git show # check the output here git format-patch -1 # check 0001-bar-bar-bar-this-is-a-very-very-long-line-I-am-tired.patch # here. It winds up merging the subject lines before the rest of git-am does - I couldn't get it to work if I preserved the newline; for some reason I always get a second newline and that's /more/ wrong. -- Peter [-- Attachment #2: 0001-this-is-an-example-with-a-very-long-subject-line-whi.patch --] [-- Type: text/x-patch, Size: 514 bytes --] >From f7521f88731f9fc696dcd8e32de58cc9d98ed892 Mon Sep 17 00:00:00 2001 From: Peter Jones <pjones@redhat.com> Date: Wed, 12 Sep 2012 13:17:11 -0400 Subject: [PATCH 1/2] this is an example with a very long subject line which is completely unreasonable and nevertheless a thing. It also has other stuff here. --- foo | 1 + 1 file changed, 1 insertion(+) create mode 100644 foo diff --git a/foo b/foo new file mode 100644 index 0000000..fa5ef85 --- /dev/null +++ b/foo @@ -0,0 +1 @@ + za za za -- 1.7.11.4 [-- Attachment #3: 0002-bar-bar-bar-this-is-a-very-very-long-line-I-am-tired.patch --] [-- Type: text/x-patch, Size: 506 bytes --] >From bc471f2b89ada6e6ddf35b5ec2538242b5639836 Mon Sep 17 00:00:00 2001 From: Peter Jones <pjones@redhat.com> Date: Wed, 12 Sep 2012 13:39:56 -0400 Subject: [PATCH 2/2] bar bar bar this is a very very long line I am tired of this game and it is quite annoying. this is really annoying. I hate perl. zonk. yes. no. --- foo | 1 + 1 file changed, 1 insertion(+) diff --git a/foo b/foo index fa5ef85..3b572f4 100644 --- a/foo +++ b/foo @@ -1 +1,2 @@ za za za + bar bar bar -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH] git-am: Handle "git show" output correctly 2012-09-12 18:00 ` Peter Jones @ 2012-09-12 18:08 ` Peter Jones 2012-09-12 20:06 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Peter Jones @ 2012-09-12 18:08 UTC (permalink / raw) To: git; +Cc: Peter Jones This patch adds the ability for "git am" to accept patches in the format generated by "git show". Some people erroneously use "git show" instead of "git format-patch", and it's nice as a maintainer to be able to easily take their patch rather than going back and forth with them to get a "correctly" formatted patch containing exactly the same actual information. Signed-off-by: Peter Jones <pjones@redhat.com> --- git-am.sh | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/git-am.sh b/git-am.sh index c682d34..210e9fe 100755 --- a/git-am.sh +++ b/git-am.sh @@ -216,6 +216,21 @@ check_patch_format () { read l2 read l3 case "$l1" in + "commit "*) + case "$l2" in + "Author: "*) + case "$l3" in + "Date: "*) + patch_format=gitshow + ;; + *) + ;; + esac + ;; + *) + ;; + esac + ;; "From "* | "From: "*) patch_format=mbox ;; @@ -321,6 +336,51 @@ split_patches () { this= msgnum= ;; + gitshow) + this=0 + for patch in "$@" + do + this=`expr "$this" + 1` + msgnum=`printf "%0${prec}d" $this` + # The first nonemptyline after an empty line is the + # subject, and the body starts with the next nonempty + # line. + perl -ne 'BEGIN { + $diff = 0; $subject = 0; $subjtext=""; + } + if ($diff == 1 || /^diff/ || /^---$/) { + $diff = 1 ; + print ; + } elsif ($subject > 1) { + s/^ // ; + print ; + } elsif ($subject == 1 && !/^\s+$/) { + s/^ // ; + $subjtext = "$subjtext $_"; + } elsif ($subject == 1) { + $subject = 2 ; + print "Subject: ", $subjtext ; + s/^ // ; + print ; + } elsif ($subject) { + print "\n" ; + s/^ // ; + print ; + } elsif (/^\s+$/) { next ; } + elsif (/^Author:/) { s/Author/From/ ; print ;} + elsif (/^(From|Date)/) { print ; } + elsif (/^commit/) { next ; } + else { + s/^ // ; + $subjtext = $_; + $subject = 1; + } + ' < "$patch" > "$dotest/$msgnum" || clean_abort + done + echo "$this" > "$dotest/last" + this= + msgnum= + ;; hg) this=0 for hg in "$@" -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] git-am: Handle "git show" output correctly 2012-09-12 18:08 ` [PATCH] git-am: " Peter Jones @ 2012-09-12 20:06 ` Junio C Hamano 2012-09-12 20:48 ` Peter Jones 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2012-09-12 20:06 UTC (permalink / raw) To: Peter Jones; +Cc: git Peter Jones <pjones@redhat.com> writes: > This patch adds the ability for "git am" to accept patches in the format > generated by "git show". Some people erroneously use "git show" instead > of "git format-patch", and it's nice as a maintainer to be able to > easily take their patch rather than going back and forth with them to > get a "correctly" formatted patch containing exactly the same actual > information. > > Signed-off-by: Peter Jones <pjones@redhat.com> > --- > git-am.sh | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 60 insertions(+) > > diff --git a/git-am.sh b/git-am.sh > index c682d34..210e9fe 100755 > --- a/git-am.sh > +++ b/git-am.sh > @@ -216,6 +216,21 @@ check_patch_format () { > read l2 > read l3 > case "$l1" in > + "commit "*) > + case "$l2" in > + "Author: "*) > + case "$l3" in > + "Date: "*) > + patch_format=gitshow > + ;; > + *) > + ;; > + esac > + ;; > + *) > + ;; > + esac > + ;; At least the inner one could become easier to read by losing one level of nesting, e.g. case "$l2,,$l3" in "Author: *",,"Date: ") found it ;; esac I wonder what the severity of the damage if we misidentify the patch format in this function would be? If it is severe enough, the check for the first line may want to become a bit more strict to avoid misidentification (e.g. expr "$l1" : 'commit [0-9a-f]\{40\}$'). Perhaps we don't care. I dunno. > @@ -321,6 +336,51 @@ split_patches () { > this= > msgnum= > ;; > + gitshow) > + this=0 > + for patch in "$@" > + do So each input file is expected to be nothing but an output from "git show" for a single commit; in other words, not concatenation of them, nor just an e-mail message that has "git show" output copy&pasted in the body with some other cruft, but plausibly was delibered as a separate attachment file. I somehow was visualizing that you were trying to accept mails I sometimes see here like: From: somebody Date: someday Hi, a long winded discussion that talks about the motivation behind the patch comes here. commit 4d8c4db13c8c4c79b6fc0a38ff52d85d3543aa7a Author: A U Thor <author@example.com> Date: Tue Sep 11 12:34:56 2012 +0900 a one liner that just says "bugfix" and nothing else diff --git .... and that was one of the reasons I thought (but didn't say in my responses) "Why bother? When running 'am' on such a message you will have to edit the message to move things around anyway". If the target is a stand-alone "git show" output, at least we do not have to worry about such a case. > + this=`expr "$this" + 1` > + msgnum=`printf "%0${prec}d" $this` > + # The first nonemptyline after an empty line is the > + # subject, and the body starts with the next nonempty > + # line. > + perl -ne 'BEGIN { > + $diff = 0; $subject = 0; $subjtext=""; > + } > + if ($diff == 1 || /^diff/ || /^---$/) { > + $diff = 1 ; > + print ; > + } elsif ($subject > 1) { > + s/^ // ; > + print ; > + } elsif ($subject == 1 && !/^\s+$/) { > + s/^ // ; > + $subjtext = "$subjtext $_"; > + } elsif ($subject == 1) { > + $subject = 2 ; > + print "Subject: ", $subjtext ; > + s/^ // ; > + print ; > + } elsif ($subject) { > + print "\n" ; > + s/^ // ; > + print ; > + } elsif (/^\s+$/) { next ; } > + elsif (/^Author:/) { s/Author/From/ ; print ;} > + elsif (/^(From|Date)/) { print ; } Where does "^From" come from? Should this be /^Date: / instead? > + elsif (/^commit/) { next ; } > + else { > + s/^ // ; > + $subjtext = $_; > + $subject = 1; > + } > + ' < "$patch" > "$dotest/$msgnum" || clean_abort > + done This reminds me of another reason why I am hesitant to make "am" take output from "git show". Unlike format-patch output, "Author:" and "Date:" in its output, because it is meant for human consumption, might be a fair game for i18n/l10n (the first line "commit [0-9][a-f]{40}" is not likely to change, though). > + echo "$this" > "$dotest/last" > + this= > + msgnum= > + ;; > hg) > this=0 > for hg in "$@" ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] git-am: Handle "git show" output correctly 2012-09-12 20:06 ` Junio C Hamano @ 2012-09-12 20:48 ` Peter Jones 2012-09-12 21:03 ` Peter Jones 2012-09-12 21:18 ` Junio C Hamano 0 siblings, 2 replies; 21+ messages in thread From: Peter Jones @ 2012-09-12 20:48 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Wed, 2012-09-12 at 13:06 -0700, Junio C Hamano wrote: > Peter Jones <pjones@redhat.com> writes: > > > This patch adds the ability for "git am" to accept patches in the format > > generated by "git show". Some people erroneously use "git show" instead > > of "git format-patch", and it's nice as a maintainer to be able to > > easily take their patch rather than going back and forth with them to > > get a "correctly" formatted patch containing exactly the same actual > > information. > > > > Signed-off-by: Peter Jones <pjones@redhat.com> > > --- > > git-am.sh | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 60 insertions(+) > > > > diff --git a/git-am.sh b/git-am.sh > > index c682d34..210e9fe 100755 > > --- a/git-am.sh > > +++ b/git-am.sh > > @@ -216,6 +216,21 @@ check_patch_format () { > > read l2 > > read l3 > > case "$l1" in > > + "commit "*) > > + case "$l2" in > > + "Author: "*) > > + case "$l3" in > > + "Date: "*) > > + patch_format=gitshow > > + ;; > > + *) > > + ;; > > + esac > > + ;; > > + *) > > + ;; > > + esac > > + ;; > > At least the inner one could become easier to read by losing one > level of nesting, e.g. > > case "$l2,,$l3" in > "Author: *",,"Date: ") > found it > ;; > esac Yeah, I can do that. > I wonder what the severity of the damage if we misidentify the patch > format in this function would be? If it is severe enough, the check > for the first line may want to become a bit more strict to avoid > misidentification (e.g. expr "$l1" : 'commit [0-9a-f]\{40\}$'). > Perhaps we don't care. I dunno. I hadn't really even considered it - are there other formats that use commit and author which git-am.sh is supposed to support? It seems as though if we get something wrong you'll wind up in clean_abort at some point anyway. At worst your patch will still be there and you'll need to do "git am --abort". > > @@ -321,6 +336,51 @@ split_patches () { > > this= > > msgnum= > > ;; > > + gitshow) > > + this=0 > > + for patch in "$@" > > + do > > So each input file is expected to be nothing but an output from "git > show" for a single commit; in other words, not concatenation of > them, nor just an e-mail message that has "git show" output > copy&pasted in the body with some other cruft, but plausibly was > delibered as a separate attachment file. > > I somehow was visualizing that you were trying to accept mails I > sometimes see here like: > > From: somebody > Date: someday > > Hi, a long winded discussion that talks about the motivation > behind the patch comes here. > > commit 4d8c4db13c8c4c79b6fc0a38ff52d85d3543aa7a > Author: A U Thor <author@example.com> > Date: Tue Sep 11 12:34:56 2012 +0900 > > a one liner that just says "bugfix" and nothing else > > diff --git .... > > and that was one of the reasons I thought (but didn't say in my > responses) "Why bother? When running 'am' on such a message you > will have to edit the message to move things around anyway". Yeah, that sounds like madness. > If the target is a stand-alone "git show" output, at least we do not > have to worry about such a case. Right. > > > + this=`expr "$this" + 1` > > + msgnum=`printf "%0${prec}d" $this` > > + # The first nonemptyline after an empty line is the > > + # subject, and the body starts with the next nonempty > > + # line. > > + perl -ne 'BEGIN { > > + $diff = 0; $subject = 0; $subjtext=""; > > + } > > + if ($diff == 1 || /^diff/ || /^---$/) { > > + $diff = 1 ; > > + print ; > > + } elsif ($subject > 1) { > > + s/^ // ; > > + print ; > > + } elsif ($subject == 1 && !/^\s+$/) { > > + s/^ // ; > > + $subjtext = "$subjtext $_"; > > + } elsif ($subject == 1) { > > + $subject = 2 ; > > + print "Subject: ", $subjtext ; > > + s/^ // ; > > + print ; > > + } elsif ($subject) { > > + print "\n" ; > > + s/^ // ; > > + print ; > > + } elsif (/^\s+$/) { next ; } > > + elsif (/^Author:/) { s/Author/From/ ; print ;} > > + elsif (/^(From|Date)/) { print ; } > > Where does "^From" come from? Should this be /^Date: / instead? Entirely copy-paste error on my part. I'll fix it for the next version. > > > + elsif (/^commit/) { next ; } > > + else { > > + s/^ // ; > > + $subjtext = $_; > > + $subject = 1; > > + } > > + ' < "$patch" > "$dotest/$msgnum" || clean_abort > > + done > > This reminds me of another reason why I am hesitant to make "am" > take output from "git show". Unlike format-patch output, "Author:" > and "Date:" in its output, because it is meant for human > consumption, might be a fair game for i18n/l10n (the first line > "commit [0-9][a-f]{40}" is not likely to change, though). Well, if that happens, maybe we could regexp match on "[[:alnum:]_-]+: /someexprthatlookslikeanemailaddress/" ? But we could also just wait to cross that bridge until we get to it? Even if that does get translated later, the current patch would continue to work for English, so even in that case it's not totally worthless. -- Peter ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] git-am: Handle "git show" output correctly 2012-09-12 20:48 ` Peter Jones @ 2012-09-12 21:03 ` Peter Jones 2012-09-12 21:18 ` Junio C Hamano 1 sibling, 0 replies; 21+ messages in thread From: Peter Jones @ 2012-09-12 21:03 UTC (permalink / raw) To: git; +Cc: Peter Jones This patch adds the ability for "git am" to accept patches in the format generated by "git show". Some people erroneously use "git show" instead of "git format-patch", and it's nice as a maintainer to be able to easily take their patch rather than going back and forth with them to get a "correctly" formatted patch containing exactly the same actual information. Signed-off-by: Peter Jones <pjones@redhat.com> --- git-am.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/git-am.sh b/git-am.sh index c682d34..d20f249 100755 --- a/git-am.sh +++ b/git-am.sh @@ -216,6 +216,18 @@ check_patch_format () { read l2 read l3 case "$l1" in + "commit "*) + case "$l2,,$l3" in + "Author: "*,,"Date: "*) + if expr "$l1" : 'commit [0-9a-f]\{40\}$' \ + >/dev/null ; then + patch_format=gitshow + fi + ;; + *) + ;; + esac + ;; "From "* | "From: "*) patch_format=mbox ;; @@ -321,6 +333,51 @@ split_patches () { this= msgnum= ;; + gitshow) + this=0 + for patch in "$@" + do + this=`expr "$this" + 1` + msgnum=`printf "%0${prec}d" $this` + # The first nonemptyline after an empty line is the + # subject, and the body starts with the next nonempty + # line. + perl -ne 'BEGIN { + $diff = 0; $subject = 0; $subjtext=""; + } + if ($diff == 1 || /^diff/ || /^---$/) { + $diff = 1 ; + print ; + } elsif ($subject > 1) { + s/^ // ; + print ; + } elsif ($subject == 1 && !/^\s+$/) { + s/^ // ; + $subjtext = "$subjtext $_"; + } elsif ($subject == 1) { + $subject = 2 ; + print "Subject: ", $subjtext ; + s/^ // ; + print ; + } elsif ($subject) { + print "\n" ; + s/^ // ; + print ; + } elsif (/^\s+$/) { next ; } + elsif (/^Author:/) { s/Author/From/ ; print ;} + elsif (/^Date:/) { print ; } + elsif (/^commit/) { next ; } + else { + s/^ // ; + $subjtext = $_; + $subject = 1; + } + ' < "$patch" > "$dotest/$msgnum" || clean_abort + done + echo "$this" > "$dotest/last" + this= + msgnum= + ;; hg) this=0 for hg in "$@" -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] git-am: Handle "git show" output correctly 2012-09-12 20:48 ` Peter Jones 2012-09-12 21:03 ` Peter Jones @ 2012-09-12 21:18 ` Junio C Hamano 2012-09-12 21:26 ` Dan Johnson 1 sibling, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2012-09-12 21:18 UTC (permalink / raw) To: Peter Jones; +Cc: git Peter Jones <pjones@redhat.com> writes: > Well, if that happens, maybe we could regexp match on > "[[:alnum:]_-]+: /someexprthatlookslikeanemailaddress/" ? I doubt that would be even reliably done. > But we could > also just wait to cross that bridge until we get to it? Not really. If we start encouraging people to use "git show" output as a kosher input to "am", we would have to support such use forever, and we end up painting ourselves in a corner we cannot get out of easily. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] git-am: Handle "git show" output correctly 2012-09-12 21:18 ` Junio C Hamano @ 2012-09-12 21:26 ` Dan Johnson 2012-09-12 22:19 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Dan Johnson @ 2012-09-12 21:26 UTC (permalink / raw) To: Junio C Hamano; +Cc: Peter Jones, git On Wed, Sep 12, 2012 at 5:18 PM, Junio C Hamano <gitster@pobox.com> wrote: > Peter Jones <pjones@redhat.com> writes: > >> Well, if that happens, maybe we could regexp match on >> "[[:alnum:]_-]+: /someexprthatlookslikeanemailaddress/" ? > > I doubt that would be even reliably done. > >> But we could >> also just wait to cross that bridge until we get to it? > > Not really. If we start encouraging people to use "git show" output > as a kosher input to "am", we would have to support such use > forever, and we end up painting ourselves in a corner we cannot get > out of easily. If git am emitted a warning when accepting "git show" output, it seems like it would support Peter's use-case without encouraging bad behavior? -- -Dan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] git-am: Handle "git show" output correctly 2012-09-12 21:26 ` Dan Johnson @ 2012-09-12 22:19 ` Junio C Hamano 2012-09-12 22:31 ` Dan Johnson 2012-09-12 22:41 ` Andreas Ericsson 0 siblings, 2 replies; 21+ messages in thread From: Junio C Hamano @ 2012-09-12 22:19 UTC (permalink / raw) To: Dan Johnson; +Cc: Peter Jones, git Dan Johnson <computerdruid@gmail.com> writes: >> Not really. If we start encouraging people to use "git show" output >> as a kosher input to "am", we would have to support such use >> forever, and we end up painting ourselves in a corner we cannot get >> out of easily. > > If git am emitted a warning when accepting "git show" output, it seems > like it would support Peter's use-case without encouraging bad > behavior? Are you seriously suggesting me to sell to our users a new feature saying "this does not work reliably, we would not recommend using it, no, really, don't trust it." from the day the feature is introduced, especially when we know it will not be "the feature does not work well yet, but it will, we promise" but is "and it may become worse in the future"? I do not see much point in doing that. Besides, what bad behaviour do we avoid from encouraging with such an approach? As Peter said, the problem is not on the part of the user who ended up with an output from "git show", when he really wants output from "git format-patch". Giving the warning to the user of "git am" is too late. I may be able to be pursuaded to swallow a new script somewhere in the contrib/ hierarchy that takes a "git show" output and formats it to look like "format-patch" output to be fed to "git am". That way, when a user has trouble with its parsing of "git show" output, at least we can ask for the output of the format massaging step to help us diagnose where the problem lies. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] git-am: Handle "git show" output correctly 2012-09-12 22:19 ` Junio C Hamano @ 2012-09-12 22:31 ` Dan Johnson 2012-09-12 23:05 ` Junio C Hamano 2012-09-12 22:41 ` Andreas Ericsson 1 sibling, 1 reply; 21+ messages in thread From: Dan Johnson @ 2012-09-12 22:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: Peter Jones, git On Wed, Sep 12, 2012 at 6:19 PM, Junio C Hamano <gitster@pobox.com> wrote: > Dan Johnson <computerdruid@gmail.com> writes: > >>> Not really. If we start encouraging people to use "git show" output >>> as a kosher input to "am", we would have to support such use >>> forever, and we end up painting ourselves in a corner we cannot get >>> out of easily. >> >> If git am emitted a warning when accepting "git show" output, it seems >> like it would support Peter's use-case without encouraging bad >> behavior? > > Are you seriously suggesting me to sell to our users a new feature > saying "this does not work reliably, we would not recommend using > it, no, really, don't trust it." from the day the feature is > introduced, especially when we know it will not be "the feature does > not work well yet, but it will, we promise" but is "and it may become > worse in the future"? > > I do not see much point in doing that. Fair enough. > Besides, what bad behaviour do we avoid from encouraging with such > an approach? As Peter said, the problem is not on the part of the > user who ended up with an output from "git show", when he really > wants output from "git format-patch". Giving the warning to the > user of "git am" is too late. I was assuming Peter would accept the patch, and reply with a "in the future, please submit the output of format-patch", thus correcting the submitter's behavior. This warning would serve someone who did not know that they wanted the output of format-patch, and hopefully teach them to send such a reply message. > I may be able to be pursuaded to swallow a new script somewhere in > the contrib/ hierarchy that takes a "git show" output and formats it > to look like "format-patch" output to be fed to "git am". That way, > when a user has trouble with its parsing of "git show" output, at > least we can ask for the output of the format massaging step to help > us diagnose where the problem lies. That sounds like a better approach to me as well. -- -Dan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] git-am: Handle "git show" output correctly 2012-09-12 22:31 ` Dan Johnson @ 2012-09-12 23:05 ` Junio C Hamano 0 siblings, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2012-09-12 23:05 UTC (permalink / raw) To: Dan Johnson; +Cc: Peter Jones, git Dan Johnson <computerdruid@gmail.com> writes: > I was assuming Peter would accept the patch, and reply with a "in the > future, please submit the output of format-patch", thus correcting the > submitter's behavior. This warning would serve someone who did not > know that they wanted the output of format-patch, and hopefully teach > them to send such a reply message. "Next time, please do this" rarely has worked in practice. This is because the moment you accepted the current patch, you have already lost the "carrot" ;-) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] git-am: Handle "git show" output correctly 2012-09-12 22:19 ` Junio C Hamano 2012-09-12 22:31 ` Dan Johnson @ 2012-09-12 22:41 ` Andreas Ericsson 1 sibling, 0 replies; 21+ messages in thread From: Andreas Ericsson @ 2012-09-12 22:41 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dan Johnson, Peter Jones, git On 09/13/2012 12:19 AM, Junio C Hamano wrote: > Dan Johnson <computerdruid@gmail.com> writes: > >>> Not really. If we start encouraging people to use "git show" output >>> as a kosher input to "am", we would have to support such use >>> forever, and we end up painting ourselves in a corner we cannot get >>> out of easily. >> >> If git am emitted a warning when accepting "git show" output, it seems >> like it would support Peter's use-case without encouraging bad >> behavior? > > Are you seriously suggesting me to sell to our users a new feature > saying "this does not work reliably, we would not recommend using > it, no, really, don't trust it." from the day the feature is > introduced, especially when we know it will not be "the feature does > not work well yet, but it will, we promise" but is "and it may become > worse in the future"? > > I do not see much point in doing that. > > Besides, what bad behaviour do we avoid from encouraging with such > an approach? As Peter said, the problem is not on the part of the > user who ended up with an output from "git show", when he really > wants output from "git format-patch". Giving the warning to the > user of "git am" is too late. > It might be enough to either enable format-patch output or print a warning to stderr when stdout is not a tty. I believe that would at least mitigate the problem, and it might educate the user as well. We already modify output format when stdout is not a tty (removing colors), so we're not giving guarantees about its format when it's piped somewhere. I believe that would provide almost every scenario with the expected outcome (including 'git show | grep'), but there will be a handful of very surprised people as well. -- Andreas Ericsson andreas.ericsson@op5.se OP5 AB www.op5.se Tel: +46 8-230225 Fax: +46 8-230231 Considering the successes of the wars on alcohol, poverty, drugs and terror, I think we should give some serious thought to declaring war on peace. ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2012-09-12 23:05 UTC | newest] Thread overview: 21+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-09-12 15:26 [PATCH] Handle "git show" output correctly Peter Jones 2012-09-12 15:30 ` Peter Jones 2012-09-12 15:41 ` Matthieu Moy 2012-09-12 15:49 ` [PATCH] [git-am] " Peter Jones 2012-09-12 15:57 ` Matthieu Moy 2012-09-12 17:32 ` Junio C Hamano 2012-09-12 18:05 ` Peter Jones 2012-09-12 19:07 ` Junio C Hamano 2012-09-12 15:42 ` [PATCH] " Peter Jones 2012-09-12 15:40 ` Matthieu Moy 2012-09-12 18:00 ` Peter Jones 2012-09-12 18:08 ` [PATCH] git-am: " Peter Jones 2012-09-12 20:06 ` Junio C Hamano 2012-09-12 20:48 ` Peter Jones 2012-09-12 21:03 ` Peter Jones 2012-09-12 21:18 ` Junio C Hamano 2012-09-12 21:26 ` Dan Johnson 2012-09-12 22:19 ` Junio C Hamano 2012-09-12 22:31 ` Dan Johnson 2012-09-12 23:05 ` Junio C Hamano 2012-09-12 22:41 ` Andreas Ericsson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).