[PATCH] Don't add To: recipients to the Cc: header

[PATCH] Don't add To: recipients to the Cc: header

[Ask Bjørn Hansen]

Signed-off-by: Ask Bjørn Hansen <ask@develooper.com>
---
 git-send-email.perl |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 65620ab..530b456 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -557,8 +557,11 @@ sub sanitize_address
 sub send_message
 {
 	my @recipients = unique_email_list(@to);
-	@cc = (map { sanitize_address($_) } @cc);
+	@cc = (grep { my $cc = extract_valid_address($_);
+		      not grep { $cc eq $_ } @recipients
+		    }
+	       map { sanitize_address($_) }
+	       @cc);
 	my $to = join (",\n\t", @recipients);
 	@recipients = unique_email_list(@recipients,@cc,@bcclist);
 	@recipients = (map { extract_valid_address($_) } @recipients);
-- 
1.5.3.5.561.g140d

Re: [PATCH] Don't add To: recipients to the Cc: header

[Ask Bjørn Hansen]

On Nov 19, 2007, at 3:00 AM, Ask Bjørn Hansen wrote:

Whoops.  I thought I stopped the mailing of the first patch.  It was  
adding whitespace to the end of one of the lines.  Sorry about the  
duplication!


  - ask

-- 
http://develooper.com/ - http://askask.com/

Re: [PATCH] Don't add To: recipients to the Cc: header

[Junio C Hamano]

Ask Bjørn Hansen <ask@develooper.com> writes:

> Signed-off-by: Ask Bjørn Hansen <ask@develooper.com>
> ---
>  git-send-email.perl |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 65620ab..530b456 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -557,8 +557,11 @@ sub sanitize_address
>  sub send_message
>  {
>  	my @recipients = unique_email_list(@to);
> -	@cc = (map { sanitize_address($_) } @cc);
> +	@cc = (grep { my $cc = extract_valid_address($_);
> +		      not grep { $cc eq $_ } @recipients
> +		    }
> +	       map { sanitize_address($_) }
> +	       @cc);
>  	my $to = join (",\n\t", @recipients);
>  	@recipients = unique_email_list(@recipients,@cc,@bcclist);
>  	@recipients = (map { extract_valid_address($_) } @recipients);
> -- 
> 1.5.3.5.561.g140d

How did you prepare and send this patch?

I see 7 preimage lines and 11 postimage lines, although the hunk
header claims otherwise.

Did you edit the patch in Emacs diff mode or something?

Re: [PATCH] Don't add To: recipients to the Cc: header

[Ask Bjørn Hansen]

On Nov 19, 2007, at 23:52, Junio C Hamano wrote:

> How did you prepare and send this patch?

git format-patch + tweak in emacs.

> I see 7 preimage lines and 11 postimage lines, although the hunk
> header claims otherwise.
>
> Did you edit the patch in Emacs diff mode or something?

Indeed!  I take it you've seen that particular way of botching it  
before.  :-)

When I was about to send the patch I realized I had added whitespace  
at the end of one of the lines.  Ironically then I ended up just  
sending the messed up patch because I couldn't apply it to my working  
copy after doing a reset.  Being a new git user I convinced myself  
that I had messed up the reset rather than the patch.  Doh.  My  
apologies!   A new patch should be on the list momentarily.


  - ask

-- 
http://develooper.com/ - http://askask.com/

Re: [PATCH] Don't add To: recipients to the Cc: header

[Junio C Hamano]

Ask Bjørn Hansen <ask@develooper.com> writes:

> On Nov 19, 2007, at 23:52, Junio C Hamano wrote:
>
>> How did you prepare and send this patch?
>
> git format-patch + tweak in emacs.
>
>> I see 7 preimage lines and 11 postimage lines, although the hunk
>> header claims otherwise.
>>
>> Did you edit the patch in Emacs diff mode or something?
>
> Indeed!  I take it you've seen that particular way of botching it
> before.  :-)

Yes and a heavy advocate of Emacs on this list wanted to get a
breakage example out of me which I forgot to supply but now we
have it.  Unfortunately I do not offhand recall who it was.

> When I was about to send the patch I realized I had added whitespace
> at the end of one of the lines.  Ironically then I ended up just
> sending the messed up patch because I couldn't apply it to my working
> copy after doing a reset.  Being a new git user I convinced myself
> that I had messed up the reset rather than the patch.  Doh.  My
> apologies!   A new patch should be on the list momentarily.

Oops, forgot to say "no need to resend".  I asked only because I
wanted an independent datapoint for Emacs diff mode breakage.

Re: [PATCH] Don't add To: recipients to the Cc: header

[Sergei Organov]

