git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH 0/4] Correct offsets of hunks when one is skipped
@ 2018-02-13 10:44 Phillip Wood
  2018-02-13 10:44 ` [PATCH 1/4] add -i: add function to format hunk header Phillip Wood
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: Phillip Wood @ 2018-02-13 10:44 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

While working on a patch series to stage selected lines from a hunk
without having to edit it I got worried that subsequent patches would
be applied in the wrong place which lead to this series to correct the
offsets of hunks following those that are skipped or edited.

Phillip Wood (4):
  add -i: add function to format hunk header
  t3701: add failing test for pathological context lines
  add -p: Adjust offsets of subsequent hunks when one is skipped
  add -p: calculate offset delta for edited patches

 git-add--interactive.perl  | 93 +++++++++++++++++++++++++++++++++++-----------
 t/t3701-add-interactive.sh | 30 +++++++++++++++
 2 files changed, 102 insertions(+), 21 deletions(-)

-- 
2.16.1


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

* [PATCH 1/4] add -i: add function to format hunk header
  2018-02-13 10:44 [PATCH 0/4] Correct offsets of hunks when one is skipped Phillip Wood
@ 2018-02-13 10:44 ` Phillip Wood
  2018-02-13 10:44 ` [PATCH 2/4] t3701: add failing test for pathological context lines Phillip Wood
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Phillip Wood @ 2018-02-13 10:44 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

This code is duplicated in a couple of places so make it into a
function as we're going to add some more callers shortly.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-add--interactive.perl | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 964c3a75420db4751cf11125b68b6904112632f1..8ababa6453ac4f57a925aacbb8b9bb1c973e4a54 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -751,6 +751,15 @@ sub parse_hunk_header {
 	return ($o_ofs, $o_cnt, $n_ofs, $n_cnt);
 }
 
+sub format_hunk_header {
+	my ($o_ofs, $o_cnt, $n_ofs, $n_cnt) = @_;
+	return ("@@ -$o_ofs" .
+		(($o_cnt != 1) ? ",$o_cnt" : '') .
+		" +$n_ofs" .
+		(($n_cnt != 1) ? ",$n_cnt" : '') .
+		" @@\n");
+}
+
 sub split_hunk {
 	my ($text, $display) = @_;
 	my @split = ();
@@ -838,11 +847,7 @@ sub split_hunk {
 		my $o_cnt = $hunk->{OCNT};
 		my $n_cnt = $hunk->{NCNT};
 
-		my $head = ("@@ -$o_ofs" .
-			    (($o_cnt != 1) ? ",$o_cnt" : '') .
-			    " +$n_ofs" .
-			    (($n_cnt != 1) ? ",$n_cnt" : '') .
-			    " @@\n");
+		my $head = format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt);
 		my $display_head = $head;
 		unshift @{$hunk->{TEXT}}, $head;
 		if ($diff_use_color) {
@@ -912,11 +917,7 @@ sub merge_hunk {
 		}
 		push @line, $line;
 	}
-	my $head = ("@@ -$o0_ofs" .
-		    (($o_cnt != 1) ? ",$o_cnt" : '') .
-		    " +$n0_ofs" .
-		    (($n_cnt != 1) ? ",$n_cnt" : '') .
-		    " @@\n");
+	my $head = format_hunk_header($o0_ofs, $o_cnt, $n0_ofs, $n_cnt);
 	@{$prev->{TEXT}} = ($head, @line);
 }
 
-- 
2.16.1


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

* [PATCH 2/4] t3701: add failing test for pathological context lines
  2018-02-13 10:44 [PATCH 0/4] Correct offsets of hunks when one is skipped Phillip Wood
  2018-02-13 10:44 ` [PATCH 1/4] add -i: add function to format hunk header Phillip Wood
