Re: [PATCH] git-am: Handle "git show" output correctly
Dan Johnson 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" ;-) -- 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] git-am: Handle "git show" output correctly
On 09/13/2012 12:19 AM, Junio C Hamano wrote: > Dan Johnson 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.erics...@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. -- 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] git-am: Handle "git show" output correctly
On Wed, Sep 12, 2012 at 6:19 PM, Junio C Hamano wrote: > Dan Johnson 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 -- 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] git-am: Handle "git show" output correctly
Dan Johnson 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. -- 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] git-am: Handle "git show" output correctly
On Wed, Sep 12, 2012 at 5:18 PM, Junio C Hamano wrote: > Peter Jones 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 -- 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] git-am: Handle "git show" output correctly
Peter Jones 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. -- 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-am: Handle "git show" output correctly
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 --- 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 -- 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] git-am: Handle "git show" output correctly
On Wed, 2012-09-12 at 13:06 -0700, Junio C Hamano wrote: > Peter Jones 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 > > --- > > 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 > 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) { > > +
Re: [PATCH] git-am: Handle "git show" output correctly
Peter Jones 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 > --- > 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 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/^// ; > +
Re: [PATCH] [git-am] Handle "git show" output correctly
Peter Jones 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. -- 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-am: Handle "git show" output correctly
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 --- 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 -- 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] [git-am] Handle "git show" output correctly
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 -- 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] [git-am] Handle "git show" output correctly
Matthieu Moy writes: > Peter Jones writes: > >> Subject: [PATCH] [git-am] Handle "git show" output correctly > > The convention in Git is ": " (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. -- 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] [git-am] Handle "git show" output correctly
Peter Jones writes: > Subject: [PATCH] [git-am] Handle "git show" output correctly The convention in Git is ": " (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/ -- 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-am] Handle "git show" output correctly
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 --- 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 -- 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