{"thread":{"id":"53093","subject":"[PATCH] added: Multi line support for ignore-paths configuration","startedAt":"2020-03-25T07:14:15Z","lastAt":"2020-04-13T18:18:07Z","messageCount":5,"participants":["Lukas Pupka-Lipinski","Eric Wong","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"393949","messageId":"e5c78a24-e17f-c1bb-4ea7-3ddaa45abcf0@lpl-mind.de","threadId":"53093","inReplyTo":null,"subject":"[PATCH] added: Multi line support for ignore-paths configuration","fromName":"Lukas Pupka-Lipinski","fromEmail":"lukas.pupkalipinski@lpl-mind.de","sentAt":"2020-03-25T06:56:31Z","receivedAt":"2020-03-25T07:14:15Z","isPatch":true,"sender":{"key":"lukas.pupkalipinski@lpl-mind.de","avatar":null},"body":" From 01d4e4dbc4e524db6188f41904a7274d1582d066 Mon Sep 17 00:00:00 2001\nFrom: lukas.pupkalipinski@lpl-mind.de\nDate: Tue, 24 Mar 2020 13:47:27 +0100\nSubject: [PATCH] added: Multi line support for ignore-paths configuration\nIn addition we should add multi line support for include-paths.\n\nSigned-off-by: lukas.pupkalipinski@lpl-mind.de\nReported-by:\nAcked-by:\nReviewed-by:\nTested-by:\n---\n  perl/Git/SVN/Fetcher.pm | 3 ++-\n  1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm\nindex 64e900a0e9..1bcba49a76 100644\n--- a/perl/Git/SVN/Fetcher.pm\n+++ b/perl/Git/SVN/Fetcher.pm\n@@ -31,7 +31,8 @@ sub new {\n      # override options set in an [svn-remote \"...\"] section\n      $repo_id = $git_svn->{repo_id};\n      my $k = \"svn-remote.$repo_id.ignore-paths\";\n-    my $v = eval { command_oneline('config', '--get', $k) };\n+    my $v = eval { command('config', '--get-all', $k) };\n+    $v  =~ s/[\\x0A\\x0D]//g if (defined $v);\n      $self->{ignore_regex} = $v;\n\n      $k = \"svn-remote.$repo_id.include-paths\";\n-- \n2.25.1.windows.1\n\n"},{"id":"394007","messageId":"20200325203823.GA21913@dcvr","threadId":"53093","inReplyTo":"e5c78a24-e17f-c1bb-4ea7-3ddaa45abcf0@lpl-mind.de","subject":"Re: [PATCH] added: Multi line support for ignore-paths configuration","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2020-03-25T20:38:23Z","receivedAt":"2020-03-25T20:38:25Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Lukas Pupka-Lipinski wrote:\n> From 01d4e4dbc4e524db6188f41904a7274d1582d066 Mon Sep 17 00:00:00 2001\n\nHello Lukas and welcome to git hacker community! It looks like\nyour first patch, so some details to point out, most of which\nshould be covered in `Documentation/SubmittingPatches'\n\n> From: lukas.pupkalipinski@lpl-mind.de\n> Date: Tue, 24 Mar 2020 13:47:27 +0100\n> Subject: [PATCH] added: Multi line support for ignore-paths configuration\n\nNo need for a From/Date/Subject in the body of the email\n(they're already in the subject), unless From or Subject differs\nfrom what's already in the email header.\n\nSince git-svn is a subsection, a better subject might be:\n\n Subject: git-svn: support multi-line ignore-paths in config\n\n(but needs clarification, below)\n\n> In addition we should add multi line support for include-paths.\n\nCan you clarify whether this allows multi-line support is\nintended to allow importing paths with newlines in them?\n\nOr is this to support multiple values of include-paths?\n\n(judging from the code below, it seem like the latter)\n\nIn any case, we prefer full sentence(s) in the commit message\n\n> Signed-off-by: lukas.pupkalipinski@lpl-mind.de\n\nThanks, though S-o-b generally includes the full name, so:\n\n Signed-off-by: Lukas Pupka-Lipinski \n\nis more appropriate\n\n> Reported-by:\n> Acked-by:\n> Reviewed-by:\n> Tested-by:\n\nEmpty fields aren't necessary, you can drop the above 4 lines.\n\n> ---\n>  perl/Git/SVN/Fetcher.pm | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm\n> index 64e900a0e9..1bcba49a76 100644\n> --- a/perl/Git/SVN/Fetcher.pm\n> +++ b/perl/Git/SVN/Fetcher.pm\n> @@ -31,7 +31,8 @@ sub new {\n>      # override options set in an [svn-remote \"...\"] section\n>      $repo_id = $git_svn->{repo_id};\n>      my $k = \"svn-remote.$repo_id.ignore-paths\";\n> -    my $v = eval { command_oneline('config', '--get', $k) };\n> +    my $v = eval { command('config', '--get-all', $k) };\n> +    $v  =~ s/[\\x0A\\x0D]//g if (defined $v);\n>      $self->{ignore_regex} = $v;\n> \n>      $k = \"svn-remote.$repo_id.include-paths\";\n\nThat looks like it would munge the following:\n\n\t[svn-remote \"foo\"]\n\t\tignore-paths = a\n\t\tignore-paths = b\n\ninto \"a\\nb\\n\"\n\nAnd finally into a regexp: /ab/\n\n...Which doesn't seem correct, to me.\n\nI suggest something like the following to retrieve values\nfrom `git config --get-all' in array context and join the\nvalues together into one regexp (totally untested):\n\n\tmy @v = eval { command('config', '--get-all', $k) };\n\tchomp(@v); # get rid of trailing newlines\n\t$self->{ignore_regexp} = '(?:' . join('|', @v) . ')';\n\n> 2.25.1.windows.1\n\nI'm not sure how git-config or chomp() behaves on Windows systems\nwith CRLF line endings, though.\n\nA possibility would be replacing chomp(@v) with:\n\n\ts/\\r?\\n\\z//s for @v;\n\nBut that is more verbose and less readable (and slower).\n\nAlso, you had a second patch which didn't hit the list but got\ninto my inbox. It contained an HTML part which vger rejects due\nto processing costs and likelyhood of being spam.\n\nCould you resend that as plain text? Some of my comments above\nwould also apply to your second patch. Thanks.\n"},{"id":"394019","messageId":"xmqqd090w2ju.fsf@gitster.c.googlers.com","threadId":"53093","inReplyTo":"20200325203823.GA21913@dcvr","subject":"Re: [PATCH] added: Multi line support for ignore-paths configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-25T21:24:37Z","receivedAt":"2020-03-25T21:24:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> Lukas Pupka-Lipinski wrote:\n>> From 01d4e4dbc4e524db6188f41904a7274d1582d066 Mon Sep 17 00:00:00 2001\n>\n> Hello Lukas and welcome to git hacker community! It looks like\n> your first patch, so some details to point out, most of which\n> should be covered in `Documentation/SubmittingPatches'\n\nThanks, Lukas and Eric for an excellent review.\n\n Thanks, though S-o-b generally includes the full name, so:\n>\n> Signed-off-by: Lukas Pupka-Lipinski \n>\n> is more appropriate\n\nYes. The author identity and the \"name \" on Signed-off-by\nline should match.\n\n"},{"id":"394300","messageId":"20200329232404.GA12701@dcvr","threadId":"53093","inReplyTo":"0515a11b-d9ae-3f22-65a8-5efee235d5c9@lpl-mind.de","subject":"Re: [PATCH] added: Multi line support for ignore-paths configuration","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2020-03-29T23:24:04Z","receivedAt":"2020-03-29T23:24:10Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Lukas Pupka-Lipinski wrote:\n> Hi Eric,\n> \n> thanks for your feedback.\n> \n> I will include your general Feedback in the next Patch mail.\n> \n> In Addition i think its not clear what i was trying to solve. I use the git\n> svn extension for our company SVN. Unfortunately we have a lot of stuff in\n> the SVN what I do not use and don’t want to checkout. So I started to use\n> the ignore-paths option. But git only allows to have ca. 150 char in one\n> config line. Which is not enough for me. So I started to extend the code to\n> use also the next line. So that your expression can be bigger than 150 char,\n> spread over several lines.\n\nThanks for the explanation. In the future, please keep\ngit@vger.kernel.org and anybody else in the discussion in the\nCc: list (use reply-to-all in your mailer).\n\n> I hope that make it clear.\n\nOK, please do, thank you :)\n\n> I will resend the second mail in few seconds\n> \n> > That looks like it would munge the following:\n> > \n> > \t[svn-remote \"foo\"]\n> > \t\tignore-paths = a\n> > \t\tignore-paths = b\n> > \n> > into \"a\\nb\\n\"\n> > \n> > And finally into a regexp:/ab/\n> > \n> > ...Which doesn't seem correct, to me.\n> \n> No this will end up into \"ab\" the \\n and \\r are removed at\n> \n> +    $v  =~ s/[\\x0A\\x0D]//g if (defined $v);\n\nRight, so do you agree your patch is broken in that case\nand needs fixing?\n\nSorry, my mental abilities are dulled from the stress and\ninsomnia caused by the pandemic, so I have more trouble\nunderstanding things than usual :<\n\n> Am 25.03.2020 um 21:38 schrieb Eric Wong:\n> > > 2.25.1.windows.1\n> > I'm not sure how git-config or chomp() behaves on Windows systems\n> > with CRLF line endings, though.\n> > \n> > A possibility would be replacing chomp(@v) with:\n> > \n> > \ts/\\r?\\n\\z//s for @v;\n> \n> Yes that has to be done. Its implemented in\n> \n> +    $v  =~ s/[\\x0A\\x0D]//g if (defined $v);\n\nDoes that mean you'll send a v2 of the patch which uses\n\n\ts/\\r?\\n\\z//s for @v;\n\n?\n\nThanks in advance for clarifying.\n"},{"id":"395367","messageId":"20200413181805.GA1872@dcvr","threadId":"53093","inReplyTo":"8fbfbf1c-ea14-6739-7881-cfa3d9642de9@lpl-mind.de","subject":"Re: [PATCH] added: Multi line support for ignore-paths configuration","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2020-04-13T18:18:05Z","receivedAt":"2020-04-13T18:18:07Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Lukas Pupka-Lipinski wrote:\n\nSorry for the long delay, had other stuff to deal with.\n(and off-list replies with HTML doesn't take high priority)\n\n> Am 30.03.2020 um 01:24 schrieb Eric Wong:\n> > > Am 25.03.2020 um 21:38 schrieb Eric Wong:\n> > > > > 2.25.1.windows.1\n> > > > I'm not sure how git-config or chomp() behaves on Windows systems\n> > > > with CRLF line endings, though.\n> > > > \n> > > > A possibility would be replacing chomp(@v) with:\n> > > > \n> > > > \ts/\\r?\\n\\z//s for @v;\n> > > Yes that has to be done. Its implemented in\n> > > \n> > > +    $v  =~ s/[\\x0A\\x0D]//g if (defined $v);\n> > Does that mean you'll send a v2 of the patch which uses\n> > \n> > \ts/\\r?\\n\\z//s for @v;\n> > \n> > ?\n> \n> Its already in the patch. Line\n> \n> +    $v  =~ s/[\\x0A\\x0D]//g if (defined $v);\n\nPlease reread my original message about how `$v'\nas a scalar is incorrect (and thus your s///g):\n\nhttps://lore.kernel.org/git/20200325203823.GA21913@dcvr/\n\nThe correct operations are on `@v' as an array,\n`$v' as a scalar. Thanks.\n"}]}