{"thread":{"id":"51835","subject":"[PATCH v2] git-svn: trim leading and trailing whitespaces in author name","startedAt":"2019-09-12T14:56:40Z","lastAt":"2019-09-13T07:47:04Z","messageCount":3,"participants":["Tobias Klauser","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"382226","messageId":"20190912145638.32192-1-tklauser@distanz.ch","threadId":"51835","inReplyTo":null,"subject":"[PATCH v2] git-svn: trim leading and trailing whitespaces in author name","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2019-09-12T14:56:38Z","receivedAt":"2019-09-12T14:56:40Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"In some cases, the svn author names might contain leading or trailing\nwhitespaces, leading to messages such as:\n\n  Author: user1\n   not defined in authors.txt\n\n(the trailing newline leads to the line break). The user \"user1\" is\ndefined in authors.txt though, e.g.\n\n  user1 = User <user1@example.com>\n\nFix this by trimming the author name retreived from svn before using it\nin check_author.\n\nSigned-off-by: Tobias Klauser <tklauser@distanz.ch>\n---\nv2:\n - move whitespace trimming below defined'ness check as per Eric Sunshine's\n   review comment\n\n perl/Git/SVN.pm | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\nindex 76b29659057d..d8c07419f51c 100644\n--- a/perl/Git/SVN.pm\n+++ b/perl/Git/SVN.pm\n@@ -1494,6 +1494,7 @@ sub check_author {\n \tif (!defined $author || length $author == 0) {\n \t\t$author = '(no author)';\n \t}\n+\t$author =~ s/^\\s+|\\s+$//g;\n \tif (!defined $::users{$author}) {\n \t\tif (defined $::_authors_prog) {\n \t\t\t$::users{$author} = call_authors_prog($author);\n-- \n2.23.0.dirty\n\n"},{"id":"382238","messageId":"CAPig+cTAY=4cuDyrsPiDH+MUvz4+H4eMKAsmTAETep2On5=q3g@mail.gmail.com","threadId":"51835","inReplyTo":"20190912145638.32192-1-tklauser@distanz.ch","subject":"Re: [PATCH v2] git-svn: trim leading and trailing whitespaces in author name","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-09-12T18:20:11Z","receivedAt":"2019-09-12T18:20:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Sep 12, 2019 at 10:56 AM Tobias Klauser <tklauser@distanz.ch> wrote:\n> v2:\n>  - move whitespace trimming below defined'ness check as per Eric Sunshine's\n>    review comment\n> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\n> @@ -1494,6 +1494,7 @@ sub check_author {\n>         if (!defined $author || length $author == 0) {\n>                 $author = '(no author)';\n>         }\n> +       $author =~ s/^\\s+|\\s+$//g;\n\nHmm, this still looks questionable. I would have expected the\nwhitespace trimming to be below the 'defined' check but before the\nlength($author)==0 check (since the length might become 0 once\nwhitespace is trimmed).\n\nAlso, a minor style/comprehension nit: Perhaps I'm just old-school,\nbut for me, the idiom:\n\n    $author =~ s/^\\s+//;\n    $author =~ s/\\s+$//;\n\nis easier to understand at-a-glance as trimming leading and trailing\nwhitespace than the more compact (noisy) expression this patch uses.\nBut that's just a subjective review comment, not necessarily\nactionable.\n"},{"id":"382284","messageId":"20190913074700.hwmyalfmn66kkgbs@distanz.ch","threadId":"51835","inReplyTo":"CAPig+cTAY=4cuDyrsPiDH+MUvz4+H4eMKAsmTAETep2On5=q3g@mail.gmail.com","subject":"Re: [PATCH v2] git-svn: trim leading and trailing whitespaces in author name","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2019-09-13T07:47:01Z","receivedAt":"2019-09-13T07:47:04Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"Thank you for the review Eric!\n\nOn 2019-09-12 at 20:20:11 +0200, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Thu, Sep 12, 2019 at 10:56 AM Tobias Klauser <tklauser@distanz.ch> wrote:\n> > v2:\n> >  - move whitespace trimming below defined'ness check as per Eric Sunshine's\n> >    review comment\n> > diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\n> > @@ -1494,6 +1494,7 @@ sub check_author {\n> >         if (!defined $author || length $author == 0) {\n> >                 $author = '(no author)';\n> >         }\n> > +       $author =~ s/^\\s+|\\s+$//g;\n> \n> Hmm, this still looks questionable. I would have expected the\n> whitespace trimming to be below the 'defined' check but before the\n> length($author)==0 check (since the length might become 0 once\n> whitespace is trimmed).\n\nAgree, I didn't consider this case. Will change in v3.\n\n> Also, a minor style/comprehension nit: Perhaps I'm just old-school,\n> but for me, the idiom:\n> \n>     $author =~ s/^\\s+//;\n>     $author =~ s/\\s+$//;\n> \n> is easier to understand at-a-glance as trimming leading and trailing\n> whitespace than the more compact (noisy) expression this patch uses.\n> But that's just a subjective review comment, not necessarily\n> actionable.\n\nAgree, the form you propose is more readable and saving just a line\ndoesn't seem worth the possible impact on comprehension/readability.\nWill change in v3.\n"}]}