user/dev discussion of public-inbox itself
 help / color / Atom feed
* [PATCH 0/3] solver bugfixes and tweaks
@ 2020-01-02  9:24 Eric Wong
  2020-01-02  9:24 ` [PATCH 1/3] solver: try the next patch on apply failures Eric Wong
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Eric Wong @ 2020-01-02  9:24 UTC (permalink / raw)
  To: meta

A couple of improvements from errors I've seen with solver
and hopefully no regressions.  [PATCH 2/3] is a bit scary :x

Eric Wong (3):
  solver: try the next patch on apply failures
  solver: extract_diff: deal with missing "diff --git" line
  qspawn: use per-call quiet flag for solver

 lib/PublicInbox/Qspawn.pm    |   6 +-
 lib/PublicInbox/SolverGit.pm | 168 ++++++++++++++++++-----------------
 2 files changed, 89 insertions(+), 85 deletions(-)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] solver: try the next patch on apply failures
  2020-01-02  9:24 [PATCH 0/3] solver bugfixes and tweaks Eric Wong
@ 2020-01-02  9:24 ` Eric Wong
  2020-01-02  9:24 ` [PATCH 2/3] solver: extract_diff: deal with missing "diff --git" line Eric Wong
  2020-01-02  9:24 ` [PATCH 3/3] qspawn: use per-call quiet flag for solver Eric Wong
  2 siblings, 0 replies; 8+ messages in thread
From: Eric Wong @ 2020-01-02  9:24 UTC (permalink / raw)
  To: meta

Sometimes a patch is corrupted and resent to create the same
OID.  We need to account for that case and actually move onto
the next patch instead of blindly trying "git ls-files" to get
nothing out of it.
---
 lib/PublicInbox/SolverGit.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm
