From: Eric Wong <e@80x24.org>
To: Lukas Pupka-Lipinski <lukas.pupkalipinski@lpl-mind.de>
Cc: git@vger.kernel.org
Subject: Re: [PATCH]v2 GitSVN: Multi line support of ignore-path, include-paths and skiping of empty commits
Date: Wed, 24 Jun 2020 03:16:46 +0000 [thread overview]
Message-ID: <20200624031646.GA24950@dcvr> (raw)
In-Reply-To: <9e582571-4b82-a46f-5eb2-14d80a4c865e@lpl-mind.de>
Lukas Pupka-Lipinski <lukas.pupkalipinski@lpl-mind.de> wrote:
> Hi Eric,
>
> I used the ignore-paths option to ignore a lot of stuff I don’t need. The
> ignore pattern works well, but it could and up in empty commits. So just the
> message without any modifications / changes. The patch below skip a commit
> if all changes are ignored by the ignore-paths option. In order to use this
> feature I includes the option to read configuration for ignore-path,
> include-paths in several lines. So that the user is not limited by the max.
> char. per line definition.
> In Addition this patch includes the optimizations which are mansion from
> your side.
Hi Lukas, sorry for the delay.
The above is not a proper commit message. Please see
Documentation/SubmittingPatches and git.git history
for some examples. I or someone else here can help you
write it if you need help.
Comments inline.
> Signed-off-by: Lukas Pupka-Lipinski <lukas.pupkalipinski@lpl-mind.de>
I'm skipping the whitespace damaged inline version and
reviewing the the attachment.
> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
> index 4b28b87784..fa87687306 100644
> --- a/perl/Git/SVN.pm
> +++ b/perl/Git/SVN.pm
> @@ -1188,6 +1188,22 @@ sub find_parent_branch {
> return undef;
> }
>
> +############################################################
> +
> +=item do_fetch()
> +
> +Fetch an Commit and returns a log entry
> +
> +Input: $path - array of strings (Paths) in a commit
> + $rev - Revision number
> +
> +Output: $log_entry if successfull
> + null if skipped
> + (die) on fetch error
> +
> +=cut
> +
> +############################################################
Documentation is good, but the above takes up more than half my
terminal. A single line is more than sufficient.
(I have poor eyesight and need very big fonts).
> sub do_fetch {
> my ($self, $paths, $rev) = @_;
> my $ed;
> @@ -1212,6 +1228,11 @@ sub do_fetch {
> }
> $ed = Git::SVN::Fetcher->new($self);
> }
> + my $skip = $ed->is_empty_commit($paths);
> + if ($skip){
> + print "skip commit $rev\n";
I think "skipping" is a better word to use, here (I'm not an
English expert, either). To be unambiguous, we prefix SVN
revision numbers with "r" (just like SVN does):
print "skipping commit r$rev\n";
> + return;
> + }
> unless ($self->ra->gs_do_update($last_rev, $rev, $self, $ed)) {
> die "SVN connection failed somewhere...\n";
> }
> diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm
> index 64e900a0e9..c414aa879f 100644
> --- a/perl/Git/SVN/Fetcher.pm
> +++ b/perl/Git/SVN/Fetcher.pm
> @@ -31,15 +31,17 @@ 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) };
> - $self->{ignore_regex} = $v;
> + my @config = eval { command('config', '--get-all', $k) };
> + chomp(@config); # Replace all \n\r on the end
> + $self->{ignore_regex} = join('', @config);
That join() doesn't make a correct regexp with multiple values
in @config. I think you would need the following to make
a proper regexp:
$self->{ignore_regex} = '(?:'.join('|', @config).')';
(totally untested, please test with multiple values)
> $k = "svn-remote.$repo_id.include-paths";
> - $v = eval { command_oneline('config', '--get', $k) };
> - $self->{include_regex} = $v;
> + @config = eval { command_oneline('config', '--get-all', $k) };
> + chomp(@config); # Replace all \n\r on the end
> + $self->{include_regex} = join('', @config);
Same comment as above w.r.t multi-value @config
> $k = "svn-remote.$repo_id.preserve-empty-dirs";
> - $v = eval { command_oneline('config', '--get', '--bool', $k) };
> + my $v = eval { command_oneline('config', '--get', '--bool', $k) };
> if ($v && $v eq 'true') {
> $_preserve_empty_dirs = 1;
> $k = "svn-remote.$repo_id.placeholder-filename";
> @@ -137,6 +139,37 @@ sub is_path_ignored {
> return 0;
> }
>
> +############################################################
> +
> +=item is_empty_commit()
> +
> +Return 1 if all given $paths are ignored, so that this commit end up in an empty commit
> +
> +Input: $path - array of strings (Paths) in a commit
> +
> +Output: { 1 if true, 0 if false }
> +
> +=cut
> +
> +############################################################
Same comment w.r.t. excessive screen space for comments.
> +sub is_empty_commit{
> + my ($self, $paths) = @_;
> + my $path = "";
Don't need to initialize $path, here...
> + unless (defined($self->{include_regex})){
> + return 0;
> + }
> +
> + foreach $path (keys %$paths){
Instead, localize variables within the loop:
foreach my $path (keys %$paths) {
Also, both our Perl and C code prefer a space in-between "){",
so ") {", everywhere, and no need to have " )" when ")" will
do (same goes for the rest of the codebase).
> + unless (defined $path && -d $path ){
It's been a while since I looked at this code; but
Git::SVN::Fetcher should NEVER be comparing paths
from SVN vs paths from the filesystem via `-d' or
similar stat() ops.
In other words, "git svn fetch" never touches the
git working tree, it should work on bare git repos.
Can the just next 4 lines work w/o the unless wrapping it?
> + my $ignored = $self->is_path_ignored($path);
> + if (!$ignored){
> + return 0;
> + }
> + }
> + }
> + return 1;
> +}
> +
> sub set_path_strip {
> my ($self, $path) = @_;
> $self->{path_strip} = qr/^\Q$path\E(\/|$)/ if length $path;
> diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
> index 56ad9870bc..63be69dc12 100644
> --- a/perl/Git/SVN/Ra.pm
> +++ b/perl/Git/SVN/Ra.pm
> @@ -475,6 +475,8 @@ sub gs_fetch_loop_common {
> my $log_entry = $gs->do_fetch($paths, $r);
> if ($log_entry) {
> $gs->do_git_commit($log_entry);
> + }else{
Again, in both C and Perl, we write "} else {"
> + next;
> }
> $Git::SVN::INDEX_FILES{$gs->{index}} = 1;
> }
Thanks.
prev parent reply other threads:[~2020-06-24 3:16 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-02 9:29 [PATCH]v2 GitSVN: Multi line support of ignore-path, include-paths and skiping of empty commits Lukas Pupka-Lipinski
2020-06-24 3:16 ` Eric Wong [this message]
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=20200624031646.GA24950@dcvr \
--to=e@80x24.org \
--cc=git@vger.kernel.org \
--cc=lukas.pupkalipinski@lpl-mind.de \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public 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).