Junio C Hamano <gitster@pobox.com> writes:
[...]
> Oops, forgot to say "no need to resend".  I asked only because I
> wanted an independent datapoint for Emacs diff mode breakage.

I bet I can damage any patch using any editor ;)

More interesting is what version of Emacs it was?

-- 
Sergei.

Re: [PATCH] Don't add To: recipients to the Cc: header

[Junio C Hamano]

Sergei Organov <osv@javad.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
> [...]
>> Oops, forgot to say "no need to resend".  I asked only because I
>> wanted an independent datapoint for Emacs diff mode breakage.
>
> I bet I can damage any patch using any editor ;)
>
> More interesting is what version of Emacs it was?

To be fair and honest, I do not think there is a simple fix for
this, although it probably is possible to fix it.

What is causing the "breakage" is the fact that format-patch
output ends with the signature delimiter line "^-- $" that
immediately follows the patch text.  The number of preimage
lines recorded in the hunk header of course does not initially
count it, but you are asking the diff editing mode to help you
edit the patch.

In diff editing mode, you can not only edit the contents of
postimage lines, but also add and delete the preimage and
postimage lines, and the diff editimg mode recounts the lines
and adjusts the number of lines recorded in the hunk header when
you do it.  It is very handy if it worked reliably (and often
it does).

But if you edit the last hunk of the format-patch output, unless
the editor very carefully keeps track of what you edited and
what was in the original, it is understandable that it would
mistake the signature delimiter line as the last preimage line
that is "^- $", and ends up miscounting the length of the hunk.

The signature delimiter was there from the beginning in the
patch file, but outside of the hunk in question.  We could argue
that it is a bug to mistake that as a preimage line added by the
user (after all the editor knows what was modified and what was
from the beginning), but it still is understandable.

Re: [PATCH] Don't add To: recipients to the Cc: header

[Sergei Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergei Organov <osv@javad.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>> [...]
>>> Oops, forgot to say "no need to resend".  I asked only because I
>>> wanted an independent datapoint for Emacs diff mode breakage.
>>
>> I bet I can damage any patch using any editor ;)
>>
>> More interesting is what version of Emacs it was?
>
> To be fair and honest, I do not think there is a simple fix for
> this, although it probably is possible to fix it.
>
> What is causing the "breakage" is the fact that format-patch
> output ends with the signature delimiter line "^-- $" that
> immediately follows the patch text.