@ 2018-02-13 10:44 ` Phillip Wood
  2018-02-13 20:35   ` Junio C Hamano
  2018-02-13 10:44 ` [PATCH 3/4] add -p: Adjust offsets of subsequent hunks when one is skipped Phillip Wood
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 7+ messages in thread
From: Phillip Wood @ 2018-02-13 10:44 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When a hunk is skipped by add -i the offsets of subsequent hunks are
not adjusted to account for any missing insertions due to the skipped
hunk. Most of the time this does not matter as apply uses the context
lines to apply the subsequent hunks in the correct place, however in
pathological cases the context lines will match at the now incorrect
offset and the hunk will be applied in the wrong place. The offsets of
hunks following an edited hunk that has had the number of insertions
or deletions changed also need to be updated in the same way. Add
failing tests to demonstrate this.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3701-add-interactive.sh | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 058698df6a4a9811b9db84fb5900472c47c61798..6838698a1382b24724cfbd3be04a7054489b94af 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -541,4 +541,34 @@ test_expect_success 'status ignores dirty submodules (except HEAD)' '
 	! grep dirty-otherwise output
 '
 
+test_expect_success 'set up pathological context' '
+	git reset --hard &&
+	test_write_lines a a a a a a a a a a a >a &&
+	git add a &&
+	git commit -m a &&
+	test_write_lines c b a a a a a a a b a a a a >a &&
+	test_write_lines     a a a a a a a b a a a a >expected-1 &&
+	test_write_lines   b a a a a a a a b a a a a >expected-2 &&
+	write_script editor <<-\EOF
+	sed /^+c/d "$1" >"$1.tmp"
+	mv "$1.tmp" "$1"
+	EOF
+'
+
+test_expect_failure 'add -p works with pathological context lines' '
+	git reset &&
+	printf "%s\n" n y |
+	git add -p &&
+	git cat-file blob :a > actual &&
+	test_cmp expected-1 actual
+'
+
+test_expect_failure 'add -p patch editing works with pathological context lines' '
+	git reset &&
+	printf "%s\n" e y |
+	GIT_EDITOR=./editor git add -p &&
+	git cat-file blob :a > actual &&
+	test_cmp expected-2 actual
+'
+
 test_done
-- 
2.16.1


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

* [PATCH 3/4] add -p: Adjust offsets of subsequent hunks when one is skipped
  2018-02-13 10:44 [PATCH 0/4] Correct offsets of hunks when one is skipped Phillip Wood
  2018-02-13 10:44 ` [PATCH 1/4] add -i: add function to format hunk header Phillip Wood
  2018-02-13 10:44 ` [PATCH 2/4] t3701: add failing test for pathological context lines Phillip Wood
@ 2018-02-13 10:44 ` Phillip Wood
  2018-02-13 10:44 ` [PATCH 4/4] add -p: calculate offset delta for edited patches Phillip Wood
  2018-02-13 23:56 ` [PATCH 0/4] Correct offsets of hunks when one is skipped brian m. carlson
  4 siblings, 0 replies; 7+ messages in thread
