[PATCH] Don't use $author_name undefined when $from contains no /\s</.

[PATCH] Don't use $author_name undefined when $from contains no /\s</.

[Jim Meyering]

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(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index b17d261..1c6d2cc 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -412,7 +412,7 @@ sub send_message
 	}

 	my ($author_name) = ($from =~ /^(.*?)\s+</);
-	if ($author_name =~ /\./ && $author_name !~ /^".*"$/) {
+	if ($author_name && $author_name =~ /\./ && $author_name !~ /^".*"$/) {
 		my ($name, $addr) = ($from =~ /^(.*?)(\s+<.*)/);
 		$from = "\"$name\"$addr";
 	}
--
1.4.3.g72bb

Re: [PATCH] Don't use $author_name undefined when $from contains no /\s</.

[Junio C Hamano]

Jim Meyering <jim@meyering.net> writes:

> I noticed a case not handled in a recent patch.

Thanks. Will apply.

Curiously your patch was whitespace damaged.

Re: [PATCH] Don't use $author_name undefined when $from contains no /\s</.

[Jim Meyering]

Junio C Hamano <junkio@cox.net> wrote:
> Jim Meyering <jim@meyering.net> writes:
>
>> I noticed a case not handled in a recent patch.
>
> Thanks. Will apply.
>
> Curiously your patch was whitespace damaged.

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

Re: [PATCH] Don't use $author_name undefined when $from contains no /\s</.

[Junio C Hamano]

Jim Meyering <jim@meyering.net> writes:

> 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.

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?

Re: [PATCH] Don't use $author_name undefined when $from contains no /\s</.

[Paul Eggert]

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

> I see no good reason, other than saving a single byte from the
> output stream perhaps.

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

> 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"?

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.

Re: [PATCH] Don't use $author_name undefined when $from contains no /\s</.

[Junio C Hamano]

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

> Junio C Hamano <junkio@cox.net> writes:
>
>> I see no good reason, other than saving a single byte from the
>> output stream perhaps.
>
> 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.

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.

> 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

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

Re: [PATCH] Don't use $author_name undefined when $from contains no /\s</.

[Paul Eggert]

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

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

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.

Re: [PATCH] Don't use $author_name undefined when $from contains no /\s</.

[Junio C Hamano]

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

> 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.

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.

Re: [PATCH] Don't use $author_name undefined when $from contains no /\s</.

[Linus Torvalds]

On Fri, 20 Oct 2006, Junio C Hamano wrote:
> 
> Coding a workaround is not a big deal; the change is simple and
> trivial.

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

Re: [PATCH] Don't use $author_name undefined when $from contains no /\s</.

[Jakub Narebski]

Paul Eggert wrote:

> Junio C Hamano <junkio@cox.net> writes:
> 
>> If "trailing space" highlighting picks up the first column blank
>> in "diff -u" output, that highlighting feature is *broken*.
> 
> 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.

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.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git