{"thread":{"id":"53373","subject":"[PATCH]v2 GitSVN: Multi line support of ignore-path, include-paths and skiping of empty commits","startedAt":"2020-05-02T09:35:56Z","lastAt":"2020-06-24T03:16:48Z","messageCount":2,"participants":["Lukas Pupka-Lipinski","Eric Wong"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"396817","messageId":"9e582571-4b82-a46f-5eb2-14d80a4c865e@lpl-mind.de","threadId":"53373","inReplyTo":null,"subject":"[PATCH]v2 GitSVN: Multi line support of ignore-path, include-paths and skiping of empty commits","fromName":"Lukas Pupka-Lipinski","fromEmail":"lukas.pupkalipinski@lpl-mind.de","sentAt":"2020-05-02T09:29:49Z","receivedAt":"2020-05-02T09:35:56Z","isPatch":true,"sender":{"key":"lukas.pupkalipinski@lpl-mind.de","avatar":null},"body":"Hi Eric,\n\nI used the ignore-paths option to ignore a lot of stuff I don’t need. \nThe ignore pattern works well, but it could and up in empty commits. So \njust the message without any modifications / changes. The patch below \nskip a commit if all changes are ignored by the ignore-paths option. In \norder to use this feature I includes the option to read configuration \nfor ignore-path, include-paths in several lines. So that the user is not \nlimited by the max. char. per line definition.\nIn Addition this patch includes the optimizations which are mansion from \nyour side.\n\n\nSigned-off-by: Lukas Pupka-Lipinski <lukas.pupkalipinski@lpl-mind.de>\n\n---\n\ndiff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\nindex 4b28b87784..fa87687306 100644\n--- a/perl/Git/SVN.pm\n+++ b/perl/Git/SVN.pm\n@@ -1188,6 +1188,22 @@ sub find_parent_branch {\n      return undef;\n  }\n\n+############################################################\n+\n+=item do_fetch()\n+\n+Fetch an Commit and returns a log entry\n+\n+Input:  $path - array of strings (Paths) in a commit\n+        $rev - Revision number\n+\n+Output: $log_entry if successfull\n+        null if skipped\n+        (die) on fetch error\n+\n+=cut\n+\n+############################################################\n  sub do_fetch {\n      my ($self, $paths, $rev) = @_;\n      my $ed;\n@@ -1212,6 +1228,11 @@ sub do_fetch {\n          }\n          $ed = Git::SVN::Fetcher->new($self);\n      }\n+    my $skip = $ed->is_empty_commit($paths);\n+    if ($skip){\n+        print \"skip commit $rev\\n\";\n+        return;\n+    }\n      unless ($self->ra->gs_do_update($last_rev, $rev, $self, $ed)) {\n          die \"SVN connection failed somewhere...\\n\";\n      }\ndiff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm\nindex 64e900a0e9..c414aa879f 100644\n--- a/perl/Git/SVN/Fetcher.pm\n+++ b/perl/Git/SVN/Fetcher.pm\n@@ -31,15 +31,17 @@ sub new {\n      # override options set in an [svn-remote \"...\"] section\n      $repo_id = $git_svn->{repo_id};\n      my $k = \"svn-remote.$repo_id.ignore-paths\";\n-    my $v = eval { command_oneline('config', '--get', $k) };\n-    $self->{ignore_regex} = $v;\n+    my @config = eval { command('config', '--get-all', $k) };\n+    chomp(@config); # Replace all \\n\\r on the end\n+    $self->{ignore_regex} = join('', @config);\n\n      $k = \"svn-remote.$repo_id.include-paths\";\n-    $v = eval { command_oneline('config', '--get', $k) };\n-    $self->{include_regex} = $v;\n+    @config = eval { command_oneline('config', '--get-all', $k) };\n+    chomp(@config); # Replace all \\n\\r on the end\n+    $self->{include_regex} = join('', @config);\n\n      $k = \"svn-remote.$repo_id.preserve-empty-dirs\";\n-    $v = eval { command_oneline('config', '--get', '--bool', $k) };\n+    my $v = eval { command_oneline('config', '--get', '--bool', $k) };\n      if ($v && $v eq 'true') {\n          $_preserve_empty_dirs = 1;\n          $k = \"svn-remote.$repo_id.placeholder-filename\";\n@@ -137,6 +139,37 @@ sub is_path_ignored {\n      return 0;\n  }\n\n+############################################################\n+\n+=item is_empty_commit()\n+\n+Return 1 if all given $paths are ignored, so that this commit end up in \nan empty commit\n+\n+Input:  $path - array of strings (Paths) in a commit\n+\n+Output: { 1 if true, 0 if false }\n+\n+=cut\n+\n+############################################################\n+sub is_empty_commit{\n+    my ($self, $paths) = @_;\n+    my $path = \"\";\n+    unless (defined($self->{include_regex})){\n+        return 0;\n+    }\n+\n+    foreach $path (keys %$paths){\n+        unless (defined $path && -d $path ){\n+            my $ignored = $self->is_path_ignored($path);\n+            if (!$ignored){\n+                return 0;\n+            }\n+        }\n+    }\n+    return 1;\n+}\n+\n  sub set_path_strip {\n      my ($self, $path) = @_;\n      $self->{path_strip} = qr/^\\Q$path\\E(\\/|$)/ if length $path;\ndiff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm\nindex 56ad9870bc..63be69dc12 100644\n--- a/perl/Git/SVN/Ra.pm\n+++ b/perl/Git/SVN/Ra.pm\n@@ -475,6 +475,8 @@ sub gs_fetch_loop_common {\n                  my $log_entry = $gs->do_fetch($paths, $r);\n                  if ($log_entry) {\n                      $gs->do_git_commit($log_entry);\n+                }else{\n+                    next;\n                  }\n                  $Git::SVN::INDEX_FILES{$gs->{index}} = 1;\n              }\n\n\n\ndiff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\nindex 4b28b87784..fa87687306 100644\n--- a/perl/Git/SVN.pm\n+++ b/perl/Git/SVN.pm\n@@ -1188,6 +1188,22 @@ sub find_parent_branch {\n \treturn undef;\n }\n \n+############################################################\n+\n+=item do_fetch()\n+\n+Fetch an Commit and returns a log entry\n+\n+Input:  $path - array of strings (Paths) in a commit\n+\t\t$rev - Revision number\n+\n+Output: $log_entry if successfull\n+\t\tnull if skipped\n+\t\t(die) on fetch error\n+\n+=cut\n+\n+############################################################\n sub do_fetch {\n \tmy ($self, $paths, $rev) = @_;\n \tmy $ed;\n@@ -1212,6 +1228,11 @@ sub do_fetch {\n \t\t}\n \t\t$ed = Git::SVN::Fetcher->new($self);\n \t}\n+\tmy $skip = $ed->is_empty_commit($paths);\n+\tif ($skip){\n+\t\tprint \"skip commit $rev\\n\";\n+\t\treturn;\n+\t}\n \tunless ($self->ra->gs_do_update($last_rev, $rev, $self, $ed)) {\n \t\tdie \"SVN connection failed somewhere...\\n\";\n \t}\ndiff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm\nindex 64e900a0e9..c414aa879f 100644\n--- a/perl/Git/SVN/Fetcher.pm\n+++ b/perl/Git/SVN/Fetcher.pm\n@@ -31,15 +31,17 @@ sub new {\n \t# override options set in an [svn-remote \"...\"] section\n \t$repo_id = $git_svn->{repo_id};\n \tmy $k = \"svn-remote.$repo_id.ignore-paths\";\n-\tmy $v = eval { command_oneline('config', '--get', $k) };\n-\t$self->{ignore_regex} = $v;\n+\tmy @config = eval { command('config', '--get-all', $k) };\n+\tchomp(@config); # Replace all \\n\\r on the end\n+\t$self->{ignore_regex} = join('', @config);\n \n \t$k = \"svn-remote.$repo_id.include-paths\";\n-\t$v = eval { command_oneline('config', '--get', $k) };\n-\t$self->{include_regex} = $v;\n+\t@config = eval { command_oneline('config', '--get-all', $k) };\n+\tchomp(@config); # Replace all \\n\\r on the end\n+\t$self->{include_regex} = join('', @config);\n \n \t$k = \"svn-remote.$repo_id.preserve-empty-dirs\";\n-\t$v = eval { command_oneline('config', '--get', '--bool', $k) };\n+\tmy $v = eval { command_oneline('config', '--get', '--bool', $k) };\n \tif ($v && $v eq 'true') {\n \t\t$_preserve_empty_dirs = 1;\n \t\t$k = \"svn-remote.$repo_id.placeholder-filename\";\n@@ -137,6 +139,37 @@ sub is_path_ignored {\n \treturn 0;\n }\n \n+############################################################\n+\n+=item is_empty_commit()\n+\n+Return 1 if all given $paths are ignored, so that this commit end up in an empty commit\n+\n+Input:  $path - array of strings (Paths) in a commit\n+\n+Output: { 1 if true, 0 if false }\n+\n+=cut\n+\n+############################################################\n+sub is_empty_commit{\n+\tmy ($self, $paths) = @_;\n+\tmy $path = \"\";\n+\tunless (defined($self->{include_regex})){\n+\t\treturn 0;\n+\t}\n+\n+\tforeach $path (keys %$paths){\n+\t\tunless (defined $path && -d $path ){\n+\t\t\tmy $ignored = $self->is_path_ignored($path);\n+\t\t\tif (!$ignored){\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t}\n+\t}\n+\treturn 1;\n+}\n+\n sub set_path_strip {\n \tmy ($self, $path) = @_;\n \t$self->{path_strip} = qr/^\\Q$path\\E(\\/|$)/ if length $path;\ndiff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm\nindex 56ad9870bc..63be69dc12 100644\n--- a/perl/Git/SVN/Ra.pm\n+++ b/perl/Git/SVN/Ra.pm\n@@ -475,6 +475,8 @@ sub gs_fetch_loop_common {\n \t\t\t\tmy $log_entry = $gs->do_fetch($paths, $r);\n \t\t\t\tif ($log_entry) {\n \t\t\t\t\t$gs->do_git_commit($log_entry);\n+\t\t\t\t}else{\n+\t\t\t\t\tnext;\n \t\t\t\t}\n \t\t\t\t$Git::SVN::INDEX_FILES{$gs->{index}} = 1;\n \t\t\t}\n"},{"id":"400487","messageId":"20200624031646.GA24950@dcvr","threadId":"53373","inReplyTo":"9e582571-4b82-a46f-5eb2-14d80a4c865e@lpl-mind.de","subject":"Re: [PATCH]v2 GitSVN: Multi line support of ignore-path, include-paths and skiping of empty commits","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2020-06-24T03:16:46Z","receivedAt":"2020-06-24T03:16:48Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Lukas Pupka-Lipinski <lukas.pupkalipinski@lpl-mind.de> wrote:\n> Hi Eric,\n> \n> I used the ignore-paths option to ignore a lot of stuff I don’t need. The\n> ignore pattern works well, but it could and up in empty commits. So just the\n> message without any modifications / changes. The patch below skip a commit\n> if all changes are ignored by the ignore-paths option. In order to use this\n> feature I includes the option to read configuration for ignore-path,\n> include-paths in several lines. So that the user is not limited by the max.\n> char. per line definition.\n> In Addition this patch includes the optimizations which are mansion from\n> your side.\n\nHi Lukas, sorry for the delay.\n\nThe above is not a proper commit message.  Please see\nDocumentation/SubmittingPatches and git.git history\nfor some examples.  I or someone else here can help you\nwrite it if you need help.\n\nComments inline.\n\n> Signed-off-by: Lukas Pupka-Lipinski <lukas.pupkalipinski@lpl-mind.de>\n\nI'm skipping the whitespace damaged inline version and\nreviewing the the attachment.\n\n> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\n> index 4b28b87784..fa87687306 100644\n> --- a/perl/Git/SVN.pm\n> +++ b/perl/Git/SVN.pm\n> @@ -1188,6 +1188,22 @@ sub find_parent_branch {\n>  \treturn undef;\n>  }\n>  \n> +############################################################\n> +\n> +=item do_fetch()\n> +\n> +Fetch an Commit and returns a log entry\n> +\n> +Input:  $path - array of strings (Paths) in a commit\n> +\t\t$rev - Revision number\n> +\n> +Output: $log_entry if successfull\n> +\t\tnull if skipped\n> +\t\t(die) on fetch error\n> +\n> +=cut\n> +\n> +############################################################\n\nDocumentation is good, but the above takes up more than half my\nterminal.  A single line is more than sufficient.\n(I have poor eyesight and need very big fonts).\n\n>  sub do_fetch {\n>  \tmy ($self, $paths, $rev) = @_;\n>  \tmy $ed;\n> @@ -1212,6 +1228,11 @@ sub do_fetch {\n>  \t\t}\n>  \t\t$ed = Git::SVN::Fetcher->new($self);\n>  \t}\n> +\tmy $skip = $ed->is_empty_commit($paths);\n> +\tif ($skip){\n> +\t\tprint \"skip commit $rev\\n\";\n\nI think \"skipping\" is a better word to use, here (I'm not an\nEnglish expert, either).  To be unambiguous, we prefix SVN\nrevision numbers with \"r\" (just like SVN does):\n\n\t\tprint \"skipping commit r$rev\\n\";\n\n> +\t\treturn;\n> +\t}\n>  \tunless ($self->ra->gs_do_update($last_rev, $rev, $self, $ed)) {\n>  \t\tdie \"SVN connection failed somewhere...\\n\";\n>  \t}\n> diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm\n> index 64e900a0e9..c414aa879f 100644\n> --- a/perl/Git/SVN/Fetcher.pm\n> +++ b/perl/Git/SVN/Fetcher.pm\n> @@ -31,15 +31,17 @@ sub new {\n>  \t# override options set in an [svn-remote \"...\"] section\n>  \t$repo_id = $git_svn->{repo_id};\n>  \tmy $k = \"svn-remote.$repo_id.ignore-paths\";\n> -\tmy $v = eval { command_oneline('config', '--get', $k) };\n> -\t$self->{ignore_regex} = $v;\n> +\tmy @config = eval { command('config', '--get-all', $k) };\n> +\tchomp(@config); # Replace all \\n\\r on the end\n> +\t$self->{ignore_regex} = join('', @config);\n\nThat join() doesn't make a correct regexp with multiple values\nin @config.  I think you would need the following to make\na proper regexp:\n\n\t$self->{ignore_regex} = '(?:'.join('|', @config).')';\n\n\t(totally untested, please test with multiple values)\n\n>  \t$k = \"svn-remote.$repo_id.include-paths\";\n> -\t$v = eval { command_oneline('config', '--get', $k) };\n> -\t$self->{include_regex} = $v;\n> +\t@config = eval { command_oneline('config', '--get-all', $k) };\n> +\tchomp(@config); # Replace all \\n\\r on the end\n> +\t$self->{include_regex} = join('', @config);\n\nSame comment as above w.r.t multi-value @config\n\n>  \t$k = \"svn-remote.$repo_id.preserve-empty-dirs\";\n> -\t$v = eval { command_oneline('config', '--get', '--bool', $k) };\n> +\tmy $v = eval { command_oneline('config', '--get', '--bool', $k) };\n>  \tif ($v && $v eq 'true') {\n>  \t\t$_preserve_empty_dirs = 1;\n>  \t\t$k = \"svn-remote.$repo_id.placeholder-filename\";\n> @@ -137,6 +139,37 @@ sub is_path_ignored {\n>  \treturn 0;\n>  }\n>  \n> +############################################################\n> +\n> +=item is_empty_commit()\n> +\n> +Return 1 if all given $paths are ignored, so that this commit end up in an empty commit\n> +\n> +Input:  $path - array of strings (Paths) in a commit\n> +\n> +Output: { 1 if true, 0 if false }\n> +\n> +=cut\n> +\n> +############################################################\n\nSame comment w.r.t. excessive screen space for comments.\n\n> +sub is_empty_commit{\n> +\tmy ($self, $paths) = @_;\n> +\tmy $path = \"\";\n\nDon't need to initialize $path, here...\n\n> +\tunless (defined($self->{include_regex})){\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tforeach $path (keys %$paths){\n\nInstead, localize variables within the loop:\n\n    foreach my $path (keys %$paths) {\n\nAlso, both our Perl and C code prefer a space in-between \"){\",\nso \") {\", everywhere, and no need to have \" )\" when \")\" will\ndo (same goes for the rest of the codebase).\n\n> +\t\tunless (defined $path && -d $path ){\n\nIt's been a while since I looked at this code; but\nGit::SVN::Fetcher should NEVER be comparing paths\nfrom SVN vs paths from the filesystem via `-d' or\nsimilar stat() ops.\n\nIn other words, \"git svn fetch\" never touches the\ngit working tree, it should work on bare git repos.\n\nCan the just next 4 lines work w/o the unless wrapping it?\n\n> +\t\t\tmy $ignored = $self->is_path_ignored($path);\n> +\t\t\tif (!$ignored){\n> +\t\t\t\treturn 0;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\treturn 1;\n> +}\n> +\n>  sub set_path_strip {\n>  \tmy ($self, $path) = @_;\n>  \t$self->{path_strip} = qr/^\\Q$path\\E(\\/|$)/ if length $path;\n> diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm\n> index 56ad9870bc..63be69dc12 100644\n> --- a/perl/Git/SVN/Ra.pm\n> +++ b/perl/Git/SVN/Ra.pm\n> @@ -475,6 +475,8 @@ sub gs_fetch_loop_common {\n>  \t\t\t\tmy $log_entry = $gs->do_fetch($paths, $r);\n>  \t\t\t\tif ($log_entry) {\n>  \t\t\t\t\t$gs->do_git_commit($log_entry);\n> +\t\t\t\t}else{\n\nAgain, in both C and Perl, we write \"} else {\"\n\n> +\t\t\t\t\tnext;\n>  \t\t\t\t}\n>  \t\t\t\t$Git::SVN::INDEX_FILES{$gs->{index}} = 1;\n>  \t\t\t}\n\nThanks.\n"}]}