index c57fb4c6..3e3a5899 100644
--- a/lib/PublicInbox/SolverGit.pm
+++ b/lib/PublicInbox/SolverGit.pm
@@ -452,6 +452,7 @@ sub apply_result ($$) {
 		if ($nxt && oids_same_ish($nxt->{oid_b}, $di->{oid_b})) {
 			dbg($self, $msg);
 			dbg($self, 'trying '.di_url($self, $nxt));
+			return do_git_apply($self);
 		} else {
 			ERR($self, $msg);
 		}

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/3] solver: extract_diff: deal with missing "diff --git" line
  2020-01-02  9:24 [PATCH 0/3] solver bugfixes and tweaks Eric Wong
  2020-01-02  9:24 ` [PATCH 1/3] solver: try the next patch on apply failures Eric Wong
@ 2020-01-02  9:24 ` Eric Wong
  2020-01-02  9:36   ` Eric Wong
                     ` (2 more replies)
  2020-01-02  9:24 ` [PATCH 3/3] qspawn: use per-call quiet flag for solver Eric Wong
  2 siblings, 3 replies; 8+ messages in thread
From: Eric Wong @ 2020-01-02  9:24 UTC (permalink / raw)
  To: meta

Rewrite the patch extraction loop using a single regexp which
accounts for missing "diff --git ..." lines and is capable of
extracting pathnames off the "+++ b/foo" line.

This fixes the solving of blob "96f1c7f" off
<2841d2de-32ad-eae8-6039-9251a40bb00e@tngtech.com>
in git@vger archives.
---
 lib/PublicInbox/SolverGit.pm | 165 ++++++++++++++++++-----------------
 1 file changed, 85 insertions(+), 80 deletions(-)

diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm
index 3e3a5899..e1b82f3e 100644
--- a/lib/PublicInbox/SolverGit.pm
+++ b/lib/PublicInbox/SolverGit.pm
@@ -10,6 +10,7 @@
 package PublicInbox::SolverGit;
 use strict;
 use warnings;
+use 5.010_001;
 use File::Temp 0.19 ();
 use Fcntl qw(SEEK_SET);
 use PublicInbox::Git qw(git_unquote git_quote);
@@ -38,7 +39,7 @@ my $MAX_PATCH = 9999;
 #	oid_a => abbreviated pre-image oid,
 #	oid_b => abbreviated post-image oid,
 #	tmp => anonymous file handle with the diff,
-#	hdr_lines => arrayref of various header lines for mode information
+#	hdr_lines => string of various header lines for mode information
 #	mode_a => original mode of oid_a (string, not integer),
 #	ibx => PublicInbox::Inbox object containing the diff
 #	smsg => PublicInbox::SearchMsg object containing diff
@@ -47,10 +48,6 @@ my $MAX_PATCH = 9999;
 #	n => numeric path of the patch (relative to worktree)
 # }
 
-# don't bother if somebody sends us a patch with these path components,
-# it's junk at best, an attack attempt at worse:
-my %bad_component = map { $_ => 1 } ('', '.', '..');
-
 sub dbg ($$) {
 	print { $_[0]->{out} } $_[1], "\n" or ERR($_[0], "print(dbg): $!");
 }
@@ -100,10 +97,8 @@ sub solve_existing ($$) {
 
 sub extract_diff ($$) {
 	my ($p, $arg) = @_;
-	my ($self, $diffs, $re, $ibx, $smsg) = @$arg;
+	my ($self, $diffs, $pre, $post, $ibx, $smsg) = @$arg;
 	my ($part) = @$p; # ignore $depth and @idx;
-	my $hdr_lines; # diff --git a/... b/...
-	my $tmp;
 	my $ct = $part->content_type || 'text/plain';
 	my ($s, undef) = msg_part_text($part, $ct);
 	defined $s or return;
@@ -116,66 +111,85 @@ sub extract_diff ($$) {
 		$s =~ s/\r\n/\n/sg;
 	}
 
-	foreach my $l (split(/^/m, $s)) {
-		if ($l =~ $re) {
-			$di->{oid_a} = $1;
-			$di->{oid_b} = $2;
-			if (defined($3)) {
-				my $mode_a = $3;
-				if ($mode_a =~ /\A(?:100644|120000|100755)\z/) {
-					$di->{mode_a} = $mode_a;
-				}
-			}
-
-
-			# start writing the diff out to a tempfile
-			my $path = ++$self->{tot};
-			$di->{n} = $path;
-			open($tmp, '>', $self->{tmp}->dirname . "/$path") or
-							die "open(tmp): $!";
-
-			push @$hdr_lines, $l;
-			$di->{hdr_lines} = $hdr_lines;
-			utf8::encode($_) for @$hdr_lines;
-			print $tmp @$hdr_lines or die "print(tmp): $!";
-
-			# for debugging/diagnostics:
-			$di->{ibx} = $ibx;
-			$di->{smsg} = $smsg;
-		} elsif ($l =~ m!\Adiff --git ("?[^/]+/.+) ("?[^/]+/.+)$!) {
-			last if $tmp; # got our blob, done!
-
-			my ($path_a, $path_b) = ($1, $2);
-
-			# diff header lines won't have \r because git
-			# will quote them, but Email::MIME gives CRLF
-			# for quoted-printable:
-			$path_b =~ tr/\r//d;
-
-			# don't care for leading 'a/' and 'b/'
-			my (undef, @a) = split(m{/}, git_unquote($path_a));
-			my (undef, @b) = split(m{/}, git_unquote($path_b));
-
-			# get rid of path-traversal attempts and junk patches:
-			foreach (@a, @b) {
-				return if $bad_component{$_};
-			}
-
-			$di->{path_a} = join('/', @a);
-			$di->{path_b} = join('/', @b);
-			$hdr_lines = [ $l ];
-		} elsif ($tmp) {
-			utf8::encode($l);
-			print $tmp $l or die "print(tmp): $!";
-		} elsif ($hdr_lines) {
-			push @$hdr_lines, $l;
-			if ($l =~ /\Anew file mode (100644|120000|100755)$/) {
-				$di->{mode_a} = $1;
-			}
-		}
-	}
-	return undef unless $tmp;
+	state $LF = qr!\r?\n!;
+	state $FN = qr!(?:("?[^/\n]+/[^\r\n]+)|/dev/null)!;
+
+	$s =~ m!( # $1 start header lines we save for debugging:
+
+		# everything before ^index is optional, but we don't
+		# want to match ^(old|copy|rename|deleted|...) unless
+		# we match /^diff --git/ first:
+		(?: # begin optional stuff:
+
+		# try to get the pre-and-post filenames as $2 and $3
+		(?:^diff\x20--git\x20$FN\x20$FN$LF)
+
+		# old mode $4
+		(?:^old mode\x20(100644|120000|100755)$LF)?
+
+		# ignore other info
+		(?:^(?:copy|rename|deleted|dissimilarity|similarity).*$LF)?
+
+		# new mode (possibly new file) ($5)
+		(?:^new\x20(?:file\x20)?mode\x20(100644|120000|100755)$LF)?
+
+		# ignore other info
+		(?:^(?:copy|rename|deleted|dissimilarity|similarity).*$LF)?
+
+		)? # end of optional stuff, everything below is required
+
+		# match the pre and post-image OIDs as $6 $7
+		^index\x20(${pre}[a-f0-9]*)\.\.(${post}[a-f0-9]*)
+			# mode if unchanged $8
+			(?:\x20(100644|120000|100755))?$LF
+	) # end of header lines ($1)
+	( # $9 is the patch body
+		# "--- a/foo.c" sets pre-filename ($10) in case
+		# $2 is missing
+		(?:^---\x20$FN$LF)
+
+		# "+++ b/foo.c" sets post-filename ($11) in case
+		# $3 is missing
+		(?:^\+{3}\x20$FN$LF)
+
+		# the meat of the diff, including "^\\No newline ..."
+		(?:^[\@\+\x20\-\\][^\r\n]*$LF)+
+	)!smx or return;
+
+	my $hdr_lines = $1;
+	my $path_a = $2 // $10;
+	my $path_b = $3 // $11;
+	$di->{oid_a} = $6;
+	$di->{oid_b} = $7;
+	$di->{mode_a} = $5 // $8 // $6; # new (file) // unchanged // old
+	my $patch = $9;
+
+	# don't care for leading 'a/' and 'b/'
+	my (undef, @a) = split(m{/}, git_unquote($path_a));
+	my (undef, @b) = split(m{/}, git_unquote($path_b));
+
+	# get rid of path-traversal attempts and junk patches:
+	# it's junk at best, an attack attempt at worse:
+	state %bad_component = map { $_ => 1 } ('', '.', '..');
+	foreach (@a, @b) { return if $bad_component{$_} }
+
+	$di->{path_a} = join('/', @a);
+	$di->{path_b} = join('/', @b);
+
+	utf8::encode($hdr_lines);
+	utf8::encode($patch);
+	my $path = ++$self->{tot};
+	$di->{n} = $path;
+	open(my $tmp, '>', $self->{tmp}->dirname . "/$path") or
+		die "open(tmp): $!";
+	print $tmp $hdr_lines, $patch or die "print(tmp): $!";
 	close $tmp or die "close(tmp): $!";
+
+	# for debugging/diagnostics:
+	$di->{ibx} = $ibx;
+	$di->{smsg} = $smsg;
+	$di->{hdr_lines} = $hdr_lines;
+
 	push @$diffs, $di;
 }
 
@@ -214,13 +228,13 @@ sub find_extract_diffs ($$$) {
 	}
 
 	my $msgs = $srch->query($q, { relevance => 1 });
-	my $re = qr/\Aindex ($pre[a-f0-9]*)\.\.($post[a-f0-9]*)(?: ([0-9]+))?/;
+
 	my $diffs = [];
 	foreach my $smsg (@$msgs) {
 		$ibx->smsg_mime($smsg) or next;
 		my $mime = delete $smsg->{mime};
 		msg_iter($mime, \&extract_diff,
-				[$self, $diffs, $re, $ibx, $smsg]);
+				[$self, $diffs, $pre, $post, $ibx, $smsg]);
 	}
 	@$diffs ? $diffs : undef;
 }
@@ -251,7 +265,7 @@ sub prepare_index ($) {
 
 	my $oid_full = $existing->[1];
 	my $path_a = $di->{path_a} or die "BUG: path_a missing for $oid_full";
-	my $mode_a = $di->{mode_a} || extract_old_mode($di);
+	my $mode_a = $di->{mode_a} // '100644';
 
 	my $in = tmpfile("update-index.$oid_full") or die "tmpfile: $!";
 	print $in "$mode_a $oid_full\t$path_a\0" or die "print: $!";
@@ -307,15 +321,6 @@ EOF
 	prepare_index($self);
 }
 
-sub extract_old_mode ($) {
-	my ($di) = @_;
-	if (join('', @{$di->{hdr_lines}}) =~
-			/^old mode (100644|100755|120000)\b/) {
-		return $1;
-	}
-	'100644';
-}
-
 sub do_finish ($) {
 	my ($self) = @_;
 	my ($found, $oid_want) = @$self{qw(found oid_want)};
@@ -484,7 +489,7 @@ sub do_git_apply ($) {
 		my $i = ++$self->{nr};
 		$di = shift @$patches;
 		dbg($self, "\napplying [$i/$total] " . di_url($self, $di) .
-			"\n" . join('', @{$di->{hdr_lines}}));
+			"\n" . $di->{hdr_lines});
 		my $path = $di->{n};
 		$len += length($path) + 1;
 		push @cmd, $path;

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/3] qspawn: use per-call quiet flag for solver
  2020-01-02  9:24 [PATCH 0/3] solver bugfixes and tweaks Eric Wong
  2020-01-02  9:24 ` [PATCH 1/3] solver: try the next patch on apply failures Eric Wong
  2020-01-02  9:24 ` [PATCH 2/3] solver: extract_diff: deal with missing "diff --git" line Eric Wong
@ 2020-01-02  9:24 ` Eric Wong
  2 siblings, 0 replies; 8+ messages in thread
