git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [PATCH] added: Multi line support for ignore-paths configuration
@ 2020-03-25  6:56 Lukas Pupka-Lipinski
  2020-03-25 20:38 ` Eric Wong
  0 siblings, 1 reply; 4+ messages in thread
From: Lukas Pupka-Lipinski @ 2020-03-25  6:56 UTC (permalink / raw)
  To: e; +Cc: git, pjwhams, normalperson

 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] added: Multi line support for ignore-paths configuration
  2020-03-25  6:56 [PATCH] added: Multi line support for ignore-paths configuration Lukas Pupka-Lipinski
@ 2020-03-25 20:38 ` Eric Wong
  2020-03-25 21:24   ` Junio C Hamano
       [not found]   ` <0515a11b-d9ae-3f22-65a8-5efee235d5c9@lpl-mind.de>
  0 siblings, 2 replies; 4+ messages in thread
From: Eric Wong @ 2020-03-25 20:38 UTC (permalink / raw)
  To: Lukas Pupka-Lipinski; +Cc: git, pjwhams

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] added: Multi line support for ignore-paths configuration
  2020-03-25 20:38 ` Eric Wong
@ 2020-03-25 21:24   ` Junio C Hamano
       [not found]   ` <0515a11b-d9ae-3f22-65a8-5efee235d5c9@lpl-mind.de>
  1 sibling, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2020-03-25 21:24 UTC (permalink / raw)
  To: Eric Wong; +Cc: Lukas Pupka-Lipinski, git, pjwhams

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.


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] added: Multi line support for ignore-paths configuration
       [not found]   ` <0515a11b-d9ae-3f22-65a8-5efee235d5c9@lpl-mind.de>
@ 2020-03-29 23:24     ` Eric Wong
  0 siblings, 0 replies; 4+ messages in thread
From: Eric Wong @ 2020-03-29 23:24 UTC (permalink / raw)
  To: Lukas Pupka-Lipinski; +Cc: git

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-25  6:56 [PATCH] added: Multi line support for ignore-paths configuration Lukas Pupka-Lipinski
2020-03-25 20:38 ` Eric Wong
2020-03-25 21:24   ` Junio C Hamano
     [not found]   ` <0515a11b-d9ae-3f22-65a8-5efee235d5c9@lpl-mind.de>
2020-03-29 23:24     ` Eric Wong

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git