{"thread":{"id":"5955","subject":"[PATCH] Don't use $author_name undefined when $from contains no /\\serr\n new-file\n OK. Log says:\n Date: Thu, 19 Oct 2006 10:26:24 +0200\n Sendmail: /usr/sbin/sendmail\n From: j\n Subject:\n Cc:\n To: k\n\n Result: OK\n $ cat err\n Use of uninitialized value in pattern match (m//) at /p/bin/git-send-email line 416.\n Use of uninitialized value in concatenation (.) or string at /p/bin/git-send-email line 420.\n Use of uninitialized value in concatenation (.) or string at /p/bin/git-send-email line 468.\n\nThere's a patch for the $author_name part below.\n\nThe example above shows that $subject may also be used uninitialized.\nThat should be easy to fix, too.\n\nSigned-off-by: Jim Meyering \n---\n git-send-email.perl | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex b17d261..1c6d2cc 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -412,7 +412,7 @@ sub send_message\n \t}\n\n \tmy ($author_name) = ($from =~ /^(.*?)\\s+ writes:\n\n> I noticed a case not handled in a recent patch.\n\nThanks. Will apply.\n\nCuriously your patch was whitespace damaged.\n"},{"id":"29259","messageId":"878xjckw7x.fsf@rho.meyering.net","threadId":"5955","inReplyTo":"7vbqo8uvkn.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Don't use $author_name undefined when $from contains no /\\s wrote:\n> Jim Meyering writes:\n>\n>> I noticed a case not handled in a recent patch.\n>\n> Thanks. Will apply.\n>\n> Curiously your patch was whitespace damaged.\n\nI wondered what you meant, so compared what I sent\nwith the output of the command I ran:\n\n git-format-patch --stdout --signoff HEAD~1\n\nThere were two differences, both involving removed trailing blanks.\nThe first was a part of the diff: a line consisting of a single space\ndenoting an empty line in the context. I understood that those types\nof lines may safely be truncated (removing the trailing blank),\nand in fact, GNU diff -u (cvs) now does this by default:\n\n2006-09-05 Paul Eggert \n\n * NEWS: diff -u no longer outputs trailing white space unless the\n input data has it. Suggested by Jim Meyering.\n * doc/diff.texi (Detailed Unified): Document this.\n * src/context.c (pr_unidiff_hunk): Implement this.\n\nThe only other difference was the removal of the trailing blank following\nthe \"--\" signature introducer.\n\nI see that git-apply does not handle this new format:\n\n $ git-apply patch\n fatal: corrupt patch at line 47\n\nThat diagnostic comes from builtin-apply.c:\n\n\t\tif (len <= 0)\n\t\t\tdie(\"corrupt patch at line %d\", linenr);\n\nIt would be nice if git would accept such unified diff output,\nsince no other program we know of rejects them. Paul Eggert has\neven submitted revised wording to make POSIX allow this style\nof output.\n\nFor reference, the GNU diff thread started here:\n http://lists.gnu.org/archive/html/bug-gnu-utils/2006-09/msg00005.html\n"},{"id":"29262","messageId":"7vk62wruum.fsf@assigned-by-dhcp.cox.net","threadId":"5955","inReplyTo":"878xjckw7x.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't use $author_name undefined when $from contains no /\\s writes:\n\n> There were two differences, both involving removed trailing blanks.\n> The first was a part of the diff: a line consisting of a single space\n> denoting an empty line in the context. I understood that those types\n> of lines may safely be truncated (removing the trailing blank),\n> and in fact, GNU diff -u (cvs) now does this by default:\n>\n> 2006-09-05 Paul Eggert \n>\n> * NEWS: diff -u no longer outputs trailing white space unless the\n> input data has it. Suggested by Jim Meyering.\n> * doc/diff.texi (Detailed Unified): Document this.\n> * src/context.c (pr_unidiff_hunk): Implement this.\n\nGaah. Paul, why did you have to break this? I see no good\nreason, other than saving a single byte from the output stream\nperhaps.\n\nLeading ' ' at the context line is _not_ trailing white space;\nit is a metadata just like a leading '+' or '-' is.\n\nWe could certainly update git-apply to understand it and we\nprobably would need to do so to cope with patch generated with\nthis *broken* GNU diff behaviour.\n\nI see why some people consider why it _might_ be a good change.\nA broken MUA tend to have trouble with lines that has only\nwhitespaces, so if a patch application program (patch or\ngit-apply) wants to deal with such a broken MUA, accepting a\ntotally empty line as if it is a line that has a single\nwhitespace at the beginning would save us from grief in some\ncases.\n\nHowever, I am not sure what \"unless input data has it\" means.\nDoes that mean if you have a line that has only one TAB (perhaps\ncaused by broken autoindent in the editor), that is \"input data\"\nand is output as \"SP TAB LF\"? If that is the case, then I do\nnot think dropping the leading SP only for an empty line makes\nany sense. A broken MUA would happily munge a line \"SP TAB LF\"\njust as it would eat a line \"SP LF\". Worse, such a MUA would\nmunge \"+ TAB LF\" into \"+ LF\", making the result of patch\napplication to be something the original patch author did not\nintend to have.\n\nIf anything, this new behaviour makes the situation *actively*\nworse.\n\nBy deciding to keep \"SP TAB LF\", you are saying that you _care_\nabout that trailing TAB in the patch and whitespace breakage\naffects your payload in a bad way in your particular\napplication. If that is the case, you would want to detect any\nwhitespace breakage a MUA might have caused before applying that\npatch, and a broken context line that ought to be \"SP LF\" but\nsomehow comes out from MUA as \"LF\" would have served us as a\ncoalmine canary to help us detect the breakage. Paul's change\nto GNU diff is to kill that canary and I do not see any benefit\nfor doing so.\n\nWhy?\n\nPlease revert the patch, pretty please?\n"},{"id":"29266","messageId":"87fydkj8q1.fsf@penguin.cs.ucla.edu","threadId":"5955","inReplyTo":"7vk62wruum.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Don't use $author_name undefined when $from contains no /\\s writes:\n\n> I see no good reason, other than saving a single byte from the\n> output stream perhaps.\n\nThat wasn't the motivation. Rather, it was to support the\nstyle where people use editors that highlight trailing\nblanks, since trailing blanks can cause trouble in some\ncontexts (e.g., they can change the semantics of C programs\nand Makefiles). When examining unified diffs, any added or\nremoved trailing blanks will be easy to spot with such an\neditor, but only if \"diff -u\" doesn't output any trailing\nblanks of its own.\n\nYou can read more about this at the thread that inspired\nthe diffutils change, rooted here:\n\nhttp://lists.gnu.org/archive/html/bug-gnu-utils/2006-09/msg00005.html\n\n> Does that mean if you have a line that has only one TAB (perhaps\n> caused by broken autoindent in the editor), that is \"input data\"\n> and is output as \"SP TAB LF\"?\n\nYes, that's correct. In the highlighting-editor scenario,\nsuch a line would be highlighted, but the people who want to\nsee trailing white space highlighted will indeed want the\nhighlighting here, so it's fine.\n\nThis change was not motivated by broken MUAs. Broken MUAs\nare a problem that GNU 'patch' has already had to deal with,\nfor many years. The change was motivated by a desire to\nmake significant trailing white space easier to find, when\npeople are examining text that contains some diffs and some\nother stuff.\n"},{"id":"29267","messageId":"7vr6x4q9b6.fsf@assigned-by-dhcp.cox.net","threadId":"5955","inReplyTo":"87fydkj8q1.fsf@penguin.cs.ucla.edu","subject":"Re: [PATCH] Don't use $author_name undefined when $from contains no /\\s writes:\n\n> Junio C Hamano writes:\n>\n>> I see no good reason, other than saving a single byte from the\n>> output stream perhaps.\n>\n> That wasn't the motivation. Rather, it was to support the\n> style where people use editors that highlight trailing\n> blanks, since trailing blanks can cause trouble in some\n> contexts (e.g., they can change the semantics of C programs\n> and Makefiles). When examining unified diffs, any added or\n> removed trailing blanks will be easy to spot with such an\n> editor, but only if \"diff -u\" doesn't output any trailing\n> blanks of its own.\n\nIf \"trailing space\" highlighting picks up the first column blank\nin \"diff -u\" output, that highlighting feature is *broken*.\n\n\"git diff --color\" does the whitespace breakage highlighting,\nbut it knows that the first column *is* not payload and does not\nhighlight it.\n\n> You can read more about this at the thread that inspired\n> the diffutils change, rooted here:\n>\n> http://lists.gnu.org/archive/html/bug-gnu-utils/2006-09/msg00005.html\n\nI've read it. It was not convincing and was not even an amusing\nread.\n"},{"id":"29272","messageId":"87pscnj29t.fsf@penguin.cs.ucla.edu","threadId":"5955","inReplyTo":"7vr6x4q9b6.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Don't use $author_name undefined when $from contains no /\\s writes:\n\n> If \"trailing space\" highlighting picks up the first column blank\n> in \"diff -u\" output, that highlighting feature is *broken*.\n\nIf the buffer contains arbitrary text, some of which is diff -u output\nand some of which is not, then it it isn't possible in general for the\nhighlighting mode to distinguish between the diff -u part and the\nother part. This sort of thing is fairly common among people who\nemail patches and code around, or who generate files containing a\ncombination of patches and other things.\n\nIf the change bothers you a lot, you might want to follow up to\n,\nwhich proposes the change in question to the POSIX folks. This change\nis atop the earlier change I proposed to specify \"diff -u\" format in\nthe first place; see\n.\nYou can follow up by writing to austin-group-l@opengroup.org and\nciting XCU ERN 103. You can find a copy of XCU ERN 103 at\n;\nlook for \"Number 103\".\n\nSince git uses diff -u format, it would make sense to git to work with\nthe upcoming POSIX spec for diff -u, either by adjusting the spec or\nby adjusting git.\n"},{"id":"29296","messageId":"7vhcxzpgot.fsf@assigned-by-dhcp.cox.net","threadId":"5955","inReplyTo":"87pscnj29t.fsf@penguin.cs.ucla.edu","subject":"Re: [PATCH] Don't use $author_name undefined when $from contains no /\\s writes:\n\n> Since git uses diff -u format, it would make sense to git to work with\n> the upcoming POSIX spec for diff -u, either by adjusting the spec or\n> by adjusting git.\n\nIt is not quite fair to talk as if I still have a choice.\n\nApparently a version of GNU diff that generates new format is\nalready in the wild (I've received such a patch which was where\nthis thread started). Whether I like your change or not, the\ndamage is already done and its output needs to be dealt with, so\nthat we do not break users.\n\nCoding a workaround is not a big deal; the change is simple and\ntrivial. It's just I am somewhat unhappy, having to do a .1\nrelease immediately after v1.4.3 which took about two months to\nstabilize, although that's not your fault. Sorry for venting.\n"},{"id":"29379","messageId":"ehar3v$e73$1@sea.gmane.org","threadId":"5955","inReplyTo":"87pscnj29t.fsf@penguin.cs.ucla.edu","subject":"Re: [PATCH] Don't use $author_name undefined when $from contains no /\\s Junio C Hamano writes:\n> \n>> If \"trailing space\" highlighting picks up the first column blank\n>> in \"diff -u\" output, that highlighting feature is *broken*.\n> \n> If the buffer contains arbitrary text, some of which is diff -u output\n> and some of which is not, then it it isn't possible in general for the\n> highlighting mode to distinguish between the diff -u part and the\n> other part.\n\nNot true. If GNU patch (and git-apply) can detect where diff begins,\nand can detect if diff was truncated, then highlighting mode can\ndistinguish between diff -u part and rest... well, unless you intermix\ndiff-u output and arbitrary text (so the patch would not apply, but what\nhappens when commenting a patch).\n\nStill I'd rather relax highlighting code to not highlight \"SPC LF\"\nthan to change diff -u format.\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"29385","messageId":"Pine.LNX.4.64.0610200911360.3962@g5.osdl.org","threadId":"5955","inReplyTo":"7vhcxzpgot.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Don't use $author_name undefined when $from contains no /\\s \n> Coding a workaround is not a big deal; the change is simple and\n> trivial.\n\nYeah, I sent Junio a patch that _should_ make git accept the patches \nalready, so technically it was easy.\n\nWhat irritates me personally about the new format for \"-u\" is that\n\n - Maybe \"-u\" is new as far as _POSIX_ is concerned, but daamn, it's been \n a standard format for a hell of a long time in real life, and this was \n a totally gratuitous change.\n\n - The new format is very much a new \"special case\". Now a totally empty \n line means exactly the same as a line that is \" \\n\", so we have a new \n special case that simply didn't use to exist - we used to be able to \n just always skip the first character on a line, and consider the rest \n of the line to be \"the data\". Now you can't do that any more.\n\n The fact that GNU patch has always accepted total crap patches, has \n always been a thorn in my side: GNU patch is simply too accepting by \n default if you care about the integrity of the end result (I always ran \n it with \"-p1 --fuzz=0\" just to at least fix the most egregious cases of \n \"we'll accept anything that loks even _remotely_ likely to apply\")\n\n - git-apply was being very strict with patches on purpose. The \"empty \n line in a patch\" error has triggered several time for me, and at least \n so far it has _not_ ever been due to a new GNU patch, but every time \n due to a broken mailer or somebody not being careful when editing the \n patch by hand. So triggering an error has been the _right_ thing to \n do so far - it's been a big red sign saying \"somebody did something bad \n to this patch\".\n\nso I think the new format is strictly speaking a regression. It takes away \na good sanity-check, and we're stuck with having to handle old-style \npatches _anyway_ for the forseeable future, so we can't replace it with a \nnew sanity check.\n\nBut it does seem like we have no choice, simply because people apparently \nalready use the broken version.\n\n\t\t\tLinus\n"}]}