Re: [PATCH v2 2/2] send-email: Support separate Reply-To address
Christian Ludwig writes:
> In some projects contributions from groups are only accepted from a
> common group email address. But every individual may want to recieve
> replies to her own personal address. That's what we have 'Reply-To'
> headers for in SMTP.
>
> Introduce an optional '--reply-to' command line option. Unfortunately
> the $reply_to variable name was already taken for the 'In-Reply-To'
> header field. To reduce code churn, use $reply_address as variable
> name instead.
>
> Signed-off-by: Christian Ludwig
> ---
> Documentation/git-send-email.txt | 5 +
> contrib/completion/git-completion.bash | 2 +-
> git-send-email.perl| 18 +-
> t/t9001-send-email.sh | 2 ++
> 4 files changed, 25 insertions(+), 2 deletions(-)
Thanks.
While merging this with other topics in flight on 'pu', there were
minor merge conflicts, especially with np/send-email-header-parsing
that ends at b6049542 ("send-email: extract email-parsing code into
a subroutine", 2017-12-15) that attempts to refactor the code that
reads the header lines. As there is *no* real change that benefits
by the refactoring accompanying the topic, it was a bit hard for me
to say if it is needless code churn or if it is a good refactoring.
I wonder if this change can be a good demonstration to measure the
goodness of it. IOW, how would these two patches look if rebased on
the result of merging b6049542 to today's 'master'? Would it make
these two patches cleaner and easier to grok?
Re: [PATCH v2 2/2] send-email: Support separate Reply-To address
On Wed, Jan 17 2018, Christian Ludwig jotted:
> +if (defined $reply_to) {
> + $reply_to =~ s/^\s+|\s+$//g;
> + ($reply_to) = expand_aliases($reply_to);
> + $reply_to = sanitize_address($reply_to);
> +}
Just a note to other reviewers: I found it odd to call a function with
one argument and then enforce list context, but I see expand_aliases()
expects to be called that way, and this is copied from a previous
pattern earlier in the file.
(As an aside, that code looks a bit odd, but then I see 302e04ea4d
("send-email: detect cycles in alias expansion", 2009-07-23) )
Looks good to me.
Re: [PATCH v2 2/2] send-email: Support separate Reply-To address
On 17 January 2018 at 19:08, Christian Ludwig wrote: > In some projects contributions from groups are only accepted from a > common group email address. But every individual may want to recieve > replies to her own personal address. That's what we have 'Reply-To' > headers for in SMTP. s/recieve/receive/ > Introduce an optional '--reply-to' command line option. Unfortunately > the $reply_to variable name was already taken for the 'In-Reply-To' > header field. To reduce code churn, use $reply_address as variable > name instead. "To reduce ..." no longer describes the patch, since v2 actually performs that refactoring in patch 1/2. "Unfortunately ..." is correct, but seems less relevant now. Except: I suppose that a non-git.git patch which uses $reply_to could start misbehaving now that this series changes the meaning of that variable. Just thinking out loud, it could make some sense to take patch 1/2 for sanity, but to then *not* re-use $reply_to for a new purpose, but to actually take your v1-patch as 2/2. Or, this potential problem can perhaps be ignored (except in the commit message?).. > Signed-off-by: Christian Ludwig "From:" and "Signed-off-by:" are different. Not sure if that's ok. Martin
