Re: [PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion
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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion
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? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion
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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion
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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 7/7] send-email: suppress leading and trailing whitespaces before alias expansion
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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html