[PATCH] added: Multi line support for ignore-paths configuration

[PATCH] added: Multi line support for ignore-paths configuration

[Lukas Pupka-Lipinski]

 From 01d4e4dbc4e524db6188f41904a7274d1582d066 Mon Sep 17 00:00:00 2001
From: lukas.pupkalipinski@lpl-mind.de
Date: Tue, 24 Mar 2020 13:47:27 +0100
Subject: [PATCH] added: Multi line support for ignore-paths configuration
In addition we should add multi line support for include-paths.

Signed-off-by: lukas.pupkalipinski@lpl-mind.de
Reported-by:
Acked-by:
Reviewed-by:
Tested-by:
---
  perl/Git/SVN/Fetcher.pm | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm
index 64e900a0e9..1bcba49a76 100644
--- a/perl/Git/SVN/Fetcher.pm
+++ b/perl/Git/SVN/Fetcher.pm
@@ -31,7 +31,8 @@ sub new {
      # override options set in an [svn-remote "..."] section
      $repo_id = $git_svn->{repo_id};
      my $k = "svn-remote.$repo_id.ignore-paths";
-    my $v = eval { command_oneline('config', '--get', $k) };
+    my $v = eval { command('config', '--get-all', $k) };
+    $v  =~ s/[\x0A\x0D]//g if (defined $v);
      $self->{ignore_regex} = $v;

      $k = "svn-remote.$repo_id.include-paths";
-- 
2.25.1.windows.1

Re: [PATCH] added: Multi line support for ignore-paths configuration

[Eric Wong]

Lukas Pupka-Lipinski <lukas.pupkalipinski@lpl-mind.de> wrote:
> From 01d4e4dbc4e524db6188f41904a7274d1582d066 Mon Sep 17 00:00:00 2001

Hello Lukas and welcome to git hacker community!  It looks like
your first patch, so some details to point out, most of which
should be covered in `Documentation/SubmittingPatches'

> From: lukas.pupkalipinski@lpl-mind.de
> Date: Tue, 24 Mar 2020 13:47:27 +0100
> Subject: [PATCH] added: Multi line support for ignore-paths configuration

No need for a From/Date/Subject in the body of the email
(they're already in the subject), unless From or Subject differs
from what's already in the email header.

Since git-svn is a subsection, a better subject might be:

  Subject: git-svn: support multi-line ignore-paths in config

(but needs clarification, below)

> In addition we should add multi line support for include-paths.

Can you clarify whether this allows multi-line support is
intended to allow importing paths with newlines in them?

Or is this to support multiple values of include-paths?

(judging from the code below, it seem like the latter)

In any case, we prefer full sentence(s) in the commit message

> Signed-off-by: lukas.pupkalipinski@lpl-mind.de

Thanks, though S-o-b generally includes the full name, so:

  Signed-off-by: Lukas Pupka-Lipinski <lukas.pupkalipinski@lpl-mind.de>

is more appropriate

> Reported-by:
> Acked-by:
> Reviewed-by:
> Tested-by:

Empty fields aren't necessary, you can drop the above 4 lines.

> ---
>  perl/Git/SVN/Fetcher.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm
> index 64e900a0e9..1bcba49a76 100644
> --- a/perl/Git/SVN/Fetcher.pm
> +++ b/perl/Git/SVN/Fetcher.pm
> @@ -31,7 +31,8 @@ sub new {
>      # override options set in an [svn-remote "..."] section
>      $repo_id = $git_svn->{repo_id};
>      my $k = "svn-remote.$repo_id.ignore-paths";
> -    my $v = eval { command_oneline('config', '--get', $k) };
> +    my $v = eval { command('config', '--get-all', $k) };
> +    $v  =~ s/[\x0A\x0D]//g if (defined $v);
>      $self->{ignore_regex} = $v;
> 
>      $k = "svn-remote.$repo_id.include-paths";

That looks like it would munge the following:

	[svn-remote "foo"]
		ignore-paths = a
		ignore-paths = b

into "a\nb\n"

And finally into a regexp: /ab/

...Which doesn't seem correct, to me.

I suggest something like the following to retrieve values
from `git config --get-all' in array context and join the
values together into one regexp (totally untested):

	my @v = eval { command('config', '--get-all', $k) };
	chomp(@v); # get rid of trailing newlines
	$self->{ignore_regexp} = '(?:' . join('|', @v) . ')';

> 2.25.1.windows.1

I'm not sure how git-config or chomp() behaves on Windows systems
with CRLF line endings, though.

A possibility would be replacing chomp(@v) with:

	s/\r?\n\z//s for @v;

But that is more verbose and less readable (and slower).

Also, you had a second patch which didn't hit the list but got
into my inbox.  It contained an HTML part which vger rejects due
to processing costs and likelyhood of being spam.

Could you resend that as plain text?  Some of my comments above
would also apply to your second patch.  Thanks.

Re: [PATCH] added: Multi line support for ignore-paths configuration

[Junio C Hamano]

Eric Wong <e@80x24.org> writes:

> Lukas Pupka-Lipinski <lukas.pupkalipinski@lpl-mind.de> wrote:
>> From 01d4e4dbc4e524db6188f41904a7274d1582d066 Mon Sep 17 00:00:00 2001
>
> Hello Lukas and welcome to git hacker community!  It looks like
> your first patch, so some details to point out, most of which
> should be covered in `Documentation/SubmittingPatches'

Thanks, Lukas and Eric for an excellent review.

 Thanks, though S-o-b generally includes the full name, so:
>
>   Signed-off-by: Lukas Pupka-Lipinski <lukas.pupkalipinski@lpl-mind.de>
>
> is more appropriate

Yes.  The author identity and the "name <addr>" on Signed-off-by
line should match.

Re: [PATCH] added: Multi line support for ignore-paths configuration

[Eric Wong]

Lukas Pupka-Lipinski <lukas.pupkalipinski@lpl-mind.de> wrote:
> Hi Eric,
> 
> thanks for your feedback.
> 
> I will include your general Feedback in the next Patch mail.
> 
> In Addition i think its not clear what i was trying to solve. I use the git
> svn extension for our company SVN. Unfortunately we have a lot of stuff in
> the SVN what I do not use and don’t want to checkout. So I started to use
> the ignore-paths option. But git only allows to have ca. 150 char in one
> config line. Which is not enough for me. So I started to extend the code to
> use also the next line. So that your expression can be bigger than 150 char,
> spread over several lines.

Thanks for the explanation.  In the future, please keep
git@vger.kernel.org and anybody else in the discussion in the
Cc: list (use reply-to-all in your mailer).

> I hope that make it clear.

OK, please do, thank you :)

> I will resend the second mail in few seconds
> 
> > That looks like it would munge the following:
> > 
> > 	[svn-remote "foo"]
> > 		ignore-paths = a
> > 		ignore-paths = b
> > 
> > into "a\nb\n"
> > 
> > And finally into a regexp:/ab/
> > 
> > ...Which doesn't seem correct, to me.
> 
> No this will end up into "ab" the \n and \r are removed at
> 
> +    $v  =~ s/[\x0A\x0D]//g if (defined $v);

Right, so do you agree your patch is broken in that case
and needs fixing?

Sorry, my mental abilities are dulled from the stress and
insomnia caused by the pandemic, so I have more trouble
understanding things than usual :<

> Am 25.03.2020 um 21:38 schrieb Eric Wong:
> > > 2.25.1.windows.1
> > I'm not sure how git-config or chomp() behaves on Windows systems
> > with CRLF line endings, though.
> > 
> > A possibility would be replacing chomp(@v) with:
> > 
> > 	s/\r?\n\z//s for @v;
> 
> Yes that has to be done. Its implemented in
> 
> +    $v  =~ s/[\x0A\x0D]//g if (defined $v);

Does that mean you'll send a v2 of the patch which uses

	s/\r?\n\z//s for @v;

?

Thanks in advance for clarifying.

Re: [PATCH] added: Multi line support for ignore-paths configuration

[Eric Wong]

Lukas Pupka-Lipinski <lukas.pupkalipinski@lpl-mind.de> wrote:

Sorry for the long delay, had other stuff to deal with.
(and off-list replies with HTML doesn't take high priority)

> Am 30.03.2020 um 01:24 schrieb Eric Wong:
> > > Am 25.03.2020 um 21:38 schrieb Eric Wong:
> > > > > 2.25.1.windows.1
> > > > I'm not sure how git-config or chomp() behaves on Windows systems
> > > > with CRLF line endings, though.
> > > > 
> > > > A possibility would be replacing chomp(@v) with:
> > > > 
> > > > 	s/\r?\n\z//s for @v;
> > > Yes that has to be done. Its implemented in
> > > 
> > > +    $v  =~ s/[\x0A\x0D]//g if (defined $v);
> > Does that mean you'll send a v2 of the patch which uses
> > 
> > 	s/\r?\n\z//s for @v;
> > 
> > ?
> 
> Its already in the patch. Line
> 
> +    $v  =~ s/[\x0A\x0D]//g if (defined $v);

Please reread my original message about how `$v'
as a scalar is incorrect (and thus your s///g):

https://lore.kernel.org/git/20200325203823.GA21913@dcvr/

The correct operations are on `@v' as an array,
`$v' as a scalar.  Thanks.