git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Eric Wong <e@80x24.org>
To: Lukas Pupka-Lipinski <lukas.pupkalipinski@lpl-mind.de>
Cc: git@vger.kernel.org, pjwhams@gmail.com
Subject: Re: [PATCH] added: Multi line support for ignore-paths configuration
Date: Wed, 25 Mar 2020 20:38:23 +0000	[thread overview]
Message-ID: <20200325203823.GA21913@dcvr> (raw)
In-Reply-To: <e5c78a24-e17f-c1bb-4ea7-3ddaa45abcf0@lpl-mind.de>

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.

  reply	other threads:[~2020-03-25 20:38 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-25  6:56 Lukas Pupka-Lipinski
2020-03-25 20:38 ` Eric Wong [this message]
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
     [not found]       ` <8fbfbf1c-ea14-6739-7881-cfa3d9642de9@lpl-mind.de>
2020-04-13 18:18         ` Eric Wong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200325203823.GA21913@dcvr \
    --to=e@80x24.org \
    --cc=git@vger.kernel.org \
    --cc=lukas.pupkalipinski@lpl-mind.de \
    --cc=pjwhams@gmail.com \
    --subject='Re: [PATCH] added: Multi line support for ignore-paths configuration' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).