patchDon't use $author_name undefined when $from contains no /\s</.

I noticed a case not handled in a recent patch. Demonstrate it like this:

  $ touch new-file
  $ git-send-email --dry-run --from j --to k new-file 2>err
  new-file
  OK. Log says:
  Date: Thu, 19 Oct 2006 10:26:24 +0200
  Sendmail: /usr/sbin/sendmail
  From: j
  Subject:
  Cc:
  To: k

  Result: OK
  $ cat err
  Use of uninitialized value in pattern match (m//) at /p/bin/git-send-email line 416.
  Use of uninitialized value in concatenation (.) or string at /p/bin/git-send-email line 420.
  Use of uninitialized value in concatenation (.) or string at /p/bin/git-send-email line 468.

There's a patch for the $author_name part below.

The example above shows that $subject may also be used uninitialized. That should be easy to fix, too.

Signed-off-by: Jim Meyering <jim@meyering.net>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Jim Meyering <jim@meyering.net> writes:

Thanks. Will apply.

Curiously your patch was whitespace damaged.

Junio C Hamano <junkio@cox.net> wrote:

I wondered what you meant, so compared what I sent with the output of the command I ran:

  git-format-patch --stdout --signoff HEAD~1

There were two differences, both involving removed trailing blanks. The first was a part of the diff: a line consisting of a single space denoting an empty line in the context. I understood that those types of lines may safely be truncated (removing the trailing blank), and in fact, GNU diff -u (cvs) now does this by default:

2006-09-05  Paul Eggert  <eggert@cs.ucla.edu>

        * NEWS: diff -u no longer outputs trailing white space unless the
        input data has it.  Suggested by Jim Meyering.
        * doc/diff.texi (Detailed Unified): Document this.
        * src/context.c (pr_unidiff_hunk): Implement this.

The only other difference was the removal of the trailing blank following the "--" signature introducer.

I see that git-apply does not handle this new format:

  $ git-apply patch
  fatal: corrupt patch at line 47

That diagnostic comes from builtin-apply.c:

		if (len <= 0)
			die("corrupt patch at line %d", linenr);

It would be nice if git would accept such unified diff output, since no other program we know of rejects them. Paul Eggert has even submitted revised wording to make POSIX allow this style of output.

For reference, the GNU diff thread started here:
  http://lists.gnu.org/archive/html/bug-gnu-utils/2006-09/msg00005.html

Jim Meyering <jim@meyering.net> writes:

Gaah. Paul, why did you have to break this? I see no good reason, other than saving a single byte from the output stream perhaps.

Leading ' ' at the context line is _not_ trailing white space; it is a metadata just like a leading '+' or '-' is.

We could certainly update git-apply to understand it and we probably would need to do so to cope with patch generated with this *broken* GNU diff behaviour.

I see why some people consider why it _might_ be a good change. A broken MUA tend to have trouble with lines that has only whitespaces, so if a patch application program (patch or git-apply) wants to deal with such a broken MUA, accepting a totally empty line as if it is a line that has a single whitespace at the beginning would save us from grief in some cases.

However, I am not sure what "unless input data has it" means. Does that mean if you have a line that has only one TAB (perhaps caused by broken autoindent in the editor), that is "input data" and is output as "SP TAB LF"? If that is the case, then I do not think dropping the leading SP only for an empty line makes any sense. A broken MUA would happily munge a line "SP TAB LF" just as it would eat a line "SP LF". Worse, such a MUA would munge "+ TAB LF" into "+ LF", making the result of patch application to be something the original patch author did not intend to have.

If anything, this new behaviour makes the situation *actively* worse.

By deciding to keep "SP TAB LF", you are saying that you _care_ about that trailing TAB in the patch and whitespace breakage affects your payload in a bad way in your particular application. If that is the case, you would want to detect any whitespace breakage a MUA might have caused before applying that patch, and a broken context line that ought to be "SP LF" but somehow comes out from MUA as "LF" would have served us as a coalmine canary to help us detect the breakage. Paul's change to GNU diff is to kill that canary and I do not see any benefit for doing so.

Why?

Please revert the patch, pretty please?

Junio C Hamano <junkio@cox.net> writes:

That wasn't the motivation. Rather, it was to support the style where people use editors that highlight trailing blanks, since trailing blanks can cause trouble in some contexts (e.g., they can change the semantics of C programs and Makefiles). When examining unified diffs, any added or removed trailing blanks will be easy to spot with such an editor, but only if "diff -u" doesn't output any trailing blanks of its own.

You can read more about this at the thread that inspired the diffutils change, rooted here:

http://lists.gnu.org/archive/html/bug-gnu-utils/2006-09/msg00005.html

Yes, that's correct. In the highlighting-editor scenario, such a line would be highlighted, but the people who want to see trailing white space highlighted will indeed want the highlighting here, so it's fine.

This change was not motivated by broken MUAs. Broken MUAs are a problem that GNU 'patch' has already had to deal with, for many years. The change was motivated by a desire to make significant trailing white space easier to find, when people are examining text that contains some diffs and some other stuff.

Paul Eggert <eggert@CS.UCLA.EDU> writes:

If "trailing space" highlighting picks up the first column blank in "diff -u" output, that highlighting feature is *broken*.

"git diff --color" does the whitespace breakage highlighting, but it knows that the first column *is* not payload and does not highlight it.

I've read it. It was not convincing and was not even an amusing read.

Junio C Hamano <junkio@cox.net> writes:

If the buffer contains arbitrary text, some of which is diff -u output and some of which is not, then it it isn't possible in general for the highlighting mode to distinguish between the diff -u part and the other part. This sort of thing is fairly common among people who email patches and code around, or who generate files containing a combination of patches and other things.

If the change bothers you a lot, you might want to follow up to <http://www.opengroup.org/austin/mailarchives/ag-review/msg02139.html>, which proposes the change in question to the POSIX folks. This change is atop the earlier change I proposed to specify "diff -u" format in the first place; see <http://www.opengroup.org/austin/mailarchives/ag-review/msg02077.html>. You can follow up by writing to austin-group-l@opengroup.org and citing XCU ERN 103. You can find a copy of XCU ERN 103 at <http://www.opengroup.org/austin/aardvark/latest/xcubug2.txt>; look for "Number 103".

Since git uses diff -u format, it would make sense to git to work with the upcoming POSIX spec for diff -u, either by adjusting the spec or by adjusting git.

Paul Eggert <eggert@CS.UCLA.EDU> writes:

It is not quite fair to talk as if I still have a choice.

Apparently a version of GNU diff that generates new format is already in the wild (I've received such a patch which was where this thread started). Whether I like your change or not, the damage is already done and its output needs to be dealt with, so that we do not break users.

Coding a workaround is not a big deal; the change is simple and trivial. It's just I am somewhat unhappy, having to do a .1 release immediately after v1.4.3 which took about two months to stabilize, although that's not your fault. Sorry for venting.

On Fri, 20 Oct 2006, Junio C Hamano wrote:

Yeah, I sent Junio a patch that _should_ make git accept the patches already, so technically it was easy.

What irritates me personally about the new format for "-u" is that

 - Maybe "-u" is new as far as _POSIX_ is concerned, but daamn, it's been 
   a standard format for a hell of a long time in real life, and this was 
   a totally gratuitous change.

 - The new format is very much a new "special case". Now a totally empty 
   line means exactly the same as a line that is " \n", so we have a new 
   special case that simply didn't use to exist - we used to be able to 
   just always skip the first character on a line, and consider the rest 
   of the line to be "the data". Now you can't do that any more.

   The fact that GNU patch has always accepted total crap patches, has 
   always been a thorn in my side: GNU patch is simply too accepting by 
   default if you care about the integrity of the end result (I always ran 
   it with "-p1 --fuzz=0" just to at least fix the most egregious cases of 
   "we'll accept anything that loks even _remotely_ likely to apply")

 - git-apply was being very strict with patches on purpose. The "empty 
   line in a patch" error has triggered several time for me, and at least 
   so far it has _not_ ever been due to a new GNU patch, but every time 
   due to a broken mailer or somebody not being careful when editing the 
   patch by hand.  So triggering an error has been the _right_ thing to 
   do so far - it's been a big red sign saying "somebody did something bad 
   to this patch".

so I think the new format is strictly speaking a regression. It takes away a good sanity-check, and we're stuck with having to handle old-style patches _anyway_ for the forseeable future, so we can't replace it with a new sanity check.

But it does seem like we have no choice, simply because people apparently already use the broken version.

			Linus

Paul Eggert wrote:

Not true. If GNU patch (and git-apply) can detect where diff begins, and can detect if diff was truncated, then highlighting mode can distinguish between diff -u part and rest... well, unless you intermix diff-u output and arbitrary text (so the patch would not apply, but what happens when commenting a patch).

Still I'd rather relax highlighting code to not highlight "SPC LF" than to change diff -u format.