From: Eric Wong @ 2020-01-02  9:24 UTC (permalink / raw)
  To: meta

solver can spawn multiple processes per HTTP request, but
"git apply" failures are needlessly noisy due to corrupt
patches.  We also don't want to silence "git ls-files"
or "git update-index" errors using $env->{'qspawn.quiet'},
either, so this granularity is needed.

Admins can check for 500 errors in access logs to detect
(and reproduce) solver failures, anyways, so there's no
need to log every time "git apply" rejects a corrupt patch.
---
 lib/PublicInbox/Qspawn.pm    | 6 ++----
 lib/PublicInbox/SolverGit.pm | 2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/lib/PublicInbox/Qspawn.pm b/lib/PublicInbox/Qspawn.pm
index 1a2b70e7..65bb178a 100644
--- a/lib/PublicInbox/Qspawn.pm
+++ b/lib/PublicInbox/Qspawn.pm
@@ -56,9 +56,7 @@ sub _do_spawn {
 
 	($self->{rpipe}, $self->{pid}) = popen_rd($cmd, $cmd_env, \%opts);
 
-	# drop any IO handles opt was holding open via $opt->{hold}
-	# No need to hold onto the descriptor once the child process has it.
-	$self->{args} = $cmd; # keep this around for logging
+	$self->{args} = $opts{quiet} ? undef : $cmd;
 
 	if (defined $self->{pid}) {
 		$limiter->{running}++;
@@ -108,7 +106,7 @@ sub waitpid_err ($$) {
 
 	if ($err) {
 		$self->{err} = $err;
-		if ($env && !$env->{'qspawn.quiet'}) {
+		if ($env && $self->{args}) {
 			log_err($env, join(' ', @{$self->{args}}) . ": $err");
 		}
 	}
diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm
index e1b82f3e..ef76d706 100644
--- a/lib/PublicInbox/SolverGit.pm
+++ b/lib/PublicInbox/SolverGit.pm
@@ -497,7 +497,7 @@ sub do_git_apply ($) {
 	} while (@$patches && $len < $ARG_SIZE_MAX &&
 		 !oids_same_ish($patches->[0]->{oid_b}, $prv_oid_b));
 
-	my $opt = { 2 => 1, -C => $dn };
+	my $opt = { 2 => 1, -C => $dn, quiet => 1 };
 	my $qsp = PublicInbox::Qspawn->new(\@cmd, $self->{git_env}, $opt);
 	$self->{-cur_di} = $di;
 	$self->{-qsp} = $qsp;

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] solver: extract_diff: deal with missing "diff --git" line
  2020-01-02  9:24 ` [PATCH 2/3] solver: extract_diff: deal with missing "diff --git" line Eric Wong
@ 2020-01-02  9:36   ` Eric Wong
  2020-01-02 19:04     ` Eric Wong
  2020-01-02 21:12   ` Eric Wong
  2020-01-03  0:43   ` Eric Wong
  2 siblings, 1 reply; 8+ messages in thread
From: Eric Wong @ 2020-01-02  9:36 UTC (permalink / raw)
  To: meta

Eric Wong <e@80x24.org> wrote:
> -my %bad_component = map { $_ => 1 } ('', '.', '..');

<snip>

> +	# get rid of path-traversal attempts and junk patches:
> +	# it's junk at best, an attack attempt at worse:
> +	state %bad_component = map { $_ => 1 } ('', '.', '..');

Nope, that fails on Perl 5.24.1.

"Initialization of state variables in list context currently forbidden"

:<  Otherwise, "state" is a nice 5.10.x feature.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] solver: extract_diff: deal with missing "diff --git" line
  2020-01-02  9:36   ` Eric Wong
@ 2020-01-02 19:04     ` Eric Wong
  0 siblings, 0 replies; 8+ messages in thread
From: Eric Wong @ 2020-01-02 19:04 UTC (permalink / raw)
  To: meta

Eric Wong <e@80x24.org> wrote:
> Eric Wong <e@80x24.org> wrote:
> > -my %bad_component = map { $_ => 1 } ('', '.', '..');
> 
> <snip>
> 
> > +	# get rid of path-traversal attempts and junk patches:
> > +	# it's junk at best, an attack attempt at worse:
> > +	state %bad_component = map { $_ => 1 } ('', '.', '..');
> 
> Nope, that fails on Perl 5.24.1.
> 
> "Initialization of state variables in list context currently forbidden"
> 
> :<  Otherwise, "state" is a nice 5.10.x feature.

The following is a simple fix which preserves compatibility
with older Perls while keeping the variable contained,
will squash:

diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm
index ef76d706..ade94bd3 100644
--- a/lib/PublicInbox/SolverGit.pm
+++ b/lib/PublicInbox/SolverGit.pm
@@ -170,8 +170,8 @@ sub extract_diff ($$) {
 
 	# get rid of path-traversal attempts and junk patches:
 	# it's junk at best, an attack attempt at worse:
-	state %bad_component = map { $_ => 1 } ('', '.', '..');
-	foreach (@a, @b) { return if $bad_component{$_} }
+	state $bad_component = { map { $_ => 1 } ('', '.', '..') };
+	foreach (@a, @b) { return if $bad_component->{$_} }
 
 	$di->{path_a} = join('/', @a);
 	$di->{path_b} = join('/', @b);

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] solver: extract_diff: deal with missing "diff --git" line
  2020-01-02  9:24 ` [PATCH 2/3] solver: extract_diff: deal with missing "diff --git" line Eric Wong
  2020-01-02  9:36   ` Eric Wong
@ 2020-01-02 21:12   ` Eric Wong
  2020-01-03  0:43   ` Eric Wong
  2 siblings, 0 replies; 8+ messages in thread
From: Eric Wong @ 2020-01-02 21:12 UTC (permalink / raw)
  To: meta

Eric Wong <e@80x24.org> wrote:
>  sub extract_diff ($$) {
> +		# old mode $4
> +		(?:^old mode\x20(100644|120000|100755)$LF)?

<snip>

> +	$di->{oid_a} = $6;
> +	$di->{oid_b} = $7;
> +	$di->{mode_a} = $5 // $8 // $6; # new (file) // unchanged // old

$6 is totally wrong, since it's already {oid_a}

diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm
index ef76d706..7c367895 100644
--- a/lib/PublicInbox/SolverGit.pm
+++ b/lib/PublicInbox/SolverGit.pm
@@ -161,7 +161,7 @@ sub extract_diff ($$) {
 	my $path_b = $3 // $11;
 	$di->{oid_a} = $6;
 	$di->{oid_b} = $7;
-	$di->{mode_a} = $5 // $8 // $6; # new (file) // unchanged // old
+	$di->{mode_a} = $5 // $8 // $4; # new (file) // unchanged // old
 	my $patch = $9;
 
 	# don't care for leading 'a/' and 'b/'

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] solver: extract_diff: deal with missing "diff --git" line
  2020-01-02  9:24 ` [PATCH 2/3] solver: extract_diff: deal with missing "diff --git" line Eric Wong
  2020-01-02  9:36   ` Eric Wong
  2020-01-02 21:12   ` Eric Wong
@ 2020-01-03  0:43   ` Eric Wong
  2 siblings, 0 replies; 8+ messages in thread
From: Eric Wong @ 2020-01-03  0:43 UTC (permalink / raw)
  To: meta

Eric Wong <e@80x24.org> wrote:
> Rewrite the patch extraction loop using a single regexp which
> accounts for missing "diff --git ..." lines and is capable of
> extracting pathnames off the "+++ b/foo" line.
> 
> This fixes the solving of blob "96f1c7f" off
> <2841d2de-32ad-eae8-6039-9251a40bb00e@tngtech.com>
> in git@vger archives.
> ---
>  lib/PublicInbox/SolverGit.pm | 165 ++++++++++++++++++-----------------
>  1 file changed, 85 insertions(+), 80 deletions(-)
> 
> diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm
> index 3e3a5899..e1b82f3e 100644
> --- a/lib/PublicInbox/SolverGit.pm
> +++ b/lib/PublicInbox/SolverGit.pm
> @@ -10,6 +10,7 @@
>  package PublicInbox::SolverGit;
>  use strict;
>  use warnings;
> +use 5.010_001;
>  use File::Temp 0.19 ();
>  use Fcntl qw(SEEK_SET);
>  use PublicInbox::Git qw(git_unquote git_quote);
> @@ -38,7 +39,7 @@ my $MAX_PATCH = 9999;
>  #	oid_a => abbreviated pre-image oid,
>  #	oid_b => abbreviated post-image oid,
>  #	tmp => anonymous file handle with the diff,
> -#	hdr_lines => arrayref of various header lines for mode information
> +#	hdr_lines => string of various header lines for mode information
>  #	mode_a => original mode of oid_a (string, not integer),
>  #	ibx => PublicInbox::Inbox object containing the diff
>  #	smsg => PublicInbox::SearchMsg object containing diff
> @@ -47,10 +48,6 @@ my $MAX_PATCH = 9999;
>  #	n => numeric path of the patch (relative to worktree)
>  # }
>  
> -# don't bother if somebody sends us a patch with these path components,
> -# it's junk at best, an attack attempt at worse:
> -my %bad_component = map { $_ => 1 } ('', '.', '..');
> -
>  sub dbg ($$) {
>  	print { $_[0]->{out} } $_[1], "\n" or ERR($_[0], "print(dbg): $!");
>  }
> @@ -100,10 +97,8 @@ sub solve_existing ($$) {
>  
>  sub extract_diff ($$) {
>  	my ($p, $arg) = @_;
> -	my ($self, $diffs, $re, $ibx, $smsg) = @$arg;
> +	my ($self, $diffs, $pre, $post, $ibx, $smsg) = @$arg;
>  	my ($part) = @$p; # ignore $depth and @idx;
> -	my $hdr_lines; # diff --git a/... b/...
> -	my $tmp;
>  	my $ct = $part->content_type || 'text/plain';
>  	my ($s, undef) = msg_part_text($part, $ct);
>  	defined $s or return;
> @@ -116,66 +111,85 @@ sub extract_diff ($$) {
>  		$s =~ s/\r\n/\n/sg;
>  	}
>  
> -	foreach my $l (split(/^/m, $s)) {
> -		if ($l =~ $re) {
> -			$di->{oid_a} = $1;
> -			$di->{oid_b} = $2;
> -			if (defined($3)) {
> -				my $mode_a = $3;
> -				if ($mode_a =~ /\A(?:100644|120000|100755)\z/) {
> -					$di->{mode_a} = $mode_a;
> -				}
> -			}
> -
> -
> -			# start writing the diff out to a tempfile
> -			my $path = ++$self->{tot};
> -			$di->{n} = $path;
> -			open($tmp, '>', $self->{tmp}->dirname . "/$path") or
> -							die "open(tmp): $!";
> -
> -			push @$hdr_lines, $l;
> -			$di->{hdr_lines} = $hdr_lines;
> -			utf8::encode($_) for @$hdr_lines;
> -			print $tmp @$hdr_lines or die "print(tmp): $!";
> -
> -			# for debugging/diagnostics:
> -			$di->{ibx} = $ibx;
> -			$di->{smsg} = $smsg;
> -		} elsif ($l =~ m!\Adiff --git ("?[^/]+/.+) ("?[^/]+/.+)$!) {
> -			last if $tmp; # got our blob, done!
> -
> -			my ($path_a, $path_b) = ($1, $2);
> -
> -			# diff header lines won't have \r because git
> -			# will quote them, but Email::MIME gives CRLF
> -			# for quoted-printable:
> -			$path_b =~ tr/\r//d;
> -
> -			# don't care for leading 'a/' and 'b/'
> -			my (undef, @a) = split(m{/}, git_unquote($path_a));
> -			my (undef, @b) = split(m{/}, git_unquote($path_b));
> -
> -			# get rid of path-traversal attempts and junk patches:
> -			foreach (@a, @b) {
> -				return if $bad_component{$_};
> -			}
> -
> -			$di->{path_a} = join('/', @a);
> -			$di->{path_b} = join('/', @b);
> -			$hdr_lines = [ $l ];
> -		} elsif ($tmp) {
> -			utf8::encode($l);
> -			print $tmp $l or die "print(tmp): $!";
> -		} elsif ($hdr_lines) {
> -			push @$hdr_lines, $l;
> -			if ($l =~ /\Anew file mode (100644|120000|100755)$/) {
> -				$di->{mode_a} = $1;
> -			}
> -		}
> -	}
> -	return undef unless $tmp;
> +	state $LF = qr!\r?\n!;
> +	state $FN = qr!(?:("?[^/\n]+/[^\r\n]+)|/dev/null)!;
> +
> +	$s =~ m!( # $1 start header lines we save for debugging:
> +
> +		# everything before ^index is optional, but we don't
> +		# want to match ^(old|copy|rename|deleted|...) unless
> +		# we match /^diff --git/ first:
> +		(?: # begin optional stuff:
> +
> +		# try to get the pre-and-post filenames as $2 and $3
> +		(?:^diff\x20--git\x20$FN\x20$FN$LF)
> +
> +		# old mode $4
> +		(?:^old mode\x20(100644|120000|100755)$LF)?
> +
> +		# ignore other info
> +		(?:^(?:copy|rename|deleted|dissimilarity|similarity).*$LF)?
> +
> +		# new mode (possibly new file) ($5)
> +		(?:^new\x20(?:file\x20)?mode\x20(100644|120000|100755)$LF)?
> +
> +		# ignore other info
> +		(?:^(?:copy|rename|deleted|dissimilarity|similarity).*$LF)?
> +
> +		)? # end of optional stuff, everything below is required
> +
> +		# match the pre and post-image OIDs as $6 $7
> +		^index\x20(${pre}[a-f0-9]*)\.\.(${post}[a-f0-9]*)
> +			# mode if unchanged $8
> +			(?:\x20(100644|120000|100755))?$LF
> +	) # end of header lines ($1)
> +	( # $9 is the patch body
> +		# "--- a/foo.c" sets pre-filename ($10) in case
> +		# $2 is missing
> +		(?:^---\x20$FN$LF)
> +
> +		# "+++ b/foo.c" sets post-filename ($11) in case
> +		# $3 is missing
> +		(?:^\+{3}\x20$FN$LF)
> +
> +		# the meat of the diff, including "^\\No newline ..."
> +		(?:^[\@\+\x20\-\\][^\r\n]*$LF)+
> +	)!smx or return;

...And we need to support context lines w/o leading space

a) because it happens in the real world because of MUAs

b) it was apparently proposed for GNU diff in the past
   (thankfully, it's not the default)

cf. https://public-inbox.org/git/b507b465f7831612b9d9fc643e3e5218b64e5bfa/s/
(oddly, I can't find the mail for that patch specifically)

This fixes a regression on git/5cd8845/s/?b=submodule.c

diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm
index ab7d5c19..a78360fd 100644
--- a/lib/PublicInbox/SolverGit.pm
+++ b/lib/PublicInbox/SolverGit.pm
@@ -153,7 +153,9 @@ sub extract_diff ($$) {
 		(?:^\+{3}\x20$FN$LF)
 
 		# the meat of the diff, including "^\\No newline ..."
-		(?:^[\@\+\x20\-\\][^\r\n]*$LF)+
+		# We also allow for totally blank lines w/o leading spaces,
+		# because git-apply(1) handles that case, too
+		(?:^(?:[\@\+\x20\-\\][^\r\n]*|)$LF)+
 	)!smx or return;
 
 	my $hdr_lines = $1;

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-02  9:24 [PATCH 0/3] solver bugfixes and tweaks Eric Wong
2020-01-02  9:24 ` [PATCH 1/3] solver: try the next patch on apply failures Eric Wong
2020-01-02  9:24 ` [PATCH 2/3] solver: extract_diff: deal with missing "diff --git" line Eric Wong
2020-01-02  9:36   ` Eric Wong
2020-01-02 19:04     ` Eric Wong
2020-01-02 21:12   ` Eric Wong
2020-01-03  0:43   ` Eric Wong
2020-01-02  9:24 ` [PATCH 3/3] qspawn: use per-call quiet flag for solver Eric Wong

user/dev discussion of public-inbox itself

Archives are clonable:
	git clone --mirror http://public-inbox.org/meta
	git clone --mirror http://czquwvybam4bgbro.onion/meta
	git clone --mirror http://hjrcffqmbrq6wope.onion/meta
	git clone --mirror http://ou63pmih66umazou.onion/meta

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.mail.public-inbox.meta
	nntp://ou63pmih66umazou.onion/inbox.comp.mail.public-inbox.meta
	nntp://czquwvybam4bgbro.onion/inbox.comp.mail.public-inbox.meta
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.mail.public-inbox.meta
	nntp://news.gmane.io/gmane.mail.public-inbox.general

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git