Re: [PATCH v2] Edit recipient addresses with the --compose flag

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 14, 2008, 03:31 UTC
Message-ID: <7v7i77f0f8.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <alpine.LFD.2.00.0811132013530.6125@sys-0.hiltweb.site>

Ian Hilt <ian.hilt@gmx.com> writes:

Show 15 quoted lines

> On Wed, 12 Nov 2008, Junio C Hamano wrote:
>> Ian Hilt <ian.hilt@gmx.com> writes:
>> 
>> > Sometimes specifying the recipient addresses can be tedious on the
>> > command-line.  This commit allows the user to edit the recipient
>> > addresses in their editor of choice.
>> >
>> > Signed-off-by: Ian Hilt <ian.hilt@gmx.com>
>> > ---
>> > Here's an updated commit with improved regex's from Junio and Francis.
>> 
>> This heavily depends on Pierre's patch, so I am CC'ing him for comments.
>> Until his series settles down, I cannot apply this anyway.
>
> I didn't realize this was such a bad time to submit this patch.

It is not a bad time. I just won't be able to apply it right away, but people (like Pierre) who are interested in send-email enhancement can help improving your patch by reviewing.

Show 9 quoted lines

>> Why does the user must keep "Cc:" in order for this new code to pick up
>> the list of recipients?  ...
>> 
>>     cc              =       "Cc:" address-list CRLF
>>     bcc             =       "Bcc:" (address-list / [CFWS]) CRLF
>>     address-list    =       (address *("," address)) / obs-addr-list
>
> I think you're mistaken here.  It is entirely possible to delete the Cc
> and Bcc lines with no ill effect.

You have this piece of code

>> > +	if ($c_file =~ /^To:\s*(\S.+?)\s*\nCc:/ism) {
>> > +		@tmp_to = get_recipients($1);
>> > +	}

to pick up the "To: " addressees. If your user deletes Cc: line, would that regexp still capture them in @tmp_to? How?

> determine if $cc is equal to ''.  If it's not, then it will use it.

Ah, somehow I thought C2 you are writing into (message.final) was used as the final payload, but you are right. The foreach () loop at the toplevel reads them and interprets them.

>> I think the parsing code you introduced simply suck.  Why isn't it done as
>> a part of the main loop to read the same file that already exists?
>
> Multiline recipient fields.

So you were trying to handle folded headers after all, I see.

But if you were to go that route, I think you are much better off doing so by enabling the header folding for all the header lines in the while (<C>) loop that currently reads one line at a time.

I however hove to wonder why we are not using any canned e-mail header parser for this part of the code. Surely there must be a widely used one that everybody who writes Perl uses???