From: Phillip Wood @ 2018-02-13 10:44 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Since commit 8cbd431082 ("git-add--interactive: replace hunk
recounting with apply --recount", 2008-7-2) if a hunk is skipped then
we rely on the context lines to apply subsequent hunks in the right
place. While this works most of the time it is possible for hunks to
end up being applied in the wrong place. To fix this adjust the offset
of subsequent hunks to correct for any change in the number of
insertions or deletions due to the skipped hunk. The change in offset
due to edited hunks that have the number of insertions or deletions
changed is ignored here, it will be fixed in the next commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-add--interactive.perl  | 15 +++++++++++++--
 t/t3701-add-interactive.sh |  2 +-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 8ababa6453ac4f57a925aacbb8b9bb1c973e4a54..7a0a5896bb8fa063ace29b85840849c867b874f5 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -926,14 +926,25 @@ sub coalesce_overlapping_hunks {
 	my @out = ();
 
 	my ($last_o_ctx, $last_was_dirty);
+	my $ofs_delta = 0;
 
-	for (grep { $_->{USE} } @in) {
+	for (@in) {
 		if ($_->{TYPE} ne 'hunk') {
 			push @out, $_;
 			next;
 		}
 		my $text = $_->{TEXT};
-		my ($o_ofs) = parse_hunk_header($text->[0]);
+		my ($o_ofs, $o_cnt, $n_ofs, $n_cnt) =
+						parse_hunk_header($text->[0]);
+		unless ($_->{USE}) {
+			$ofs_delta += $o_cnt - $n_cnt;
+			next;
+		}
+		if ($ofs_delta) {
+			$n_ofs += $ofs_delta;
+			$_->{TEXT}->[0] = format_hunk_header($o_ofs, $o_cnt,
+							     $n_ofs, $n_cnt);
+		}
 		if (defined $last_o_ctx &&
 		    $o_ofs <= $last_o_ctx &&
 		    !$_->{DIRTY} &&
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 6838698a1382b24724cfbd3be04a7054489b94af..5401d74d5804b5e43286d08a905fb96c68b02e42 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -555,7 +555,7 @@ test_expect_success 'set up pathological context' '
 	EOF
 '
 
-test_expect_failure 'add -p works with pathological context lines' '
+test_expect_success 'add -p works with pathological context lines' '
 	git reset &&
 	printf "%s\n" n y |
 	git add -p &&
-- 
2.16.1


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

* [PATCH 4/4] add -p: calculate offset delta for edited patches
  2018-02-13 10:44 [PATCH 0/4] Correct offsets of hunks when one is skipped Phillip Wood
                   ` (2 preceding siblings ...)
  2018-02-13 10:44 ` [PATCH 3/4] add -p: Adjust offsets of subsequent hunks when one is skipped Phillip Wood
@ 2018-02-13 10:44 ` Phillip Wood
  2018-02-13 23:56 ` [PATCH 0/4] Correct offsets of hunks when one is skipped brian m. carlson
  4 siblings, 0 replies; 7+ messages in thread
From: Phillip Wood @ 2018-02-13 10:44 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Recount the number of preimage and postimage lines in a hunk after it
has been edited so any change in the number of insertions or deletions
can be used to adjust the offsets of subsequent hunks. If an edited
hunk is subsequently split then the offset correction will be lost. It
would be possible to fix this if it is a problem, however the code
here is still an improvement on the status quo for the common case
where an edited hunk is applied without being split.

This is also a necessary step to removing '--recount' and
'--allow-overlap' from the invocation of 'git apply'. Before
'--recount' can be removed the splitting and coalescing counting needs
to be fixed to handle a missing newline at the end of a file. In order
to remove '--allow-overlap' there needs to be i) some way of verifying
the offset data in the edited hunk (probably by correlating the
preimage (or postimage if the patch is going to be applied in reverse)
lines of the edited and unedited versions to see if they are offset or
if any leading/trailing context lines have been removed) and ii) a way of
dealing with edited hunks that change context lines that are shared
with neighbouring hunks.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-add--interactive.perl  | 57 ++++++++++++++++++++++++++++++++++++++--------
 t/t3701-add-interactive.sh |  2 +-
 2 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 7a0a5896bb8fa063ace29b85840849c867b874f5..f139e545a4fb9cbaa757208a44cb2b5c3ad3678a 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -938,6 +938,9 @@ sub coalesce_overlapping_hunks {
 						parse_hunk_header($text->[0]);
 		unless ($_->{USE}) {
 			$ofs_delta += $o_cnt - $n_cnt;
+			# If this hunk has been edited then subtract
+			# the delta that is due to the edit.
+			$_->{OFS_DELTA} and $ofs_delta -= $_->{OFS_DELTA};
 			next;
 		}
 		if ($ofs_delta) {
@@ -945,6 +948,9 @@ sub coalesce_overlapping_hunks {
 			$_->{TEXT}->[0] = format_hunk_header($o_ofs, $o_cnt,
 							     $n_ofs, $n_cnt);
 		}
+		# If this hunk was edited then adjust the offset delta
+		# to reflect the edit.
+		$_->{OFS_DELTA} and $ofs_delta += $_->{OFS_DELTA};
 		if (defined $last_o_ctx &&
 		    $o_ofs <= $last_o_ctx &&
 		    !$_->{DIRTY} &&
@@ -1016,6 +1022,30 @@ marked for discarding."),
 marked for applying."),
 );
 
+sub recount_edited_hunk {
+	local $_;
+	my ($oldtext, $newtext) = @_;
+	my ($o_cnt, $n_cnt) = (0, 0);
+	for (@{$newtext}[1..$#{$newtext}]) {
+		my $mode = substr($_, 0, 1);
+		if ($mode eq '-') {
+			$o_cnt++;
+		} elsif ($mode eq '+') {
+			$n_cnt++;
+		} elsif ($mode eq ' ') {
+			$o_cnt++;
+			$n_cnt++;
+		}
+	}
+	my ($o_ofs, undef, $n_ofs, undef) =
+					parse_hunk_header($newtext->[0]);
+	$newtext->[0] = format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt);
+	my (undef, $orig_o_cnt, undef, $orig_n_cnt) =
+					parse_hunk_header($oldtext->[0]);
+	# Return the change in the number of lines inserted by this hunk
+	return $orig_o_cnt - $orig_n_cnt - $o_cnt + $n_cnt;
+}
+
 sub edit_hunk_manually {
 	my ($oldtext) = @_;
 
@@ -1114,25 +1144,34 @@ sub prompt_yesno {
 }
 
 sub edit_hunk_loop {
-	my ($head, $hunk, $ix) = @_;
-	my $text = $hunk->[$ix]->{TEXT};
+	my ($head, $hunks, $ix) = @_;
+	my $hunk = $hunks->[$ix];
+	my $text = $hunk->{TEXT};
 
 	while (1) {
-		$text = edit_hunk_manually($text);
-		if (!defined $text) {
+		my $newtext = edit_hunk_manually($text);
+		if (!defined $newtext) {
 			return undef;
 		}
 		my $newhunk = {
-			TEXT => $text,
-			TYPE => $hunk->[$ix]->{TYPE},
+			TEXT => $newtext,
+			TYPE => $hunk->{TYPE},
 			USE => 1,
 			DIRTY => 1,
 		};
 		if (diff_applies($head,
-				 @{$hunk}[0..$ix-1],
+				 @{$hunks}[0..$ix-1],
 				 $newhunk,
-				 @{$hunk}[$ix+1..$#{$hunk}])) {
-			$newhunk->{DISPLAY} = [color_diff(@{$text})];
+				 @{$hunks}[$ix+1..$#{$hunks}])) {
+			$newhunk->{OFS_DELTA} =
+					recount_edited_hunk($text, $newtext);
+			# If this hunk has already been edited then
+			# add the offset delta of the previous edit to
+			# get the real delta from the original
+			# unedited hunk.
+			$hunk->{OFS_DELTA} and
+				$newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA};
+			$newhunk->{DISPLAY} = [color_diff(@{$newtext})];
 			return $newhunk;
 		}
 		else {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 5401d74d5804b5e43286d08a905fb96c68b02e42..5681bb2bb7e9a9c0702f8ebbcd97efbb5c5d1d58 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -563,7 +563,7 @@ test_expect_success 'add -p works with pathological context lines' '
 	test_cmp expected-1 actual
 '
 
-test_expect_failure 'add -p patch editing works with pathological context lines' '
+test_expect_success 'add -p patch editing works with pathological context lines' '
 	git reset &&
 	printf "%s\n" e y |
 	GIT_EDITOR=./editor git add -p &&
-- 
2.16.1


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

* Re: [PATCH 2/4] t3701: add failing test for pathological context lines
  2018-02-13 10:44 ` [PATCH 2/4] t3701: add failing test for pathological context lines Phillip Wood
@ 2018-02-13 20:35   ` Junio C Hamano
  0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2018-02-13 20:35 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Git Mailing List, Phillip Wood

Phillip Wood <phillip.wood@talktalk.net> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When a hunk is skipped by add -i the offsets of subsequent hunks are
> not adjusted to account for any missing insertions due to the skipped
> hunk. Most of the time this does not matter as apply uses the context
> lines to apply the subsequent hunks in the correct place, however in
> pathological cases the context lines will match at the now incorrect
> offset and the hunk will be applied in the wrong place. The offsets of

Good.  The --recount "feature" on the receiving end does not have
enough information to do a job as good as the code sitting at the
side of producing a patch to be applied, and this goes in the right
direction.


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

* Re: [PATCH 0/4] Correct offsets of hunks when one is skipped
  2018-02-13 10:44 [PATCH 0/4] Correct offsets of hunks when one is skipped Phillip Wood
                   ` (3 preceding siblings ...)
  2018-02-13 10:44 ` [PATCH 4/4] add -p: calculate offset delta for edited patches Phillip Wood
@ 2018-02-13 23:56 ` brian m. carlson
  4 siblings, 0 replies; 7+ messages in thread
From: brian m. carlson @ 2018-02-13 23:56 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Git Mailing List

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]

On Tue, Feb 13, 2018 at 10:44:04AM +0000, Phillip Wood wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> While working on a patch series to stage selected lines from a hunk
> without having to edit it I got worried that subsequent patches would
> be applied in the wrong place which lead to this series to correct the
> offsets of hunks following those that are skipped or edited.
> 
> Phillip Wood (4):
>   add -i: add function to format hunk header
>   t3701: add failing test for pathological context lines
>   add -p: Adjust offsets of subsequent hunks when one is skipped
>   add -p: calculate offset delta for edited patches
> 
>  git-add--interactive.perl  | 93 +++++++++++++++++++++++++++++++++++-----------
>  t/t3701-add-interactive.sh | 30 +++++++++++++++
>  2 files changed, 102 insertions(+), 21 deletions(-)

This looks reasonably sane to me.  I really like that you managed to
produce failing tests for this situation.  I know pathological cases
like this have bit GCC in the past, so it's good that you fixed this.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

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

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-13 10:44 [PATCH 0/4] Correct offsets of hunks when one is skipped Phillip Wood
2018-02-13 10:44 ` [PATCH 1/4] add -i: add function to format hunk header Phillip Wood
2018-02-13 10:44 ` [PATCH 2/4] t3701: add failing test for pathological context lines Phillip Wood
2018-02-13 20:35   ` Junio C Hamano
2018-02-13 10:44 ` [PATCH 3/4] add -p: Adjust offsets of subsequent hunks when one is skipped Phillip Wood
2018-02-13 10:44 ` [PATCH 4/4] add -p: calculate offset delta for edited patches Phillip Wood
2018-02-13 23:56 ` [PATCH 0/4] Correct offsets of hunks when one is skipped brian m. carlson

git@vger.kernel.org mailing list mirror (one of many)

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

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

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

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