Re: [PATCH]v2 GitSVN: Multi line support of ignore-path, include-paths and skiping of empty commits
- From
- Eric Wong <e@80x24.org>
- Date
- Jun 24, 2020, 03:16 UTC
- Message-ID
- <20200624031646.GA24950@dcvr>
- In-Reply-To
- <9e582571-4b82-a46f-5eb2-14d80a4c865e@lpl-mind.de>
Lukas Pupka-Lipinski <lukas.pupkalipinski@lpl-mind.de> wrote:
Show 11 quoted lines
> 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.
Show 24 quoted lines
> 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).
Show 10 quoted lines
> 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";
Show 18 quoted lines
> + 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)
Show 6 quoted lines
> $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
Show 23 quoted lines
> $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...
Show 5 quoted lines
> + 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?
Show 21 quoted lines
> + 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.