# [PATCH v3 6/7] send-email: suppress leading and trailing whitespaces in addresses 11 messages from 2015-06-09 to 2015-06-10. Participants: Remi Lespinet, Matthieu Moy, Junio C Hamano. Thread: https://gitlist.dev/t/39577 ## Remi Lespinet, 2015-06-09 18:50 Subject: [PATCH v3 6/7] send-email: suppress leading and trailing whitespaces in addresses Message-ID: <1433875804-16007-1-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr> URL: https://gitlist.dev/e/1433875804-16007-1-git-send-email-remi.lespinet%40ensimag.grenoble-inp.fr ``` Remove leading and trailing whitespaces when sanitizing addresses so that git send-email give the same output when passing arguments like " jdoe@example.com " or "\t jdoe@example.com " as with "jdoe@example.com". The next commit will introduce a test for this aswell. Signed-off-by: Remi Lespinet --- git-send-email.perl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/git-send-email.perl b/git-send-email.perl index ea03308..3d144bd 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -978,6 +978,9 @@ sub sanitize_address { # remove garbage after email address $recipient =~ s/(.*>).*$/$1/; + # remove leading and trailing whitespace + $recipient =~ s/^\s+|\s+$//g; + my ($recipient_name, $recipient_addr) = ($recipient =~ /^(.*?)\s*(<.*)/); if (not $recipient_name) { -- 1.9.1 ``` ## Remi Lespinet, 2015-06-09 18:50 Subject: [PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion Message-ID: <1433875804-16007-2-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr> URL: https://gitlist.dev/e/1433875804-16007-2-git-send-email-remi.lespinet%40ensimag.grenoble-inp.fr In-Reply-To: <1433875804-16007-1-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr> ``` As alias file formats supported by git send-email doesn't take whitespace into account, it is useless to consider whitespaces in alias name. remove leading and trailing whitespace before expanding allow to recognize strings like " alias" or "alias\t" passed by --to, --cc, --bcc options or by the git send-email prompt. Signed-off-by: Remi Lespinet --- git-send-email.perl | 1 + t/t9001-send-email.sh | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/git-send-email.perl b/git-send-email.perl index 3d144bd..34c8b8b 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -787,6 +787,7 @@ sub expand_aliases { my %EXPANDED_ALIASES; sub expand_one_alias { my $alias = shift; + $alias =~ s/^\s+|\s+$//g; if ($EXPANDED_ALIASES{$alias}) { die "fatal: alias '$alias' expands to itself\n"; } diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 9aee474..bbfed56 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -1692,4 +1692,28 @@ test_expect_success $PREREQ 'aliases work with email list' ' test_cmp expected-list actual-list ' +test_expect_success $PREREQ 'leading and trailing whitespaces are removed' ' + echo "alias to2 to2@example.com" >.mutt && + echo "alias cc1 Cc 1 " >>.mutt && + test_config sendemail.aliasesfile ".mutt" && + test_config sendemail.aliasfiletype mutt && + TO1=$(echo "QTo 1 " | q_to_tab) && + TO2=$(echo "QZto2" | qz_to_tab_space) && + CC1=$(echo "cc1" | append_cr) && + BCC1=$(echo "Q bcc1@example.com Q" | q_to_nul) && + git send-email \ + --dry-run \ + --from=" Example " \ + --to="$TO1" \ + --to="$TO2" \ + --to=" to3@example.com " \ + --cc="$CC1" \ + --cc="Cc2 " \ + --bcc="$BCC1" \ + --bcc="bcc2@example.com" \ + 0001-add-master.patch | replace_variable_fields \ + >actual-list && + test_cmp expected-list actual-list +' + test_done -- 1.9.1 ``` ## Matthieu Moy, 2015-06-10 08:17 Subject: Re: [PATCH v3 6/7] send-email: suppress leading and trailing whitespaces in addresses Message-ID: URL: https://gitlist.dev/e/vpqr3pk9dv3.fsf%40anie.imag.fr In-Reply-To: <1433875804-16007-1-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr> ``` Nothing serious, but you did something weird while sending. This message does not have a References: or an In-reply-to: field, so it breaks threading. See how it's displayed on http://thread.gmane.org/gmane.comp.version-control.git Remi Lespinet writes: > Remove leading and trailing whitespaces when sanitizing addresses so > that git send-email give the same output when passing arguments like > " jdoe@example.com " or "\t jdoe@example.com " as with > "jdoe@example.com". > > The next commit will introduce a test for this aswell. s/aswell/as well/ -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Matthieu Moy, 2015-06-10 08:32 Subject: Re: [PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion Message-ID: URL: https://gitlist.dev/e/vpqa8w89d5x.fsf%40anie.imag.fr In-Reply-To: <1433875804-16007-2-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr> ``` Remi Lespinet writes: > As alias file formats supported by git send-email doesn't take > whitespace into account, it is useless to consider whitespaces in > alias name. remove leading and trailing whitespace before expanding s/remove/Remove/ > allow to recognize strings like " alias" or "alias\t" passed by --to, > --cc, --bcc options or by the git send-email prompt. > > Signed-off-by: Remi Lespinet > --- > git-send-email.perl | 1 + > t/t9001-send-email.sh | 24 ++++++++++++++++++++++++ > 2 files changed, 25 insertions(+) > > diff --git a/git-send-email.perl b/git-send-email.perl > index 3d144bd..34c8b8b 100755 > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -787,6 +787,7 @@ sub expand_aliases { > my %EXPANDED_ALIASES; > sub expand_one_alias { > my $alias = shift; > + $alias =~ s/^\s+|\s+$//g; > if ($EXPANDED_ALIASES{$alias}) { > die "fatal: alias '$alias' expands to itself\n"; > } You should explain why you need that, when the previous patch was already about removing whitespaces around addresses. I finally understood that this was needed because alias expansion comes before sanitize_address, but the commit message should have told me that. Actually, once you have this, PATCH 6/7 becomes useless, right? (at least, the test passes if I revert it) It seems to me that doing this space trimming just once, inside or right after split_at_commas would be clearer. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Remi Lespinet, 2015-06-10 08:33 Subject: [PATCH v3 6/7] send-email: suppress leading and trailing whitespaces in addresses Message-ID: <1862589361.335676.1433925234231.JavaMail.zimbra@ensimag.grenoble-inp.fr> URL: https://gitlist.dev/e/1862589361.335676.1433925234231.JavaMail.zimbra%40ensimag.grenoble-inp.fr In-Reply-To: ``` > Nothing serious, but you did something weird while sending. This message > does not have a References: or an In-reply-to: field, so it breaks > threading. See how it's displayed on > > http://thread.gmane.org/gmane.comp.version-control.git Yes, send-email was aborted after 5/7, I realized and retry sending 6/7 and 7/7 but I didn't noticed that. I'll be careful next time, thanks. ``` ## Remi Lespinet, 2015-06-10 09:30 Subject: [PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion Message-ID: <1281238070.338321.1433928615479.JavaMail.zimbra@ensimag.grenoble-inp.fr> URL: https://gitlist.dev/e/1281238070.338321.1433928615479.JavaMail.zimbra%40ensimag.grenoble-inp.fr In-Reply-To: ``` Matthieu Moy writes: > Actually, once you have this, PATCH 6/7 becomes useless, right? (at > least, the test passes if I revert it) > It seems to me that doing this space trimming just once, inside or right > after split_at_commas would be clearer. You're right, I put it twice because of there's occurrences of sanitize_address which are not associated with expand_aliases, but it seems that it's all taken care of separately in different regexp. So there's no point to 6/7. I agree, I'd like to put it right after split_at_commas in a separate function "trim_list". Is it a good idea even if the function is one line long ? ``` ## Junio C Hamano, 2015-06-10 15:15 Subject: Re: [PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion Message-ID: URL: https://gitlist.dev/e/xmqqa8w7oarl.fsf%40gitster.dls.corp.google.com In-Reply-To: <1281238070.338321.1433928615479.JavaMail.zimbra@ensimag.grenoble-inp.fr> ``` Remi Lespinet writes: > I agree, I'd like to put it right after split_at_commas in a separate > function "trim_list". Is it a good idea even if the function is one > line long ? Hmph, if I have "A, B, C" and call a function that gives an array of addresses, treating the input as comma-separated addresses, I would expect ("A", "B", "C") to be returned from that function, instead of having to later trim the whitespace around what is returned. It suggests that split-at-commas _is_ a wrong abstraction, doesn't it? In other words, I think whitespace trimming is part of what the split-a-single-string-into-array-of-addresses helper function should be doing. ``` ## Matthieu Moy, 2015-06-10 15:28 Subject: Re: [PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion Message-ID: URL: https://gitlist.dev/e/vpqoaknr39r.fsf%40anie.imag.fr In-Reply-To: ``` Junio C Hamano writes: > Remi Lespinet writes: > >> I agree, I'd like to put it right after split_at_commas in a separate >> function "trim_list". Is it a good idea even if the function is one >> line long ? > > Hmph, if I have "A, B, C" and call a function that gives an array of > addresses, treating the input as comma-separated addresses, I would > expect ("A", "B", "C") to be returned from that function, instead of > having to later trim the whitespace around what is returned. It is actually doing this. But if you have " A,B,C ", then you'll get " A", "B", "C ". But once you're trimming around commas, trimming leading and trailing spaces fits well with split itself. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Remi Lespinet, 2015-06-10 16:10 Subject: [PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion Message-ID: <595924690.351865.1433952607248.JavaMail.zimbra@ensimag.grenoble-inp.fr> URL: https://gitlist.dev/e/595924690.351865.1433952607248.JavaMail.zimbra%40ensimag.grenoble-inp.fr In-Reply-To: ``` Matthieu Moy writes: > Junio C Hamano writes: > > > Remi Lespinet writes: > > > >> I agree, I'd like to put it right after split_at_commas in a separate > >> function "trim_list". Is it a good idea even if the function is one > >> line long ? > > > > Hmph, if I have "A, B, C" and call a function that gives an array of > > addresses, treating the input as comma-separated addresses, I would > > expect ("A", "B", "C") to be returned from that function, instead of > > having to later trim the whitespace around what is returned. > > It is actually doing this. But if you have " A,B,C ", then you'll get > " A", "B", "C ". But once you're trimming around commas, trimming > leading and trailing spaces fits well with split itself. Yes and if we have a single address with leading or/and trailing whitespaces, such as " A ", I think that we don't expect split_in_commas to suppress these whitespaces as there's no commas in this address. As Junio said, I think I should rename the function. Thanks! ``` ## Junio C Hamano, 2015-06-10 16:15 Subject: Re: [PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion Message-ID: URL: https://gitlist.dev/e/xmqqfv5zmtf0.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Matthieu Moy writes: > Junio C Hamano writes: > >> Remi Lespinet writes: >> >>> I agree, I'd like to put it right after split_at_commas in a separate >>> function "trim_list". Is it a good idea even if the function is one >>> line long ? >> >> Hmph, if I have "A, B, C" and call a function that gives an array of >> addresses, treating the input as comma-separated addresses, I would >> expect ("A", "B", "C") to be returned from that function, instead of >> having to later trim the whitespace around what is returned. > > It is actually doing this. But if you have " A,B,C ", then you'll get > " A", "B", "C ". But once you're trimming around commas, trimming > leading and trailing spaces fits well with split itself. I guess we are saying the same thing, then? That is, trim-list as a separate step does not make sense an it is part of the job for the helper to turn a single list with multiple addresses into an array? ``` ## Matthieu Moy, 2015-06-10 16:25 Subject: Re: [PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion Message-ID: URL: https://gitlist.dev/e/vpq8ubrwmwe.fsf%40anie.imag.fr In-Reply-To: ``` Junio C Hamano writes: > Matthieu Moy writes: > >> Junio C Hamano writes: >> >>> Hmph, if I have "A, B, C" and call a function that gives an array of >>> addresses, treating the input as comma-separated addresses, I would >>> expect ("A", "B", "C") to be returned from that function, instead of >>> having to later trim the whitespace around what is returned. >> >> It is actually doing this. But if you have " A,B,C ", then you'll get >> " A", "B", "C ". But once you're trimming around commas, trimming >> leading and trailing spaces fits well with split itself. > > I guess we are saying the same thing, then? That is, trim-list as a > separate step does not make sense an it is part of the job for the > helper to turn a single list with multiple addresses into an array? Yes. I was clarifying what was done and what wasn't, not disagreeing. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ```