diff mbox series

v2 GitSVN: Multi line support of ignore-path, include-paths and skiping of empty commits

Message ID 9e582571-4b82-a46f-5eb2-14d80a4c865e@lpl-mind.de (mailing list archive)
State New, archived
Headers show
Series v2 GitSVN: Multi line support of ignore-path, include-paths and skiping of empty commits | expand

Commit Message

Lukas Pupka-Lipinski May 2, 2020, 9:29 a.m. UTC 
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.


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

---
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
+
+############################################################
 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";
+		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);
 
 	$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);
 
 	$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
+
+############################################################
+sub is_empty_commit{
+	my ($self, $paths) = @_;
+	my $path = "";
+	unless (defined($self->{include_regex})){
+		return 0;
+	}
+
+	foreach $path (keys %$paths){
+		unless (defined $path && -d $path ){
+			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{
+					next;
 				}
 				$Git::SVN::INDEX_FILES{$gs->{index}} = 1;
 			}

Comments

Eric Wong June 24, 2020, 3:16 a.m. UTC | #1 
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.
diff mbox series

Patch

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
+
+############################################################
  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";
+        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);

      $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);

      $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
+
+############################################################
+sub is_empty_commit{
+    my ($self, $paths) = @_;
+    my $path = "";
+    unless (defined($self->{include_regex})){
+        return 0;
+    }
+
+    foreach $path (keys %$paths){
+        unless (defined $path && -d $path ){
+            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{
+                    next;
                  }
                  $Git::SVN::INDEX_FILES{$gs->{index}} = 1;
              }