Exactly. What causes breakage is the fact that the '-' character (as
well as '+', ' ', '!', '#', and '\'), being the first symbol of a line
has special meaning in the diff format.

Therefore it seems that format-patch should better put one empty line
after the last diff hunk and before the signature. Any objections?

-- 
Sergei.

Re: [PATCH] Don't add To: recipients to the Cc: header

[Junio C Hamano]

Sergei Organov <osv@javad.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergei Organov <osv@javad.com> writes:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>> [...]
>>>> Oops, forgot to say "no need to resend".  I asked only because I
>>>> wanted an independent datapoint for Emacs diff mode breakage.
>>>
>>> I bet I can damage any patch using any editor ;)
>>>
>>> More interesting is what version of Emacs it was?
>>
>> To be fair and honest, I do not think there is a simple fix for
>> this, although it probably is possible to fix it.
>>
>> What is causing the "breakage" is the fact that format-patch
>> output ends with the signature delimiter line "^-- $" that
>> immediately follows the patch text.
>
> Exactly. What causes breakage is the fact that the '-' character (as
> well as '+', ' ', '!', '#', and '\'), being the first symbol of a line
> has special meaning in the diff format.

That is correct only if they appear inside a hunk.  The number
of preimage and postimage lines in a hunk is recorded on the
hunk header line --- tools are given enough information to tell
a line that begins with a SP (or '+' or '-') outside a patch
from another such line that is inside the patch.

The diff editing mode of Emacs, at least the version that caused
this issue, however did not make use of that information.
That's the breakage.  Not format-patch output.

Re: [PATCH] Don't add To: recipients to the Cc: header

[Sergei Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergei Organov <osv@javad.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Sergei Organov <osv@javad.com> writes:
>>>
>>>> Junio C Hamano <gitster@pobox.com> writes:
>>>> [...]
>>>>> Oops, forgot to say "no need to resend".  I asked only because I
>>>>> wanted an independent datapoint for Emacs diff mode breakage.
>>>>
>>>> I bet I can damage any patch using any editor ;)
>>>>
>>>> More interesting is what version of Emacs it was?
>>>
>>> To be fair and honest, I do not think there is a simple fix for
>>> this, although it probably is possible to fix it.
>>>
>>> What is causing the "breakage" is the fact that format-patch
>>> output ends with the signature delimiter line "^-- $" that
>>> immediately follows the patch text.
>>
>> Exactly. What causes breakage is the fact that the '-' character (as
>> well as '+', ' ', '!', '#', and '\'), being the first symbol of a line
>> has special meaning in the diff format.
>
> That is correct only if they appear inside a hunk.  The number
> of preimage and postimage lines in a hunk is recorded on the
> hunk header line --- tools are given enough information to tell
> a line that begins with a SP (or '+' or '-') outside a patch
> from another such line that is inside the patch.

Yeah, it's one valid interpretation. Here is another one:

  "The chunk range for the original should be the sum of all contextual
  and deletion (including changed) chunk lines. The chunk range for the
  new file should be a sum of all contextual and addition (including
  changed) chunk lines. If chunk size information does not correspond
  with the number of lines in the hunk, then the diff could be
  considered invalid and be rejected."

taken from here: <http://www.answers.com/topic/diff?cat=technology>

The above implies that a tool should be able to determine the "end of
hunk" without using the hunk header information. This is rather hard to
do with current format-patch output, and it's impossible to do if there
are no "unchanged context" lines at all (i.e., format-patch -U0).

> The diff editing mode of Emacs, at least the version that caused
> this issue, however did not make use of that information.
> That's the breakage.  Not format-patch output.

IMHO it's rather useless to argue about it without strict definition of
correct format of a patch (do you have one?). However, it's easy to add
an empty line for format-patch and very difficult, if not impossible,
for Emacs to handle this without such a line.

Therefore I repeat my question: are there any objections to add such an
empty line by format-patch?

-- 
Sergei.

Re: [PATCH] Don't add To: recipients to the Cc: header

[Junio C Hamano]

Sergei Organov <osv@javad.com> writes:

> Yeah, it's one valid interpretation. Here is another one:
> ...
> taken from here: <http://www.answers.com/topic/diff?cat=technology>

Rants about how dangerous GNU patch liberally (mis)interprets a
broken patch and git-apply is written deliberately more strict
have been repeated on this list, and I would not steal it from
Linus in this response.

>> The diff editing mode of Emacs, at least the version that caused
>> this issue, however did not make use of that information.
>
> IMHO it's rather useless to argue about it without strict definition of
> correct format of a patch (do you have one?)

Yes.  2004 POSIX does not talk about unified context, but Austin
group's SD5-XCU-ERN-103/120 has additions to define unified
context and renames the traditional '-c' output to "copied
context".  Obviously it defines what the numbers on the hunk
header lines mean quite precisely.  GNU folks even managed to
insert text that allows a completely empty line (not a line with
a single SP on it) to express a context line that is empty,
which means...

> However, it's easy to add
> an empty line for format-patch and very difficult, if not impossible,
> for Emacs to handle this without such a line.
>
> Therefore I repeat my question: are there any objections to add such an
> empty line by format-patch?

... there is a strong objection, if you are talking about adding
an empty line before "-- \n" that is in front of the GIT version
signature: such an empty line would not help at all.  A broken
implementation will just skip over such an empty line, counting
it as a line common to both preimage and postimage, and will
still miscount the e-mail signature separator "-- \n" as a line
removed from the preimage.

Having said that, the _ONLY_ reason I made format-patch end its
output with "-- \n" with GIT version was because I wanted to do
an informal census of the user community by observing mailing
list traffic of projects that use git.  The tool has since
matured, and census in such a form is not so important anymore.

If we wanted to have a workaround to this issue, we could simply
remove these last two lines, and that would a be much better one
than an extra blank line.  I do not have a strong objection to
such a change, but you would need to adjust the tests.  The most
depressing part of the whole exercise would be to make sure that
the adjustments to the tests are correct.

Re: [PATCH] Don't add To: recipients to the Cc: header

[Sergei Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergei Organov <osv@javad.com> writes:
>
>> Yeah, it's one valid interpretation. Here is another one:
>> ...
>> taken from here: <http://www.answers.com/topic/diff?cat=technology>
>
> Rants about how dangerous GNU patch liberally (mis)interprets a
> broken patch and git-apply is written deliberately more strict
> have been repeated on this list, and I would not steal it from
> Linus in this response.

Yeah, being strict when applying patches is a good idea, I agree, though
I fail to see how it is relevant to the problem at hand. I mean that if
we either remove the signature or put an empty line before it, the
resulting file won't become less conforming to the patch format, isn't
it?

>>> The diff editing mode of Emacs, at least the version that caused
>>> this issue, however did not make use of that information.
>>
>> IMHO it's rather useless to argue about it without strict definition of
>> correct format of a patch (do you have one?)
>
> Yes.  2004 POSIX does not talk about unified context, but Austin
> group's SD5-XCU-ERN-103/120 has additions to define unified
> context and renames the traditional '-c' output to "copied
> context".  Obviously it defines what the numbers on the hunk
> header lines mean quite precisely.

I don't argue it. But the descriptions of the format I've seen suggest
that the hunks of the proper unified diff format could be easily
syntactically separated, thus providing a way to check a patch for
correctness by comparing what is written in headers with what is
gathered by syntactic analysis. Git's signature makes this rather
difficult.

IMHO, putting extra lines that don't follow the patch format precisely
is an extension, and being an extension, it should better be compatible
with as many tools as possible, and this signature of the format-patch
is known to break at least one tool. Unfortunately I don't have the
document you mention, but I doubt it discusses extensions, and therefore
I'm afraid we won't be able to conclude from it if putting signature
just after the hunk is allowed by the format or not.

As for Emacs, the problem is not that it doesn't know what the numbers
of the hunk header lines mean, but that it needs to re-build them from a
patch that has been arbitrary edited and could potentially be broken
from the beginning. Therefore, it needs hunks to be clearly
syntactically separated to rebuild header numbers from the context.

> GNU folks even managed to insert text that allows a completely empty
> line (not a line with a single SP on it) to express a context line
> that is empty, which means...

Really? That's a surprise for me. What I can tell for sure, Emacs' diff
mode doesn't support this, as it does interpret plain empty line as a
hunk delimiter, at least in Emacs 22.1.

[...]
>> Therefore I repeat my question: are there any objections to add such an
>> empty line by format-patch?
>
> ... there is a strong objection, if you are talking about adding
> an empty line before "-- \n" that is in front of the GIT version
> signature: such an empty line would not help at all.

Yes, I'm talking about exactly this, and the fact is that it does help,
at least in Emacs case. [How comes you think I didn't check it before
posting?!]

> A broken implementation will just skip over such an empty line,
> counting it as a line common to both preimage and postimage, and will
> still miscount the e-mail signature separator "-- \n" as a line
> removed from the preimage.

Emacs doesn't do it when empty line is present, -- it considers empty
line as hunk delimiter (along with any line that doesn't start with ' ',
'+', '-', or '!'). BTW, it counts lines starting with either '#' or '\'
as comments, i.e., just ignores them when counting the number of lines
in the hunk.

> If we wanted to have a workaround to this issue, we could simply
> remove these last two lines, and that would a be much better one
> than an extra blank line.

Why? I think it's nice idea to put git version as signature, and I see
no reason to remove it.

Besides, if empty line is put there before the signature, it'd be more
readable for human beings as well, as humans seem to prefer to put empty
line before their signature anyway.

-- 
Sergei.

Re: [PATCH] Don't add To: recipients to the Cc: header

[Junio C Hamano]

Sergei Organov <osv@javad.com> writes:

>> GNU folks even managed to insert text that allows a completely empty
>> line (not a line with a single SP on it) to express a context line
>> that is empty, which means...
>
> Really? That's a surprise for me. What I can tell for sure, Emacs' diff
> mode doesn't support this, as it does interpret plain empty line as a
> hunk delimiter, at least in Emacs 22.1.

See b507b465f7831612b9d9fc643e3e5218b64e5bfa (git-apply: prepare for
upcoming GNU diff -u format change).  Around the time that eventually
lead to this commit (mid October 2006) there was a discussion on this
mailing list on the issue, too.  I do not doubt you checked with your
version of Emacs diff mode that it does not support this yet, but it's
only prudent to assume that a new version someday will.

Re: [PATCH] Don't add To: recipients to the Cc: header

[Sergei Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergei Organov <osv@javad.com> writes:
>
>>> GNU folks even managed to insert text that allows a completely empty
>>> line (not a line with a single SP on it) to express a context line
>>> that is empty, which means...
>>
>> Really? That's a surprise for me. What I can tell for sure, Emacs' diff
>> mode doesn't support this, as it does interpret plain empty line as a
>> hunk delimiter, at least in Emacs 22.1.
>
> See b507b465f7831612b9d9fc643e3e5218b64e5bfa (git-apply: prepare for
> upcoming GNU diff -u format change).  Around the time that eventually
> lead to this commit (mid October 2006) there was a discussion on this
> mailing list on the issue, too.  I do not doubt you checked with your
> version of Emacs diff mode that it does not support this yet, but it's
> only prudent to assume that a new version someday will.

Thanks, -- it was interesting to read corresponding discussions.

Due to this change in GNU diff, it seems that empty line is indeed a
wrong choice for syntactic hunk separator :( I wonder if there is a
common way to say "here the patch ends" then[1]? My best guess is that

===

will do.

[1] I've checked bzr and hg. Bzr uses empty line for that (followed by
"# Begin bundle" line) in their "merge directive" format. Not
Emacs-friendly either :( Hg's "export" just EOFs after the patch.

-- 
Sergei.