* [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 ` (8 more replies) 0 siblings, 9 replies; 79+ 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] 79+ 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 ` (7 subsequent siblings) 8 siblings, 0 replies; 79+ 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 related [flat|nested] 79+ 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 ` (6 subsequent siblings) 8 siblings, 1 reply; 79+ 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 related [flat|nested] 79+ 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; 79+ 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] 79+ 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 ` (5 subsequent siblings) 8 siblings, 0 replies; 79+ 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 related [flat|nested] 79+ 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 subsequent siblings) 8 siblings, 0 replies; 79+ 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 related [flat|nested] 79+ 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 2018-02-19 13:01 ` Phillip Wood 2018-02-19 11:29 ` [PATCH v2 0/9] " Phillip Wood ` (3 subsequent siblings) 8 siblings, 1 reply; 79+ 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] 79+ messages in thread
* Re: [PATCH 0/4] Correct offsets of hunks when one is skipped 2018-02-13 23:56 ` [PATCH 0/4] Correct offsets of hunks when one is skipped brian m. carlson @ 2018-02-19 13:01 ` Phillip Wood 0 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-02-19 13:01 UTC (permalink / raw) To: brian m. carlson, Phillip Wood, Git Mailing List On 13/02/18 23:56, brian m. carlson wrote: > 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. > Thanks Brain, it's interesting to hear that GCC has been bitten in the past Best Wishes Phillip ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v2 0/9] 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 ` (4 preceding siblings ...) 2018-02-13 23:56 ` [PATCH 0/4] Correct offsets of hunks when one is skipped brian m. carlson @ 2018-02-19 11:29 ` Phillip Wood 2018-02-19 11:29 ` [PATCH v2 1/9] add -i: add function to format hunk header Phillip Wood ` (8 more replies) 2018-02-27 11:03 ` [PATCH v3 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (2 subsequent siblings) 8 siblings, 9 replies; 79+ messages in thread From: Phillip Wood @ 2018-02-19 11:29 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Brian M. Carlson, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Since v1 I've added some test cleanups for t3701, fixed the counting when splitting and coalescing hunks containing "\ No newline at end of file" lines and added a patch to remove '--recount' from the invocation of 'git apply'. There are minor changes to patches 5 (previously patch 2) and patch 7 (previously patch 4) which I've explained in the comments on those patches. Otherwise the original patches are unchanged. Cover letter to v1: 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 (9): add -i: add function to format hunk header t3701: indent here documents t3701: use test_write_lines and write_script t3701: don't hard code sha1 hash values 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 add -p: fix counting when splitting and coalescing add -p: don't rely on apply's '--recount' option git-add--interactive.perl | 106 ++++++++++++----- t/t3701-add-interactive.sh | 281 ++++++++++++++++++++++++--------------------- 2 files changed, 229 insertions(+), 158 deletions(-) -- 2.16.1 ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v2 1/9] add -i: add function to format hunk header 2018-02-19 11:29 ` [PATCH v2 0/9] " Phillip Wood @ 2018-02-19 11:29 ` Phillip Wood 2018-02-20 18:21 ` Junio C Hamano 2018-02-19 11:29 ` [PATCH v2 2/9] t3701: indent here documents Phillip Wood ` (7 subsequent siblings) 8 siblings, 1 reply; 79+ messages in thread From: Phillip Wood @ 2018-02-19 11:29 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Brian M. Carlson, 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 related [flat|nested] 79+ messages in thread
* Re: [PATCH v2 1/9] add -i: add function to format hunk header 2018-02-19 11:29 ` [PATCH v2 1/9] add -i: add function to format hunk header Phillip Wood @ 2018-02-20 18:21 ` Junio C Hamano 0 siblings, 0 replies; 79+ messages in thread From: Junio C Hamano @ 2018-02-20 18:21 UTC (permalink / raw) To: Phillip Wood; +Cc: Git Mailing List, Brian M. Carlson, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > 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 This is quite a tangent, but please refrain from using full object name on the index line. Because it does not add much value in the context of patch exchange for project of this size to use more than 7-12 hexdigits each, the only effect of doing so is to push the mode bit far to the right. ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v2 2/9] t3701: indent here documents 2018-02-19 11:29 ` [PATCH v2 0/9] " Phillip Wood 2018-02-19 11:29 ` [PATCH v2 1/9] add -i: add function to format hunk header Phillip Wood @ 2018-02-19 11:29 ` Phillip Wood 2018-02-19 18:36 ` Eric Sunshine 2018-02-19 11:29 ` [PATCH v2 3/9] t3701: use test_write_lines and write_script Phillip Wood ` (6 subsequent siblings) 8 siblings, 1 reply; 79+ messages in thread From: Phillip Wood @ 2018-02-19 11:29 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Brian M. Carlson, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Indent here documents in line with the current style for tests. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- t/t3701-add-interactive.sh | 174 ++++++++++++++++++++++----------------------- 1 file changed, 87 insertions(+), 87 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 058698df6a4a9811b9db84fb5900472c47c61798..861ea2e08cce750515f59fc424b3f8336fd9b1a9 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -22,14 +22,14 @@ test_expect_success 'status works (initial)' ' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -new file mode 100644 -index 0000000..d95f3ad ---- /dev/null -+++ b/file -@@ -0,0 +1 @@ -+content -EOF + cat >expected <<-EOF + new file mode 100644 + index 0000000..d95f3ad + --- /dev/null + +++ b/file + @@ -0,0 +1 @@ + +content + EOF ' test_expect_success 'diff works (initial)' ' @@ -59,14 +59,14 @@ test_expect_success 'status works (commit)' ' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -index 180b47c..b6f2c08 100644 ---- a/file -+++ b/file -@@ -1 +1,2 @@ - baseline -+content -EOF + cat >expected <<-EOF + index 180b47c..b6f2c08 100644 + --- a/file + +++ b/file + @@ -1 +1,2 @@ + baseline + +content + EOF ' test_expect_success 'diff works (commit)' ' @@ -83,8 +83,8 @@ test_expect_success 'revert works (commit)' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -EOF + cat >expected <<-EOF + EOF ' test_expect_success 'setup fake editor' ' @@ -100,21 +100,21 @@ test_expect_success 'dummy edit works' ' ' test_expect_success 'setup patch' ' -cat >patch <<EOF -@@ -1,1 +1,4 @@ - this -+patch --does not - apply -EOF + cat >patch <<-EOF + @@ -1,1 +1,4 @@ + this + +patch + -does not + apply + EOF ' test_expect_success 'setup fake editor' ' echo "#!$SHELL_PATH" >fake_editor.sh && - cat >>fake_editor.sh <<\EOF && -mv -f "$1" oldpatch && -mv -f patch "$1" -EOF + cat >>fake_editor.sh <<-\EOF && + mv -f "$1" oldpatch && + mv -f patch "$1" + EOF chmod a+x fake_editor.sh && test_set_editor "$(pwd)/fake_editor.sh" ' @@ -126,10 +126,10 @@ test_expect_success 'bad edit rejected' ' ' test_expect_success 'setup patch' ' -cat >patch <<EOF -this patch -is garbage -EOF + cat >patch <<-EOF + this patch + is garbage + EOF ' test_expect_success 'garbage edit rejected' ' @@ -139,28 +139,28 @@ test_expect_success 'garbage edit rejected' ' ' test_expect_success 'setup patch' ' -cat >patch <<EOF -@@ -1,0 +1,0 @@ - baseline -+content -+newcontent -+lines -EOF + cat >patch <<-EOF + @@ -1,0 +1,0 @@ + baseline + +content + +newcontent + +lines + EOF ' test_expect_success 'setup expected' ' -cat >expected <<EOF -diff --git a/file b/file -index b5dd6c9..f910ae9 100644 ---- a/file -+++ b/file -@@ -1,4 +1,4 @@ - baseline - content --newcontent -+more - lines -EOF + cat >expected <<-EOF + diff --git a/file b/file + index b5dd6c9..f910ae9 100644 + --- a/file + +++ b/file + @@ -1,4 +1,4 @@ + baseline + content + -newcontent + +more + lines + EOF ' test_expect_success 'real edit works' ' @@ -222,31 +222,31 @@ test_expect_success 'setup again' ' # Write the patch file with a new line at the top and bottom test_expect_success 'setup patch' ' -cat >patch <<EOF -index 180b47c..b6f2c08 100644 ---- a/file -+++ b/file -@@ -1,2 +1,4 @@ -+firstline - baseline - content -+lastline -EOF + cat >patch <<-EOF + index 180b47c..b6f2c08 100644 + --- a/file + +++ b/file + @@ -1,2 +1,4 @@ + +firstline + baseline + content + +lastline + EOF ' # Expected output, similar to the patch but w/ diff at the top test_expect_success 'setup expected' ' -cat >expected <<EOF -diff --git a/file b/file -index b6f2c08..61b9053 100755 ---- a/file -+++ b/file -@@ -1,2 +1,4 @@ -+firstline - baseline - content -+lastline -EOF + cat >expected <<-EOF + diff --git a/file b/file + index b6f2c08..61b9053 100755 + --- a/file + +++ b/file + @@ -1,2 +1,4 @@ + +firstline + baseline + content + +lastline + EOF ' # Test splitting the first patch, then adding both @@ -259,15 +259,15 @@ test_expect_success 'add first line works' ' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -diff --git a/non-empty b/non-empty -deleted file mode 100644 -index d95f3ad..0000000 ---- a/non-empty -+++ /dev/null -@@ -1 +0,0 @@ --content -EOF + cat >expected <<-EOF + diff --git a/non-empty b/non-empty + deleted file mode 100644 + index d95f3ad..0000000 + --- a/non-empty + +++ /dev/null + @@ -1 +0,0 @@ + -content + EOF ' test_expect_success 'deleting a non-empty file' ' @@ -282,11 +282,11 @@ test_expect_success 'deleting a non-empty file' ' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -diff --git a/empty b/empty -deleted file mode 100644 -index e69de29..0000000 -EOF + cat >expected <<-EOF + diff --git a/empty b/empty + deleted file mode 100644 + index e69de29..0000000 + EOF ' test_expect_success 'deleting an empty file' ' -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v2 2/9] t3701: indent here documents 2018-02-19 11:29 ` [PATCH v2 2/9] t3701: indent here documents Phillip Wood @ 2018-02-19 18:36 ` Eric Sunshine 0 siblings, 0 replies; 79+ messages in thread From: Eric Sunshine @ 2018-02-19 18:36 UTC (permalink / raw) To: Phillip Wood; +Cc: Git Mailing List, Junio C Hamano, Brian M. Carlson On Mon, Feb 19, 2018 at 6:29 AM, Phillip Wood <phillip.wood@talktalk.net> wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Indent here documents in line with the current style for tests. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > @@ -22,14 +22,14 @@ test_expect_success 'status works (initial)' ' > test_expect_success 'setup expected' ' > -cat >expected <<EOF > -new file mode 100644 > -index 0000000..d95f3ad > ---- /dev/null > -+++ b/file > -@@ -0,0 +1 @@ > -+content > -EOF > + cat >expected <<-EOF Minor: You could take the opportunity to update these to use -\EOF (rather than -EOF) to document that no variable interpolation is expected inside the 'here' document. Probably itself not worth a re-roll. > + new file mode 100644 > + index 0000000..d95f3ad > + --- /dev/null > + +++ b/file > + @@ -0,0 +1 @@ > + +content > + EOF > ' ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v2 3/9] t3701: use test_write_lines and write_script 2018-02-19 11:29 ` [PATCH v2 0/9] " Phillip Wood 2018-02-19 11:29 ` [PATCH v2 1/9] add -i: add function to format hunk header Phillip Wood 2018-02-19 11:29 ` [PATCH v2 2/9] t3701: indent here documents Phillip Wood @ 2018-02-19 11:29 ` Phillip Wood 2018-02-19 18:47 ` Eric Sunshine 2018-02-19 11:29 ` [PATCH v2 4/9] t3701: don't hard code sha1 hash values Phillip Wood ` (5 subsequent siblings) 8 siblings, 1 reply; 79+ messages in thread From: Phillip Wood @ 2018-02-19 11:29 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Brian M. Carlson, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Simplify things slightly by using the above helpers. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- t/t3701-add-interactive.sh | 36 +++++++----------------------------- 1 file changed, 7 insertions(+), 29 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 861ea2e08cce750515f59fc424b3f8336fd9b1a9..b73a9598ab3eaed074735e99f3dcbc8f88d86f4c 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -87,13 +87,8 @@ test_expect_success 'setup expected' ' EOF ' -test_expect_success 'setup fake editor' ' - >fake_editor.sh && - chmod a+x fake_editor.sh && - test_set_editor "$(pwd)/fake_editor.sh" -' - test_expect_success 'dummy edit works' ' + test_set_editor : && (echo e; echo a) | git add -p && git diff > diff && test_cmp expected diff @@ -110,13 +105,12 @@ test_expect_success 'setup patch' ' ' test_expect_success 'setup fake editor' ' - echo "#!$SHELL_PATH" >fake_editor.sh && - cat >>fake_editor.sh <<-\EOF && + FAKE_EDITOR="$(pwd)/fake-editor.sh" && + write_script "$FAKE_EDITOR" <<-\EOF && mv -f "$1" oldpatch && mv -f patch "$1" EOF - chmod a+x fake_editor.sh && - test_set_editor "$(pwd)/fake_editor.sh" + test_set_editor "$FAKE_EDITOR" ' test_expect_success 'bad edit rejected' ' @@ -302,18 +296,12 @@ test_expect_success 'deleting an empty file' ' test_expect_success 'split hunk setup' ' git reset --hard && - for i in 10 20 30 40 50 60 - do - echo $i - done >test && + test_write_lines 10 20 30 40 50 60 >test && git add test && test_tick && git commit -m test && - for i in 10 15 20 21 22 23 24 30 40 50 60 - do - echo $i - done >test + test_write_lines 10 15 20 21 22 23 24 30 40 50 60 >test ' test_expect_success 'split hunk "add -p (edit)"' ' @@ -334,17 +322,7 @@ test_expect_success 'split hunk "add -p (edit)"' ' ' test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' - cat >test <<-\EOF && - 5 - 10 - 20 - 21 - 30 - 31 - 40 - 50 - 60 - EOF + test_write_lines 5 10 20 21 30 31 40 50 60 >test && git reset && # test sequence is s(plit), n(o), y(es), e(dit) # q n q q is there to make sure we exit at the end. -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v2 3/9] t3701: use test_write_lines and write_script 2018-02-19 11:29 ` [PATCH v2 3/9] t3701: use test_write_lines and write_script Phillip Wood @ 2018-02-19 18:47 ` Eric Sunshine 2018-02-20 17:19 ` Junio C Hamano 0 siblings, 1 reply; 79+ messages in thread From: Eric Sunshine @ 2018-02-19 18:47 UTC (permalink / raw) To: Phillip Wood; +Cc: Git Mailing List, Junio C Hamano, Brian M. Carlson On Mon, Feb 19, 2018 at 6:29 AM, Phillip Wood <phillip.wood@talktalk.net> wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Simplify things slightly by using the above helpers. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > @@ -87,13 +87,8 @@ test_expect_success 'setup expected' ' > @@ -110,13 +105,12 @@ test_expect_success 'setup patch' ' > test_expect_success 'setup fake editor' ' > - echo "#!$SHELL_PATH" >fake_editor.sh && > - cat >>fake_editor.sh <<-\EOF && > + FAKE_EDITOR="$(pwd)/fake-editor.sh" && > + write_script "$FAKE_EDITOR" <<-\EOF && > mv -f "$1" oldpatch && > mv -f patch "$1" > EOF > - chmod a+x fake_editor.sh && > - test_set_editor "$(pwd)/fake_editor.sh" > + test_set_editor "$FAKE_EDITOR" > ' The very first thing that test_set_editor() does is set FAKE_EDITOR to the value of $1, so it is confusing to see it getting setting it here first; the reader has to spend extra brain cycles wondering if something non-obvious is going on that requires this manual assignment. Perhaps drop the assignment altogether and just write literal "fake_editor.sh" in the couple places it's needed (as was done in the original code)? ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v2 3/9] t3701: use test_write_lines and write_script 2018-02-19 18:47 ` Eric Sunshine @ 2018-02-20 17:19 ` Junio C Hamano 2018-02-21 11:26 ` Phillip Wood 0 siblings, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2018-02-20 17:19 UTC (permalink / raw) To: Eric Sunshine; +Cc: Phillip Wood, Git Mailing List, Brian M. Carlson Eric Sunshine <sunshine@sunshineco.com> writes: >> test_expect_success 'setup fake editor' ' >> - echo "#!$SHELL_PATH" >fake_editor.sh && >> - cat >>fake_editor.sh <<-\EOF && >> + FAKE_EDITOR="$(pwd)/fake-editor.sh" && >> + write_script "$FAKE_EDITOR" <<-\EOF && >> mv -f "$1" oldpatch && >> mv -f patch "$1" >> EOF >> - chmod a+x fake_editor.sh && >> - test_set_editor "$(pwd)/fake_editor.sh" >> + test_set_editor "$FAKE_EDITOR" >> ' > > The very first thing that test_set_editor() does is set FAKE_EDITOR to > the value of $1, so it is confusing to see it getting setting it here > first; the reader has to spend extra brain cycles wondering if > something non-obvious is going on that requires this manual > assignment. Perhaps drop the assignment altogether and just write > literal "fake_editor.sh" in the couple places it's needed (as was done > in the original code)? Yeah, I think $(pwd)/ prefix is needed at the final step (i.e. as the first argument to test_set_editor) for this to be a faithful rewrite but it is distracting having to write it anywhere else. Other than that, this looks like a quite straight-forward cleanup. Thanks, both. Here is what I'd be queuing tentatively. t/t3701-add-interactive.sh | 33 +++++---------------------------- 1 file changed, 5 insertions(+), 28 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 39c7423069..4a369fcb51 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -87,13 +87,8 @@ test_expect_success 'setup expected' ' EOF ' -test_expect_success 'setup fake editor' ' - >fake_editor.sh && - chmod a+x fake_editor.sh && - test_set_editor "$(pwd)/fake_editor.sh" -' - test_expect_success 'dummy edit works' ' + test_set_editor : && (echo e; echo a) | git add -p && git diff > diff && test_cmp expected diff @@ -110,12 +105,10 @@ test_expect_success 'setup patch' ' ' test_expect_success 'setup fake editor' ' - echo "#!$SHELL_PATH" >fake_editor.sh && - cat >>fake_editor.sh <<-\EOF && + write_script "fake_editor.sh" <<-\EOF && mv -f "$1" oldpatch && mv -f patch "$1" EOF - chmod a+x fake_editor.sh && test_set_editor "$(pwd)/fake_editor.sh" ' @@ -302,18 +295,12 @@ test_expect_success 'deleting an empty file' ' test_expect_success 'split hunk setup' ' git reset --hard && - for i in 10 20 30 40 50 60 - do - echo $i - done >test && + test_write_lines 10 20 30 40 50 60 >test && git add test && test_tick && git commit -m test && - for i in 10 15 20 21 22 23 24 30 40 50 60 - do - echo $i - done >test + test_write_lines 10 15 20 21 22 23 24 30 40 50 60 >test ' test_expect_success 'split hunk "add -p (edit)"' ' @@ -334,17 +321,7 @@ test_expect_success 'split hunk "add -p (edit)"' ' ' test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' - cat >test <<-\EOF && - 5 - 10 - 20 - 21 - 30 - 31 - 40 - 50 - 60 - EOF + test_write_lines 5 10 20 21 30 31 40 50 60 >test && git reset && # test sequence is s(plit), n(o), y(es), e(dit) # q n q q is there to make sure we exit at the end. ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v2 3/9] t3701: use test_write_lines and write_script 2018-02-20 17:19 ` Junio C Hamano @ 2018-02-21 11:26 ` Phillip Wood 0 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-02-21 11:26 UTC (permalink / raw) To: Junio C Hamano, Eric Sunshine Cc: Phillip Wood, Git Mailing List, Brian M. Carlson On 20/02/18 17:19, Junio C Hamano wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > >>> test_expect_success 'setup fake editor' ' >>> - echo "#!$SHELL_PATH" >fake_editor.sh && >>> - cat >>fake_editor.sh <<-\EOF && >>> + FAKE_EDITOR="$(pwd)/fake-editor.sh" && >>> + write_script "$FAKE_EDITOR" <<-\EOF && >>> mv -f "$1" oldpatch && >>> mv -f patch "$1" >>> EOF >>> - chmod a+x fake_editor.sh && >>> - test_set_editor "$(pwd)/fake_editor.sh" >>> + test_set_editor "$FAKE_EDITOR" >>> ' >> >> The very first thing that test_set_editor() does is set FAKE_EDITOR to >> the value of $1, so it is confusing to see it getting setting it here >> first; the reader has to spend extra brain cycles wondering if >> something non-obvious is going on that requires this manual >> assignment. Perhaps drop the assignment altogether and just write >> literal "fake_editor.sh" in the couple places it's needed (as was done >> in the original code)? > > Yeah, I think $(pwd)/ prefix is needed at the final step (i.e. as > the first argument to test_set_editor) for this to be a faithful > rewrite but it is distracting having to write it anywhere else. > > Other than that, this looks like a quite straight-forward cleanup. > > Thanks, both. Here is what I'd be queuing tentatively. That looks good, thanks for fixing it up Phillip > > t/t3701-add-interactive.sh | 33 +++++---------------------------- > 1 file changed, 5 insertions(+), 28 deletions(-) > > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > index 39c7423069..4a369fcb51 100755 > --- a/t/t3701-add-interactive.sh > +++ b/t/t3701-add-interactive.sh > @@ -87,13 +87,8 @@ test_expect_success 'setup expected' ' > EOF > ' > > -test_expect_success 'setup fake editor' ' > - >fake_editor.sh && > - chmod a+x fake_editor.sh && > - test_set_editor "$(pwd)/fake_editor.sh" > -' > - > test_expect_success 'dummy edit works' ' > + test_set_editor : && > (echo e; echo a) | git add -p && > git diff > diff && > test_cmp expected diff > @@ -110,12 +105,10 @@ test_expect_success 'setup patch' ' > ' > > test_expect_success 'setup fake editor' ' > - echo "#!$SHELL_PATH" >fake_editor.sh && > - cat >>fake_editor.sh <<-\EOF && > + write_script "fake_editor.sh" <<-\EOF && > mv -f "$1" oldpatch && > mv -f patch "$1" > EOF > - chmod a+x fake_editor.sh && > test_set_editor "$(pwd)/fake_editor.sh" > ' > > @@ -302,18 +295,12 @@ test_expect_success 'deleting an empty file' ' > > test_expect_success 'split hunk setup' ' > git reset --hard && > - for i in 10 20 30 40 50 60 > - do > - echo $i > - done >test && > + test_write_lines 10 20 30 40 50 60 >test && > git add test && > test_tick && > git commit -m test && > > - for i in 10 15 20 21 22 23 24 30 40 50 60 > - do > - echo $i > - done >test > + test_write_lines 10 15 20 21 22 23 24 30 40 50 60 >test > ' > > test_expect_success 'split hunk "add -p (edit)"' ' > @@ -334,17 +321,7 @@ test_expect_success 'split hunk "add -p (edit)"' ' > ' > > test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' > - cat >test <<-\EOF && > - 5 > - 10 > - 20 > - 21 > - 30 > - 31 > - 40 > - 50 > - 60 > - EOF > + test_write_lines 5 10 20 21 30 31 40 50 60 >test && > git reset && > # test sequence is s(plit), n(o), y(es), e(dit) > # q n q q is there to make sure we exit at the end. > ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v2 4/9] t3701: don't hard code sha1 hash values 2018-02-19 11:29 ` [PATCH v2 0/9] " Phillip Wood ` (2 preceding siblings ...) 2018-02-19 11:29 ` [PATCH v2 3/9] t3701: use test_write_lines and write_script Phillip Wood @ 2018-02-19 11:29 ` Phillip Wood 2018-02-19 18:52 ` Eric Sunshine 2018-02-20 17:39 ` Junio C Hamano 2018-02-19 11:29 ` [PATCH v2 5/9] t3701: add failing test for pathological context lines Phillip Wood ` (4 subsequent siblings) 8 siblings, 2 replies; 79+ messages in thread From: Phillip Wood @ 2018-02-19 11:29 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Brian M. Carlson, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Purge the index lines from diffs so we're not hard coding sha1 hash values in the expected output. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- t/t3701-add-interactive.sh | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index b73a9598ab3eaed074735e99f3dcbc8f88d86f4c..70748393f28c93f4bc5d43b07bd96bd208aba3e9 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -24,7 +24,6 @@ test_expect_success 'status works (initial)' ' test_expect_success 'setup expected' ' cat >expected <<-EOF new file mode 100644 - index 0000000..d95f3ad --- /dev/null +++ b/file @@ -0,0 +1 @@ @@ -34,7 +33,7 @@ test_expect_success 'setup expected' ' test_expect_success 'diff works (initial)' ' (echo d; echo 1) | git add -i >output && - sed -ne "/new file/,/content/p" <output >diff && + sed -ne /^index/d -e "/new file/,/content/p" <output >diff && test_cmp expected diff ' test_expect_success 'revert works (initial)' ' @@ -60,7 +59,6 @@ test_expect_success 'status works (commit)' ' test_expect_success 'setup expected' ' cat >expected <<-EOF - index 180b47c..b6f2c08 100644 --- a/file +++ b/file @@ -1 +1,2 @@ @@ -71,7 +69,7 @@ test_expect_success 'setup expected' ' test_expect_success 'diff works (commit)' ' (echo d; echo 1) | git add -i >output && - sed -ne "/^index/,/content/p" <output >diff && + sed -ne "/^---/,/content/p" <output >diff && test_cmp expected diff ' test_expect_success 'revert works (commit)' ' @@ -90,7 +88,7 @@ test_expect_success 'setup expected' ' test_expect_success 'dummy edit works' ' test_set_editor : && (echo e; echo a) | git add -p && - git diff > diff && + git diff | sed /^index/d >diff && test_cmp expected diff ' @@ -145,7 +143,6 @@ test_expect_success 'setup patch' ' test_expect_success 'setup expected' ' cat >expected <<-EOF diff --git a/file b/file - index b5dd6c9..f910ae9 100644 --- a/file +++ b/file @@ -1,4 +1,4 @@ @@ -159,7 +156,7 @@ test_expect_success 'setup expected' ' test_expect_success 'real edit works' ' (echo e; echo n; echo d) | git add -p && - git diff >output && + git diff | sed /^index/d >output && test_cmp expected output ' @@ -168,10 +165,10 @@ test_expect_success 'skip files similarly as commit -a' ' echo file >.gitignore && echo changed >file && echo y | git add -p file && - git diff >output && + git diff | sed /^index/d >output && git reset && git commit -am commit && - git diff >expected && + git diff | sed /^index/d >expected && test_cmp expected output && git reset --hard HEAD^ ' @@ -217,7 +214,6 @@ test_expect_success 'setup again' ' # Write the patch file with a new line at the top and bottom test_expect_success 'setup patch' ' cat >patch <<-EOF - index 180b47c..b6f2c08 100644 --- a/file +++ b/file @@ -1,2 +1,4 @@ @@ -232,7 +228,6 @@ test_expect_success 'setup patch' ' test_expect_success 'setup expected' ' cat >expected <<-EOF diff --git a/file b/file - index b6f2c08..61b9053 100755 --- a/file +++ b/file @@ -1,2 +1,4 @@ @@ -248,15 +243,14 @@ test_expect_success 'add first line works' ' git commit -am "clear local changes" && git apply patch && (echo s; echo y; echo y) | git add -p file && - git diff --cached > diff && + git diff --cached | sed /^index/d >diff && test_cmp expected diff ' test_expect_success 'setup expected' ' cat >expected <<-EOF diff --git a/non-empty b/non-empty deleted file mode 100644 - index d95f3ad..0000000 --- a/non-empty +++ /dev/null @@ -1 +0,0 @@ @@ -271,15 +265,14 @@ test_expect_success 'deleting a non-empty file' ' git commit -m non-empty && rm non-empty && echo y | git add -p non-empty && - git diff --cached >diff && + git diff --cached | sed /^index/d >diff && test_cmp expected diff ' test_expect_success 'setup expected' ' cat >expected <<-EOF diff --git a/empty b/empty deleted file mode 100644 - index e69de29..0000000 EOF ' @@ -290,7 +283,7 @@ test_expect_success 'deleting an empty file' ' git commit -m empty && rm empty && echo y | git add -p empty && - git diff --cached >diff && + git diff --cached | sed /^index/d >diff && test_cmp expected diff ' @@ -317,7 +310,7 @@ test_expect_success 'split hunk "add -p (edit)"' ' # exits. printf "%s\n" s e q n q q | EDITOR=: git add -p && - git diff >actual && + git diff | sed /^index/d >actual && ! grep "^+15" actual ' @@ -329,7 +322,7 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' printf "%s\n" s n y e q n q q | EDITOR=: git add -p 2>error && test_must_be_empty error && - git diff >actual && + git diff | sed /^index/d >actual && ! grep "^+31" actual ' @@ -348,14 +341,13 @@ test_expect_success 'patch mode ignores unmerged entries' ' cat >expected <<-\EOF && * Unmerged path conflict.t diff --git a/non-conflict.t b/non-conflict.t - index f766221..5ea2ed4 100644 --- a/non-conflict.t +++ b/non-conflict.t @@ -1 +1 @@ -non-conflict +changed EOF - git diff --cached >diff && + git diff --cached | sed /^index/d >diff && test_cmp expected diff ' -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v2 4/9] t3701: don't hard code sha1 hash values 2018-02-19 11:29 ` [PATCH v2 4/9] t3701: don't hard code sha1 hash values Phillip Wood @ 2018-02-19 18:52 ` Eric Sunshine 2018-02-20 17:39 ` Junio C Hamano 1 sibling, 0 replies; 79+ messages in thread From: Eric Sunshine @ 2018-02-19 18:52 UTC (permalink / raw) To: Phillip Wood; +Cc: Git Mailing List, Junio C Hamano, Brian M. Carlson On Mon, Feb 19, 2018 at 6:29 AM, Phillip Wood <phillip.wood@talktalk.net> wrote: > Purge the index lines from diffs so we're not hard coding sha1 hash > values in the expected output. Perhaps the commit message could provide a bit more explanation about why this is a good idea. For instance, briefly mention that doing so will ease Git's transition from SHA1 to some newer hash function. > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v2 4/9] t3701: don't hard code sha1 hash values 2018-02-19 11:29 ` [PATCH v2 4/9] t3701: don't hard code sha1 hash values Phillip Wood 2018-02-19 18:52 ` Eric Sunshine @ 2018-02-20 17:39 ` Junio C Hamano 2018-02-21 11:42 ` Phillip Wood 1 sibling, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2018-02-20 17:39 UTC (permalink / raw) To: Phillip Wood; +Cc: Git Mailing List, Brian M. Carlson, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Purge the index lines from diffs so we're not hard coding sha1 hash > values in the expected output. The motivation of this patch is clear, but all-zero object name for missing side of deletion or creation patch should not change when we transition to any hash function. Neither the permission bits shown in the output (and whether the index line has the bits are shown on it in the first place, i.e. the index line of a creation patch does not, whilethe one in a modification patch does). So I am a bit ambivalent about this change. Perhaps have a filter that redacts, instead of removes, selected pieces of information that are likely to change while hash transition, and use that consistently to filter both the expected output and the actual output before comparing? ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v2 4/9] t3701: don't hard code sha1 hash values 2018-02-20 17:39 ` Junio C Hamano @ 2018-02-21 11:42 ` Phillip Wood 2018-02-21 16:58 ` Junio C Hamano 0 siblings, 1 reply; 79+ messages in thread From: Phillip Wood @ 2018-02-21 11:42 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, Brian M. Carlson, Phillip Wood On 20/02/18 17:39, Junio C Hamano wrote: > Phillip Wood <phillip.wood@talktalk.net> writes: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> Purge the index lines from diffs so we're not hard coding sha1 hash >> values in the expected output. > > The motivation of this patch is clear, but all-zero object name for > missing side of deletion or creation patch should not change when we > transition to any hash function. Neither the permission bits shown > in the output (and whether the index line has the bits are shown on > it in the first place, i.e. the index line of a creation patch does > not, whilethe one in a modification patch does). > > So I am a bit ambivalent about this change. > > Perhaps have a filter that redacts, instead of removes, selected > pieces of information that are likely to change while hash > transition, and use that consistently to filter both the expected > output and the actual output before comparing? > Keeping the permission bits makes sense (I'd not thought of them when I created the patch) as we want to check that the file has the correct permissions. As for the all-zero object name, is it really worth leaving it in - if a file has been created or deleted then we'll still have /dev/null as the file name for one side or the other and the diff lines will show it as well. As these tests are just to check the state of the index then I'm not sure the hash values add anything. How do you feel about a filter like sed "/^ index/s/ [0-9a-f][0-9a-f]*\.\.[0-9a-f][0-9a-f]*/x/" Best Wishes Phillip ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v2 4/9] t3701: don't hard code sha1 hash values 2018-02-21 11:42 ` Phillip Wood @ 2018-02-21 16:58 ` Junio C Hamano 0 siblings, 0 replies; 79+ messages in thread From: Junio C Hamano @ 2018-02-21 16:58 UTC (permalink / raw) To: Phillip Wood; +Cc: Git Mailing List, Brian M. Carlson, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > Keeping the permission bits makes sense (I'd not thought of them when > I created the patch) as we want to check that the file has the correct > permissions. As for the all-zero object name, is it really worth > leaving it in - if a file has been created or deleted then we'll still > have /dev/null as the file name for one side or the other and the diff > lines will show it as well. As these tests are just to check the state > of the index then I'm not sure the hash values add anything. How do > you feel about a filter like > > sed "/^ index/s/ [0-9a-f][0-9a-f]*\.\.[0-9a-f][0-9a-f]*/x/" Something like that, with perhaps the single 'x' replaced with something more normal looking and the all-zero thing special cased, was what I had in mind. Special casing all-zero matters. I know that the current code uses all-zero on the missing side. The thing is, tests are about protecting the correctness we currently have, and we want to catch the next idiot that breaks the code to stop showing all-zero when talking about the missing side. ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v2 5/9] t3701: add failing test for pathological context lines 2018-02-19 11:29 ` [PATCH v2 0/9] " Phillip Wood ` (3 preceding siblings ...) 2018-02-19 11:29 ` [PATCH v2 4/9] t3701: don't hard code sha1 hash values Phillip Wood @ 2018-02-19 11:29 ` Phillip Wood 2018-02-21 11:28 ` Phillip Wood 2018-02-19 11:29 ` [PATCH v2 6/9] add -p: Adjust offsets of subsequent hunks when one is skipped Phillip Wood ` (3 subsequent siblings) 8 siblings, 1 reply; 79+ messages in thread From: Phillip Wood @ 2018-02-19 11:29 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Brian M. Carlson, 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> --- Notes: changes since v1: - changed edit test to use the existing fake editor and to strip the hunk header and some context lines as well as deleting an insertion - style fixes t/t3701-add-interactive.sh | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 70748393f28c93f4bc5d43b07bd96bd208aba3e9..06c4747f506a1b05ccad0f9387e1fbd69cfd7784 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -511,4 +511,35 @@ 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 && + # check editing can cope with missing header and deleted context lines + # as well as changes to other lines + test_write_lines +b " a" >patch +' + +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 && + test_set_editor "$FAKE_EDITOR" && + # n q q below is in case edit fails + printf "%s\n" e y n q q | + git add -p && + git cat-file blob :a >actual && + test_cmp expected-2 actual +' + test_done -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v2 5/9] t3701: add failing test for pathological context lines 2018-02-19 11:29 ` [PATCH v2 5/9] t3701: add failing test for pathological context lines Phillip Wood @ 2018-02-21 11:28 ` Phillip Wood 0 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-02-21 11:28 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Phillip Wood On 19/02/18 11:29, Phillip Wood wrote: > 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> > --- > > Notes: > changes since v1: > - changed edit test to use the existing fake editor and to strip > the hunk header and some context lines as well as deleting an > insertion > - style fixes > > t/t3701-add-interactive.sh | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > index 70748393f28c93f4bc5d43b07bd96bd208aba3e9..06c4747f506a1b05ccad0f9387e1fbd69cfd7784 100755 > --- a/t/t3701-add-interactive.sh > +++ b/t/t3701-add-interactive.sh > @@ -511,4 +511,35 @@ 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 && > + # check editing can cope with missing header and deleted context lines > + # as well as changes to other lines > + test_write_lines +b " a" >patch > +' > + > +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 && > + test_set_editor "$FAKE_EDITOR" && In light of the discussion of patch 2, I think this line should be deleted > + # n q q below is in case edit fails > + printf "%s\n" e y n q q | > + git add -p && > + git cat-file blob :a >actual && > + test_cmp expected-2 actual > +' > + > test_done > ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v2 6/9] add -p: Adjust offsets of subsequent hunks when one is skipped 2018-02-19 11:29 ` [PATCH v2 0/9] " Phillip Wood ` (4 preceding siblings ...) 2018-02-19 11:29 ` [PATCH v2 5/9] t3701: add failing test for pathological context lines Phillip Wood @ 2018-02-19 11:29 ` Phillip Wood 2018-02-19 11:29 ` [PATCH v2 7/9] add -p: calculate offset delta for edited patches Phillip Wood ` (2 subsequent siblings) 8 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-02-19 11:29 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Brian M. Carlson, 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 06c4747f506a1b05ccad0f9387e1fbd69cfd7784..e153a02605df25ea40e49dd48b7c9fd981713b9d 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -524,7 +524,7 @@ test_expect_success 'set up pathological context' ' test_write_lines +b " a" >patch ' -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 related [flat|nested] 79+ messages in thread
* [PATCH v2 7/9] add -p: calculate offset delta for edited patches 2018-02-19 11:29 ` [PATCH v2 0/9] " Phillip Wood ` (5 preceding siblings ...) 2018-02-19 11:29 ` [PATCH v2 6/9] add -p: Adjust offsets of subsequent hunks when one is skipped Phillip Wood @ 2018-02-19 11:29 ` Phillip Wood 2018-02-19 11:29 ` [PATCH v2 8/9] add -p: fix counting when splitting and coalescing Phillip Wood 2018-02-19 11:29 ` [PATCH v2 9/9] add -p: don't rely on apply's '--recount' option Phillip Wood 8 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-02-19 11:29 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Brian M. Carlson, 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> --- Notes: changes since v1 - edited hunks are now recounted before seeing if they apply in preparation for removing '--recount' when invoking 'git apply' - added sentence to commit message about the offset data being lost if an edited hunk is split git-add--interactive.perl | 55 ++++++++++++++++++++++++++++++++++++++-------- t/t3701-add-interactive.sh | 2 +- 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 7a0a5896bb8fa063ace29b85840849c867b874f5..0df0c2aa065af88e159f8e9a2febe12f4ef8ee75 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -938,13 +938,19 @@ 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) { $n_ofs += $ofs_delta; $_->{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,32 @@ 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, }; + $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}; 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->{DISPLAY} = [color_diff(@{$newtext})]; return $newhunk; } else { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index e153a02605df25ea40e49dd48b7c9fd981713b9d..bbda771ba7e516aa37a204beffba7eeb0c85a2f4 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -532,7 +532,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 && test_set_editor "$FAKE_EDITOR" && # n q q below is in case edit fails -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v2 8/9] add -p: fix counting when splitting and coalescing 2018-02-19 11:29 ` [PATCH v2 0/9] " Phillip Wood ` (6 preceding siblings ...) 2018-02-19 11:29 ` [PATCH v2 7/9] add -p: calculate offset delta for edited patches Phillip Wood @ 2018-02-19 11:29 ` Phillip Wood 2018-02-19 11:29 ` [PATCH v2 9/9] add -p: don't rely on apply's '--recount' option Phillip Wood 8 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-02-19 11:29 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Brian M. Carlson, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> When a file has no trailing new line at the end diff records this by appending "\ No newline at end of file" below the last line of the file. This line should not be counted in the hunk header. Fix the splitting and coalescing code to count files without a trailing new line properly and change one of the tests to test splitting without a trailing new line. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- git-add--interactive.perl | 13 +++++++++---- t/t3701-add-interactive.sh | 28 ++++++++++++++++++++++------ 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 0df0c2aa065af88e159f8e9a2febe12f4ef8ee75..3226c2c4f02d5f8679d77b8eede984fc727b422d 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -793,6 +793,11 @@ sub split_hunk { while (++$i < @$text) { my $line = $text->[$i]; my $display = $display->[$i]; + if ($line =~ /^\\/) { + push @{$this->{TEXT}}, $line; + push @{$this->{DISPLAY}}, $display; + next; + } if ($line =~ /^ /) { if ($this->{ADDDEL} && !defined $next_hunk_start) { @@ -887,8 +892,8 @@ sub merge_hunk { $o_cnt = $n_cnt = 0; for ($i = 1; $i < @{$prev->{TEXT}}; $i++) { my $line = $prev->{TEXT}[$i]; - if ($line =~ /^\+/) { - $n_cnt++; + if ($line =~ /^[+\\]/) { + $n_cnt++ if ($line =~ /^\+/); push @line, $line; next; } @@ -905,8 +910,8 @@ sub merge_hunk { for ($i = 1; $i < @{$this->{TEXT}}; $i++) { my $line = $this->{TEXT}[$i]; - if ($line =~ /^\+/) { - $n_cnt++; + if ($line =~ /^[+\\]/) { + $n_cnt++ if ($line =~ /^\+/); push @line, $line; next; } diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index bbda771ba7e516aa37a204beffba7eeb0c85a2f4..0fb9c0e0f140e21ef7ad467c40b9211d29f53db6 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -221,30 +221,46 @@ test_expect_success 'setup patch' ' baseline content +lastline + \ No newline at end of file EOF ' -# Expected output, similar to the patch but w/ diff at the top +# Expected output, diff is similar to the patch but w/ diff at the top test_expect_success 'setup expected' ' - cat >expected <<-EOF - diff --git a/file b/file + echo diff --git a/file b/file >expected && + cat patch >>expected && + cat >expected-output <<-EOF --- a/file +++ b/file @@ -1,2 +1,4 @@ +firstline baseline content +lastline + \ No newline at end of file + @@ -1,2 +1,3 @@ + +firstline + baseline + content + @@ -1,2 +2,3 @@ + baseline + content + +lastline + \ No newline at end of file EOF ' # Test splitting the first patch, then adding both -test_expect_success 'add first line works' ' +test_expect_success C_LOCALE_OUTPUT 'add first line works' ' git commit -am "clear local changes" && git apply patch && - (echo s; echo y; echo y) | git add -p file && + printf "%s\n" s y y | git add -p file 2>error | + sed -n -e "s/^Stage this hunk[^@]*\(@@ .*\)/\1/" \ + -e "/^[-+@ \\\\]"/p >output && + test_must_be_empty error && git diff --cached | sed /^index/d >diff && - test_cmp expected diff + test_cmp expected diff && + test_cmp expected-output output ' test_expect_success 'setup expected' ' -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v2 9/9] add -p: don't rely on apply's '--recount' option 2018-02-19 11:29 ` [PATCH v2 0/9] " Phillip Wood ` (7 preceding siblings ...) 2018-02-19 11:29 ` [PATCH v2 8/9] add -p: fix counting when splitting and coalescing Phillip Wood @ 2018-02-19 11:29 ` Phillip Wood 2018-02-20 18:50 ` Junio C Hamano 8 siblings, 1 reply; 79+ messages in thread From: Phillip Wood @ 2018-02-19 11:29 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Brian M. Carlson, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Now that add -p counts patches properly it should be possible to turn off the '--recount' option when invoking 'git apply' Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Notes: I can't think of a reason why this shouldn't be OK but I can't help feeling slightly nervous about it. I've made it a separate patch so it can be easily dropped or reverted if I've missed something. git-add--interactive.perl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 3226c2c4f02d5f8679d77b8eede984fc727b422d..a64c0db57d62ab02ef718b8c8f821105132d9920 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -677,7 +677,7 @@ sub add_untracked_cmd { sub run_git_apply { my $cmd = shift; my $fh; - open $fh, '| git ' . $cmd . " --recount --allow-overlap"; + open $fh, '| git ' . $cmd . " --allow-overlap"; print $fh @_; return close $fh; } -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v2 9/9] add -p: don't rely on apply's '--recount' option 2018-02-19 11:29 ` [PATCH v2 9/9] add -p: don't rely on apply's '--recount' option Phillip Wood @ 2018-02-20 18:50 ` Junio C Hamano 0 siblings, 0 replies; 79+ messages in thread From: Junio C Hamano @ 2018-02-20 18:50 UTC (permalink / raw) To: Phillip Wood; +Cc: Git Mailing List, Brian M. Carlson, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Now that add -p counts patches properly it should be possible to turn > off the '--recount' option when invoking 'git apply' > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > > Notes: > I can't think of a reason why this shouldn't be OK but I can't help > feeling slightly nervous about it. I've made it a separate patch so it > can be easily dropped or reverted if I've missed something. It is a lot more preferrable approach to produce a patch with information as precise as the producer (i.e. add -p) can and feed it to the consumer (i.e. apply) than to rely on the --recount and the --allow-overlap options that are mainly ways to force the consumer make imprecise guesses and accepting potential garbage with looser consistency checks. So I very much like the direction (and the next step may be to make sure we coalesce correctly so that we do not have to use "--allow-overlap"). Thanks for working on this. Let's see if the updated code really "counts patches properly" as the log message claims to ;-) > git-add--interactive.perl | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/git-add--interactive.perl b/git-add--interactive.perl > index 3226c2c4f02d5f8679d77b8eede984fc727b422d..a64c0db57d62ab02ef718b8c8f821105132d9920 100755 > --- a/git-add--interactive.perl > +++ b/git-add--interactive.perl > @@ -677,7 +677,7 @@ sub add_untracked_cmd { > sub run_git_apply { > my $cmd = shift; > my $fh; > - open $fh, '| git ' . $cmd . " --recount --allow-overlap"; > + open $fh, '| git ' . $cmd . " --allow-overlap"; > print $fh @_; > return close $fh; > } ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v3 0/9] 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 ` (5 preceding siblings ...) 2018-02-19 11:29 ` [PATCH v2 0/9] " Phillip Wood @ 2018-02-27 11:03 ` Phillip Wood 2018-02-27 11:03 ` [PATCH v3 1/9] add -i: add function to format hunk header Phillip Wood ` (8 more replies) 2018-03-01 10:50 ` [PATCH v4 0/9] Correct offsets of hunks when one is skipped Phillip Wood 2018-03-05 10:56 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Phillip Wood 8 siblings, 9 replies; 79+ messages in thread From: Phillip Wood @ 2018-02-27 11:03 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> I've fixed the use of test_set_editor most of which was already in pu and reworked the sha1 comparisons to rewrite the hashes in the index lines rather than deleting the index lines. Cover letter to v1: 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. Interdiff: diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 0fb9c0e0f1..05540ee9ef 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -10,6 +10,16 @@ then test_done fi +diff_cmp () { + for x + do + sed -e '/^index/s/[0-9a-f]*[1-9a-f][0-9a-f]*\.\./1234567../' \ + -e '/^index/s/\.\.[0-9a-f]*[1-9a-f][0-9a-f]*/..9abcdef/' \ + "$x" >"$x.filtered" + done + test_cmp "$1.filtered" "$2.filtered" +} + test_expect_success 'setup (initial)' ' echo content >file && git add file && @@ -24,6 +34,7 @@ test_expect_success 'status works (initial)' ' test_expect_success 'setup expected' ' cat >expected <<-EOF new file mode 100644 + index 0000000..d95f3ad --- /dev/null +++ b/file @@ -0,0 +1 @@ @@ -33,8 +44,8 @@ test_expect_success 'setup expected' ' test_expect_success 'diff works (initial)' ' (echo d; echo 1) | git add -i >output && - sed -ne /^index/d -e "/new file/,/content/p" <output >diff && - test_cmp expected diff + sed -ne "/new file/,/content/p" <output >diff && + diff_cmp expected diff ' test_expect_success 'revert works (initial)' ' git add file && @@ -59,6 +70,7 @@ test_expect_success 'status works (commit)' ' test_expect_success 'setup expected' ' cat >expected <<-EOF + index 180b47c..b6f2c08 100644 --- a/file +++ b/file @@ -1 +1,2 @@ @@ -69,8 +81,8 @@ test_expect_success 'setup expected' ' test_expect_success 'diff works (commit)' ' (echo d; echo 1) | git add -i >output && - sed -ne "/^---/,/content/p" <output >diff && - test_cmp expected diff + sed -ne "/^index/,/content/p" <output >diff && + diff_cmp expected diff ' test_expect_success 'revert works (commit)' ' git add file && @@ -88,8 +100,8 @@ test_expect_success 'setup expected' ' test_expect_success 'dummy edit works' ' test_set_editor : && (echo e; echo a) | git add -p && - git diff | sed /^index/d >diff && - test_cmp expected diff + git diff > diff && + diff_cmp expected diff ' test_expect_success 'setup patch' ' @@ -103,12 +115,11 @@ test_expect_success 'setup patch' ' ' test_expect_success 'setup fake editor' ' - FAKE_EDITOR="$(pwd)/fake-editor.sh" && - write_script "$FAKE_EDITOR" <<-\EOF && + write_script "fake_editor.sh" <<-\EOF && mv -f "$1" oldpatch && mv -f patch "$1" EOF - test_set_editor "$FAKE_EDITOR" + test_set_editor "$(pwd)/fake_editor.sh" ' test_expect_success 'bad edit rejected' ' @@ -143,6 +154,7 @@ test_expect_success 'setup patch' ' test_expect_success 'setup expected' ' cat >expected <<-EOF diff --git a/file b/file + index b5dd6c9..f910ae9 100644 --- a/file +++ b/file @@ -1,4 +1,4 @@ @@ -156,8 +168,8 @@ test_expect_success 'setup expected' ' test_expect_success 'real edit works' ' (echo e; echo n; echo d) | git add -p && - git diff | sed /^index/d >output && - test_cmp expected output + git diff >output && + diff_cmp expected output ' test_expect_success 'skip files similarly as commit -a' ' @@ -165,11 +177,11 @@ test_expect_success 'skip files similarly as commit -a' ' echo file >.gitignore && echo changed >file && echo y | git add -p file && - git diff | sed /^index/d >output && + git diff >output && git reset && git commit -am commit && - git diff | sed /^index/d >expected && - test_cmp expected output && + git diff >expected && + diff_cmp expected output && git reset --hard HEAD^ ' rm -f .gitignore @@ -214,6 +226,7 @@ test_expect_success 'setup again' ' # Write the patch file with a new line at the top and bottom test_expect_success 'setup patch' ' cat >patch <<-EOF + index 180b47c..b6f2c08 100644 --- a/file +++ b/file @@ -1,2 +1,4 @@ @@ -228,7 +241,7 @@ test_expect_success 'setup patch' ' # Expected output, diff is similar to the patch but w/ diff at the top test_expect_success 'setup expected' ' echo diff --git a/file b/file >expected && - cat patch >>expected && + cat patch |sed "/^index/s/ 100644/ 100755/" >>expected && cat >expected-output <<-EOF --- a/file +++ b/file @@ -258,8 +271,8 @@ test_expect_success C_LOCALE_OUTPUT 'add first line works' ' sed -n -e "s/^Stage this hunk[^@]*\(@@ .*\)/\1/" \ -e "/^[-+@ \\\\]"/p >output && test_must_be_empty error && - git diff --cached | sed /^index/d >diff && - test_cmp expected diff && + git diff --cached >diff && + diff_cmp expected diff && test_cmp expected-output output ' @@ -267,6 +280,7 @@ test_expect_success 'setup expected' ' cat >expected <<-EOF diff --git a/non-empty b/non-empty deleted file mode 100644 + index d95f3ad..0000000 --- a/non-empty +++ /dev/null @@ -1 +0,0 @@ @@ -281,14 +295,15 @@ test_expect_success 'deleting a non-empty file' ' git commit -m non-empty && rm non-empty && echo y | git add -p non-empty && - git diff --cached | sed /^index/d >diff && - test_cmp expected diff + git diff --cached >diff && + diff_cmp expected diff ' test_expect_success 'setup expected' ' cat >expected <<-EOF diff --git a/empty b/empty deleted file mode 100644 + index e69de29..0000000 EOF ' @@ -299,8 +314,8 @@ test_expect_success 'deleting an empty file' ' git commit -m empty && rm empty && echo y | git add -p empty && - git diff --cached | sed /^index/d >diff && - test_cmp expected diff + git diff --cached >diff && + diff_cmp expected diff ' test_expect_success 'split hunk setup' ' @@ -326,7 +341,7 @@ test_expect_success 'split hunk "add -p (edit)"' ' # exits. printf "%s\n" s e q n q q | EDITOR=: git add -p && - git diff | sed /^index/d >actual && + git diff >actual && ! grep "^+15" actual ' @@ -338,7 +353,7 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' printf "%s\n" s n y e q n q q | EDITOR=: git add -p 2>error && test_must_be_empty error && - git diff | sed /^index/d >actual && + git diff >actual && ! grep "^+31" actual ' @@ -357,14 +372,15 @@ test_expect_success 'patch mode ignores unmerged entries' ' cat >expected <<-\EOF && * Unmerged path conflict.t diff --git a/non-conflict.t b/non-conflict.t + index f766221..5ea2ed4 100644 --- a/non-conflict.t +++ b/non-conflict.t @@ -1 +1 @@ -non-conflict +changed EOF - git diff --cached | sed /^index/d >diff && - test_cmp expected diff + git diff --cached >diff && + diff_cmp expected diff ' test_expect_success TTY 'diffs can be colorized' ' @@ -393,7 +409,7 @@ test_expect_success 'patch-mode via -i prompts for files' ' echo test >expect && git diff --cached --name-only >actual && - test_cmp expect actual + diff_cmp expect actual ' test_expect_success 'add -p handles globs' ' @@ -550,7 +566,6 @@ test_expect_success 'add -p works with pathological context lines' ' test_expect_success 'add -p patch editing works with pathological context lines' ' git reset && - test_set_editor "$FAKE_EDITOR" && # n q q below is in case edit fails printf "%s\n" e y n q q | git add -p && Phillip Wood (9): add -i: add function to format hunk header t3701: indent here documents t3701: use test_write_lines and write_script t3701: don't hard code sha1 hash values 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 add -p: fix counting when splitting and coalescing add -p: don't rely on apply's '--recount' option git-add--interactive.perl | 106 +++++++++++++---- t/t3701-add-interactive.sh | 288 +++++++++++++++++++++++++-------------------- 2 files changed, 240 insertions(+), 154 deletions(-) -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v3 1/9] add -i: add function to format hunk header 2018-02-27 11:03 ` [PATCH v3 0/9] Correct offsets of hunks when one is skipped Phillip Wood @ 2018-02-27 11:03 ` Phillip Wood 2018-02-27 11:03 ` [PATCH v3 2/9] t3701: indent here documents Phillip Wood ` (7 subsequent siblings) 8 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-02-27 11:03 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, 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 964c3a7542..8ababa6453 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 related [flat|nested] 79+ messages in thread
* [PATCH v3 2/9] t3701: indent here documents 2018-02-27 11:03 ` [PATCH v3 0/9] Correct offsets of hunks when one is skipped Phillip Wood 2018-02-27 11:03 ` [PATCH v3 1/9] add -i: add function to format hunk header Phillip Wood @ 2018-02-27 11:03 ` Phillip Wood 2018-02-27 22:35 ` Junio C Hamano 2018-02-27 11:03 ` [PATCH v3 3/9] t3701: use test_write_lines and write_script Phillip Wood ` (6 subsequent siblings) 8 siblings, 1 reply; 79+ messages in thread From: Phillip Wood @ 2018-02-27 11:03 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Indent here documents in line with the current style for tests. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- t/t3701-add-interactive.sh | 174 ++++++++++++++++++++++----------------------- 1 file changed, 87 insertions(+), 87 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 058698df6a..861ea2e08c 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -22,14 +22,14 @@ test_expect_success 'status works (initial)' ' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -new file mode 100644 -index 0000000..d95f3ad ---- /dev/null -+++ b/file -@@ -0,0 +1 @@ -+content -EOF + cat >expected <<-EOF + new file mode 100644 + index 0000000..d95f3ad + --- /dev/null + +++ b/file + @@ -0,0 +1 @@ + +content + EOF ' test_expect_success 'diff works (initial)' ' @@ -59,14 +59,14 @@ test_expect_success 'status works (commit)' ' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -index 180b47c..b6f2c08 100644 ---- a/file -+++ b/file -@@ -1 +1,2 @@ - baseline -+content -EOF + cat >expected <<-EOF + index 180b47c..b6f2c08 100644 + --- a/file + +++ b/file + @@ -1 +1,2 @@ + baseline + +content + EOF ' test_expect_success 'diff works (commit)' ' @@ -83,8 +83,8 @@ test_expect_success 'revert works (commit)' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -EOF + cat >expected <<-EOF + EOF ' test_expect_success 'setup fake editor' ' @@ -100,21 +100,21 @@ test_expect_success 'dummy edit works' ' ' test_expect_success 'setup patch' ' -cat >patch <<EOF -@@ -1,1 +1,4 @@ - this -+patch --does not - apply -EOF + cat >patch <<-EOF + @@ -1,1 +1,4 @@ + this + +patch + -does not + apply + EOF ' test_expect_success 'setup fake editor' ' echo "#!$SHELL_PATH" >fake_editor.sh && - cat >>fake_editor.sh <<\EOF && -mv -f "$1" oldpatch && -mv -f patch "$1" -EOF + cat >>fake_editor.sh <<-\EOF && + mv -f "$1" oldpatch && + mv -f patch "$1" + EOF chmod a+x fake_editor.sh && test_set_editor "$(pwd)/fake_editor.sh" ' @@ -126,10 +126,10 @@ test_expect_success 'bad edit rejected' ' ' test_expect_success 'setup patch' ' -cat >patch <<EOF -this patch -is garbage -EOF + cat >patch <<-EOF + this patch + is garbage + EOF ' test_expect_success 'garbage edit rejected' ' @@ -139,28 +139,28 @@ test_expect_success 'garbage edit rejected' ' ' test_expect_success 'setup patch' ' -cat >patch <<EOF -@@ -1,0 +1,0 @@ - baseline -+content -+newcontent -+lines -EOF + cat >patch <<-EOF + @@ -1,0 +1,0 @@ + baseline + +content + +newcontent + +lines + EOF ' test_expect_success 'setup expected' ' -cat >expected <<EOF -diff --git a/file b/file -index b5dd6c9..f910ae9 100644 ---- a/file -+++ b/file -@@ -1,4 +1,4 @@ - baseline - content --newcontent -+more - lines -EOF + cat >expected <<-EOF + diff --git a/file b/file + index b5dd6c9..f910ae9 100644 + --- a/file + +++ b/file + @@ -1,4 +1,4 @@ + baseline + content + -newcontent + +more + lines + EOF ' test_expect_success 'real edit works' ' @@ -222,31 +222,31 @@ test_expect_success 'setup again' ' # Write the patch file with a new line at the top and bottom test_expect_success 'setup patch' ' -cat >patch <<EOF -index 180b47c..b6f2c08 100644 ---- a/file -+++ b/file -@@ -1,2 +1,4 @@ -+firstline - baseline - content -+lastline -EOF + cat >patch <<-EOF + index 180b47c..b6f2c08 100644 + --- a/file + +++ b/file + @@ -1,2 +1,4 @@ + +firstline + baseline + content + +lastline + EOF ' # Expected output, similar to the patch but w/ diff at the top test_expect_success 'setup expected' ' -cat >expected <<EOF -diff --git a/file b/file -index b6f2c08..61b9053 100755 ---- a/file -+++ b/file -@@ -1,2 +1,4 @@ -+firstline - baseline - content -+lastline -EOF + cat >expected <<-EOF + diff --git a/file b/file + index b6f2c08..61b9053 100755 + --- a/file + +++ b/file + @@ -1,2 +1,4 @@ + +firstline + baseline + content + +lastline + EOF ' # Test splitting the first patch, then adding both @@ -259,15 +259,15 @@ test_expect_success 'add first line works' ' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -diff --git a/non-empty b/non-empty -deleted file mode 100644 -index d95f3ad..0000000 ---- a/non-empty -+++ /dev/null -@@ -1 +0,0 @@ --content -EOF + cat >expected <<-EOF + diff --git a/non-empty b/non-empty + deleted file mode 100644 + index d95f3ad..0000000 + --- a/non-empty + +++ /dev/null + @@ -1 +0,0 @@ + -content + EOF ' test_expect_success 'deleting a non-empty file' ' @@ -282,11 +282,11 @@ test_expect_success 'deleting a non-empty file' ' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -diff --git a/empty b/empty -deleted file mode 100644 -index e69de29..0000000 -EOF + cat >expected <<-EOF + diff --git a/empty b/empty + deleted file mode 100644 + index e69de29..0000000 + EOF ' test_expect_success 'deleting an empty file' ' -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v3 2/9] t3701: indent here documents 2018-02-27 11:03 ` [PATCH v3 2/9] t3701: indent here documents Phillip Wood @ 2018-02-27 22:35 ` Junio C Hamano 2018-02-28 11:00 ` Phillip Wood 0 siblings, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2018-02-27 22:35 UTC (permalink / raw) To: Phillip Wood Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Indent here documents in line with the current style for tests. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- This loses the hand-edit-while-queuing done based on Eric's comment for the previous round (see what has been queued on 'pu' for quite a while), which is not very much appreciated. Also you lost the title fix done while queuing on 6/9 (see 'pu'). ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3 2/9] t3701: indent here documents 2018-02-27 22:35 ` Junio C Hamano @ 2018-02-28 11:00 ` Phillip Wood 2018-02-28 15:37 ` Junio C Hamano 0 siblings, 1 reply; 79+ messages in thread From: Phillip Wood @ 2018-02-28 11:00 UTC (permalink / raw) To: Junio C Hamano Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood On 27/02/18 22:35, Junio C Hamano wrote: > Phillip Wood <phillip.wood@talktalk.net> writes: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> Indent here documents in line with the current style for tests. >> >> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >> --- > > This loses the hand-edit-while-queuing done based on Eric's comment > for the previous round (see what has been queued on 'pu' for quite a > while), which is not very much appreciated. Sorry as you had not commented on that patch I didn't realize you had edited it before queueing it. (There are probably other here documents in that file that were already indented that would benefit from Eric's suggestion.) > Also you lost the title fix done while queuing on 6/9 (see 'pu'). > Is there an easy way for contributors to compare the branch they post to what ends up it pu? Best Wishes Phillip ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3 2/9] t3701: indent here documents 2018-02-28 11:00 ` Phillip Wood @ 2018-02-28 15:37 ` Junio C Hamano 2018-03-01 10:57 ` Phillip Wood 0 siblings, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2018-02-28 15:37 UTC (permalink / raw) To: Phillip Wood Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > Is there an easy way for contributors to compare the branch they post to > what ends up it pu? Distributed work is pretty much symmetric, so it can be done the same way as one would review a rerolled series by another co-worker. $ git log --oneline --first-parent origin/master..origin/pu would show merges of topic branches, so you can find the tip of the topic of your earlier submission (it would be one $commit^2; call that $topic). origin/master..$topic would be the one branch (i.e. what is in 'pu') to be compared. The other branch to be compared is what you sent the previous one out of, or the new version of the patches. To compare two branches, git://github.com/trast/tbdiff is one of the easier way. Before I learned about the tool, I used to "format-patch --stdout" on both branches, and ran "diff -u" between them, as a crude measure; it was more useful for spotting typofixes in the log messages than code changes, before I got good at reading diff of diffs ;-). Also, tentatively rebasing the two branches on a common base, and then doing "git diff $oldtopic~$N $newtopic~$N" or something like that for varying value of $N (and N==0 is a good way for final sanity checks). ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3 2/9] t3701: indent here documents 2018-02-28 15:37 ` Junio C Hamano @ 2018-03-01 10:57 ` Phillip Wood 2018-03-01 15:58 ` Junio C Hamano 0 siblings, 1 reply; 79+ messages in thread From: Phillip Wood @ 2018-03-01 10:57 UTC (permalink / raw) To: Junio C Hamano Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood Hi Junio On 28/02/18 15:37, Junio C Hamano wrote: > Phillip Wood <phillip.wood@talktalk.net> writes: > >> Is there an easy way for contributors to compare the branch they post to >> what ends up it pu? > > Distributed work is pretty much symmetric, so it can be done the > same way as one would review a rerolled series by another co-worker. > > $ git log --oneline --first-parent origin/master..origin/pu > > would show merges of topic branches, so you can find the tip of the > topic of your earlier submission (it would be one $commit^2; call > that $topic). origin/master..$topic would be the one branch > (i.e. what is in 'pu') to be compared. > > The other branch to be compared is what you sent the previous one > out of, or the new version of the patches. > > To compare two branches, git://github.com/trast/tbdiff is one of the > easier way. > > Before I learned about the tool, I used to "format-patch --stdout" > on both branches, and ran "diff -u" between them, as a crude measure; > it was more useful for spotting typofixes in the log messages than > code changes, before I got good at reading diff of diffs ;-). > > Also, tentatively rebasing the two branches on a common base, and > then doing "git diff $oldtopic~$N $newtopic~$N" or something like > that for varying value of $N (and N==0 is a good way for final > sanity checks). Thanks for the tips, tbdiff looks useful (I just need to learn to read diffs of diffs!). I also find rebasing them on a common ancestor useful but its a bit tedious. Thanks again Phillip ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3 2/9] t3701: indent here documents 2018-03-01 10:57 ` Phillip Wood @ 2018-03-01 15:58 ` Junio C Hamano 0 siblings, 0 replies; 79+ messages in thread From: Junio C Hamano @ 2018-03-01 15:58 UTC (permalink / raw) To: Phillip Wood Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > Thanks for the tips, tbdiff looks useful (I just need to learn to read > diffs of diffs!). I also find rebasing them on a common ancestor useful > but its a bit tedious. Yes, comparing two versions of a series is somewhat unusual and needs getting used to before one can do so efficiently. You do not have to always rebase (tbdiff is fairly good at what it does even when two ranges are far apart), but it helps not to begin developing on a codebase that is newer than needed (e.g. a bugfix on 'next' is unneeded unless you are fixing a bug introduced by a topic that is still on 'next'---in which case the best fix is one that is on that topic, without depending on the rest of 'next'). In any case, thank _you_ for contributing. ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v3 3/9] t3701: use test_write_lines and write_script 2018-02-27 11:03 ` [PATCH v3 0/9] Correct offsets of hunks when one is skipped Phillip Wood 2018-02-27 11:03 ` [PATCH v3 1/9] add -i: add function to format hunk header Phillip Wood 2018-02-27 11:03 ` [PATCH v3 2/9] t3701: indent here documents Phillip Wood @ 2018-02-27 11:03 ` Phillip Wood 2018-02-27 11:03 ` [PATCH v3 4/9] t3701: don't hard code sha1 hash values Phillip Wood ` (5 subsequent siblings) 8 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-02-27 11:03 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Simplify things slightly by using the above helpers. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Notes: changes since v2 - fixed use of test_set_editor to match what was in pu t/t3701-add-interactive.sh | 33 +++++---------------------------- 1 file changed, 5 insertions(+), 28 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 861ea2e08c..bdd1f292a9 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -87,13 +87,8 @@ test_expect_success 'setup expected' ' EOF ' -test_expect_success 'setup fake editor' ' - >fake_editor.sh && - chmod a+x fake_editor.sh && - test_set_editor "$(pwd)/fake_editor.sh" -' - test_expect_success 'dummy edit works' ' + test_set_editor : && (echo e; echo a) | git add -p && git diff > diff && test_cmp expected diff @@ -110,12 +105,10 @@ test_expect_success 'setup patch' ' ' test_expect_success 'setup fake editor' ' - echo "#!$SHELL_PATH" >fake_editor.sh && - cat >>fake_editor.sh <<-\EOF && + write_script "fake_editor.sh" <<-\EOF && mv -f "$1" oldpatch && mv -f patch "$1" EOF - chmod a+x fake_editor.sh && test_set_editor "$(pwd)/fake_editor.sh" ' @@ -302,18 +295,12 @@ test_expect_success 'deleting an empty file' ' test_expect_success 'split hunk setup' ' git reset --hard && - for i in 10 20 30 40 50 60 - do - echo $i - done >test && + test_write_lines 10 20 30 40 50 60 >test && git add test && test_tick && git commit -m test && - for i in 10 15 20 21 22 23 24 30 40 50 60 - do - echo $i - done >test + test_write_lines 10 15 20 21 22 23 24 30 40 50 60 >test ' test_expect_success 'split hunk "add -p (edit)"' ' @@ -334,17 +321,7 @@ test_expect_success 'split hunk "add -p (edit)"' ' ' test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' - cat >test <<-\EOF && - 5 - 10 - 20 - 21 - 30 - 31 - 40 - 50 - 60 - EOF + test_write_lines 5 10 20 21 30 31 40 50 60 >test && git reset && # test sequence is s(plit), n(o), y(es), e(dit) # q n q q is there to make sure we exit at the end. -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v3 4/9] t3701: don't hard code sha1 hash values 2018-02-27 11:03 ` [PATCH v3 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (2 preceding siblings ...) 2018-02-27 11:03 ` [PATCH v3 3/9] t3701: use test_write_lines and write_script Phillip Wood @ 2018-02-27 11:03 ` Phillip Wood 2018-02-27 22:42 ` Junio C Hamano 2018-02-27 11:04 ` [PATCH v3 5/9] t3701: add failing test for pathological context lines Phillip Wood ` (4 subsequent siblings) 8 siblings, 1 reply; 79+ messages in thread From: Phillip Wood @ 2018-02-27 11:03 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Use a filter when comparing diffs to fix the value of non-zero hashes in diff index lines so we're not hard coding sha1 hash values in the expected output. This makes it easier to change the expected output if a test is edited as we don't need to worry about the exact hash value and means the tests will work when the hash algorithm is transitioned away from sha1. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Notes: changes since v2: - fix hash values in index lines rather than removing the line - reworded commit message t/t3701-add-interactive.sh | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index bdd1f292a9..46d655038f 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -10,6 +10,16 @@ then test_done fi +diff_cmp () { + for x + do + sed -e '/^index/s/[0-9a-f]*[1-9a-f][0-9a-f]*\.\./1234567../' \ + -e '/^index/s/\.\.[0-9a-f]*[1-9a-f][0-9a-f]*/..9abcdef/' \ + "$x" >"$x.filtered" + done + test_cmp "$1.filtered" "$2.filtered" +} + test_expect_success 'setup (initial)' ' echo content >file && git add file && @@ -35,7 +45,7 @@ test_expect_success 'setup expected' ' test_expect_success 'diff works (initial)' ' (echo d; echo 1) | git add -i >output && sed -ne "/new file/,/content/p" <output >diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'revert works (initial)' ' git add file && @@ -72,7 +82,7 @@ test_expect_success 'setup expected' ' test_expect_success 'diff works (commit)' ' (echo d; echo 1) | git add -i >output && sed -ne "/^index/,/content/p" <output >diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'revert works (commit)' ' git add file && @@ -91,7 +101,7 @@ test_expect_success 'dummy edit works' ' test_set_editor : && (echo e; echo a) | git add -p && git diff > diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'setup patch' ' @@ -159,7 +169,7 @@ test_expect_success 'setup expected' ' test_expect_success 'real edit works' ' (echo e; echo n; echo d) | git add -p && git diff >output && - test_cmp expected output + diff_cmp expected output ' test_expect_success 'skip files similarly as commit -a' ' @@ -171,7 +181,7 @@ test_expect_success 'skip files similarly as commit -a' ' git reset && git commit -am commit && git diff >expected && - test_cmp expected output && + diff_cmp expected output && git reset --hard HEAD^ ' rm -f .gitignore @@ -248,7 +258,7 @@ test_expect_success 'add first line works' ' git apply patch && (echo s; echo y; echo y) | git add -p file && git diff --cached > diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'setup expected' ' @@ -271,7 +281,7 @@ test_expect_success 'deleting a non-empty file' ' rm non-empty && echo y | git add -p non-empty && git diff --cached >diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'setup expected' ' @@ -290,7 +300,7 @@ test_expect_success 'deleting an empty file' ' rm empty && echo y | git add -p empty && git diff --cached >diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'split hunk setup' ' @@ -355,7 +365,7 @@ test_expect_success 'patch mode ignores unmerged entries' ' +changed EOF git diff --cached >diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success TTY 'diffs can be colorized' ' @@ -384,7 +394,7 @@ test_expect_success 'patch-mode via -i prompts for files' ' echo test >expect && git diff --cached --name-only >actual && - test_cmp expect actual + diff_cmp expect actual ' test_expect_success 'add -p handles globs' ' -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v3 4/9] t3701: don't hard code sha1 hash values 2018-02-27 11:03 ` [PATCH v3 4/9] t3701: don't hard code sha1 hash values Phillip Wood @ 2018-02-27 22:42 ` Junio C Hamano 2018-02-28 11:03 ` Phillip Wood 0 siblings, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2018-02-27 22:42 UTC (permalink / raw) To: Phillip Wood Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > t/t3701-add-interactive.sh | 30 ++++++++++++++++++++---------- > 1 file changed, 20 insertions(+), 10 deletions(-) > > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > index bdd1f292a9..46d655038f 100755 > --- a/t/t3701-add-interactive.sh > +++ b/t/t3701-add-interactive.sh > @@ -10,6 +10,16 @@ then > test_done > fi > > +diff_cmp () { > + for x > + do > + sed -e '/^index/s/[0-9a-f]*[1-9a-f][0-9a-f]*\.\./1234567../' \ > + -e '/^index/s/\.\.[0-9a-f]*[1-9a-f][0-9a-f]*/..9abcdef/' \ > + "$x" >"$x.filtered" Interesting ;-) You require .. and on the left hand side you want to see a run of hexdec with at least one non-zero hexdigit, which is filtered to fixed-length 1234567; right hand side is the same deal. Which sounds like a reasonable way to future-proof the comparison. If 7 zeros are expected in the result, and the actual output had 8 zeros, the filter does not touch either so they compare differently, which is somewhat unfortunate. Perhaps something like /^index/s/^00*\.\./0000000../ /^index/s/\([^0-9a-f]\)00*\.\./\10000000../ /^index/s/\.\.00*$/..0000000/ /^index/s/\.\.00*\([^0-9a-f]\)/..0000000\1/ after the above two patterns help? ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3 4/9] t3701: don't hard code sha1 hash values 2018-02-27 22:42 ` Junio C Hamano @ 2018-02-28 11:03 ` Phillip Wood 2018-02-28 11:10 ` Phillip Wood 0 siblings, 1 reply; 79+ messages in thread From: Phillip Wood @ 2018-02-28 11:03 UTC (permalink / raw) To: Junio C Hamano Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood On 27/02/18 22:42, Junio C Hamano wrote: > Phillip Wood <phillip.wood@talktalk.net> writes: > >> t/t3701-add-interactive.sh | 30 ++++++++++++++++++++---------- >> 1 file changed, 20 insertions(+), 10 deletions(-) >> >> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh >> index bdd1f292a9..46d655038f 100755 >> --- a/t/t3701-add-interactive.sh >> +++ b/t/t3701-add-interactive.sh >> @@ -10,6 +10,16 @@ then >> test_done >> fi >> >> +diff_cmp () { >> + for x >> + do >> + sed -e '/^index/s/[0-9a-f]*[1-9a-f][0-9a-f]*\.\./1234567../' \ >> + -e '/^index/s/\.\.[0-9a-f]*[1-9a-f][0-9a-f]*/..9abcdef/' \ >> + "$x" >"$x.filtered" > > Interesting ;-) You require .. and on the left hand side you want > to see a run of hexdec with at least one non-zero hexdigit, which is > filtered to fixed-length 1234567; right hand side is the same deal. > > Which sounds like a reasonable way to future-proof the comparison. > > If 7 zeros are expected in the result, and the actual output had 8 > zeros, the filter does not touch either so they compare differently, > which is somewhat unfortunate. Perhaps something like Ah, good point > /^index/s/^00*\.\./0000000../ > /^index/s/\([^0-9a-f]\)00*\.\./\10000000../ > /^index/s/\.\.00*$/..0000000/ > /^index/s/\.\.00*\([^0-9a-f]\)/..0000000\1/ > > after the above two patterns help? Yeah, something like that though matching the beginning and end of the line for the beginning and end of the hashes wont work. I'll reroll with something similar Thanks Phillip ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v3 4/9] t3701: don't hard code sha1 hash values 2018-02-28 11:03 ` Phillip Wood @ 2018-02-28 11:10 ` Phillip Wood 0 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-02-28 11:10 UTC (permalink / raw) To: Junio C Hamano Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood On 28/02/18 11:03, Phillip Wood wrote: > On 27/02/18 22:42, Junio C Hamano wrote: >> Phillip Wood <phillip.wood@talktalk.net> writes: >> >>> t/t3701-add-interactive.sh | 30 ++++++++++++++++++++---------- >>> 1 file changed, 20 insertions(+), 10 deletions(-) >>> >>> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh >>> index bdd1f292a9..46d655038f 100755 >>> --- a/t/t3701-add-interactive.sh >>> +++ b/t/t3701-add-interactive.sh >>> @@ -10,6 +10,16 @@ then >>> test_done >>> fi >>> >>> +diff_cmp () { >>> + for x >>> + do >>> + sed -e '/^index/s/[0-9a-f]*[1-9a-f][0-9a-f]*\.\./1234567../' \ >>> + -e '/^index/s/\.\.[0-9a-f]*[1-9a-f][0-9a-f]*/..9abcdef/' \ >>> + "$x" >"$x.filtered" >> >> Interesting ;-) You require .. and on the left hand side you want >> to see a run of hexdec with at least one non-zero hexdigit, which is >> filtered to fixed-length 1234567; right hand side is the same deal. >> >> Which sounds like a reasonable way to future-proof the comparison. >> >> If 7 zeros are expected in the result, and the actual output had 8 >> zeros, the filter does not touch either so they compare differently, >> which is somewhat unfortunate. Perhaps something like > > Ah, good point > >> /^index/s/^00*\.\./0000000../ >> /^index/s/\([^0-9a-f]\)00*\.\./\10000000../ >> /^index/s/\.\.00*$/..0000000/ >> /^index/s/\.\.00*\([^0-9a-f]\)/..0000000\1/ >> >> after the above two patterns help? > > Yeah, something like that though matching the beginning and end of the > line for the beginning and end of the hashes wont work. I'll reroll with > something similar Thinking about it some more, just using your last three patterns should do the job. - I'll test and reroll. > Thanks > > Phillip > ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v3 5/9] t3701: add failing test for pathological context lines 2018-02-27 11:03 ` [PATCH v3 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (3 preceding siblings ...) 2018-02-27 11:03 ` [PATCH v3 4/9] t3701: don't hard code sha1 hash values Phillip Wood @ 2018-02-27 11:04 ` Phillip Wood 2018-02-27 11:04 ` [PATCH v3 6/9] add -p: Adjust offsets of subsequent hunks when one is skipped Phillip Wood ` (3 subsequent siblings) 8 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-02-27 11:04 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, 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> --- Notes: changes since v2: - removed test_set_editor as it is already set changes since v1: - changed edit test to use the existing fake editor and to strip the hunk header and some context lines as well as deleting an insertion - style fixes 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 46d655038f..da73fbdf4d 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -528,4 +528,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 && + # check editing can cope with missing header and deleted context lines + # as well as changes to other lines + test_write_lines +b " a" >patch +' + +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 && + # n q q below is in case edit fails + printf "%s\n" e y n q q | + git add -p && + git cat-file blob :a >actual && + test_cmp expected-2 actual +' + test_done -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v3 6/9] add -p: Adjust offsets of subsequent hunks when one is skipped 2018-02-27 11:03 ` [PATCH v3 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (4 preceding siblings ...) 2018-02-27 11:04 ` [PATCH v3 5/9] t3701: add failing test for pathological context lines Phillip Wood @ 2018-02-27 11:04 ` Phillip Wood 2018-02-27 11:04 ` [PATCH v3 7/9] add -p: calculate offset delta for edited patches Phillip Wood ` (2 subsequent siblings) 8 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-02-27 11:04 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, 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 8ababa6453..7a0a5896bb 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 da73fbdf4d..4c30aa6b6e 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -541,7 +541,7 @@ test_expect_success 'set up pathological context' ' test_write_lines +b " a" >patch ' -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 related [flat|nested] 79+ messages in thread
* [PATCH v3 7/9] add -p: calculate offset delta for edited patches 2018-02-27 11:03 ` [PATCH v3 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (5 preceding siblings ...) 2018-02-27 11:04 ` [PATCH v3 6/9] add -p: Adjust offsets of subsequent hunks when one is skipped Phillip Wood @ 2018-02-27 11:04 ` Phillip Wood 2018-02-27 11:04 ` [PATCH v3 8/9] add -p: fix counting when splitting and coalescing Phillip Wood 2018-02-27 11:04 ` [PATCH v3 9/9] add -p: don't rely on apply's '--recount' option Phillip Wood 8 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-02-27 11:04 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, 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> --- Notes: changes since v1 - edited hunks are now recounted before seeing if they apply in preparation for removing '--recount' when invoking 'git apply' - added sentence to commit message about the offset data being lost if an edited hunk is split git-add--interactive.perl | 55 ++++++++++++++++++++++++++++++++++++++-------- t/t3701-add-interactive.sh | 2 +- 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 7a0a5896bb..0df0c2aa06 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -938,13 +938,19 @@ 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) { $n_ofs += $ofs_delta; $_->{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,32 @@ 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, }; + $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}; 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->{DISPLAY} = [color_diff(@{$newtext})]; return $newhunk; } else { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 4c30aa6b6e..83f4c8d6ea 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -549,7 +549,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 && # n q q below is in case edit fails printf "%s\n" e y n q q | -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v3 8/9] add -p: fix counting when splitting and coalescing 2018-02-27 11:03 ` [PATCH v3 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (6 preceding siblings ...) 2018-02-27 11:04 ` [PATCH v3 7/9] add -p: calculate offset delta for edited patches Phillip Wood @ 2018-02-27 11:04 ` Phillip Wood 2018-02-27 11:04 ` [PATCH v3 9/9] add -p: don't rely on apply's '--recount' option Phillip Wood 8 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-02-27 11:04 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> When a file has no trailing new line at the end diff records this by appending "\ No newline at end of file" below the last line of the file. This line should not be counted in the hunk header. Fix the splitting and coalescing code to count files without a trailing new line properly and change one of the tests to test splitting without a trailing new line. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- git-add--interactive.perl | 13 +++++++++---- t/t3701-add-interactive.sh | 31 +++++++++++++++++++++++-------- 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 0df0c2aa06..3226c2c4f0 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -793,6 +793,11 @@ sub split_hunk { while (++$i < @$text) { my $line = $text->[$i]; my $display = $display->[$i]; + if ($line =~ /^\\/) { + push @{$this->{TEXT}}, $line; + push @{$this->{DISPLAY}}, $display; + next; + } if ($line =~ /^ /) { if ($this->{ADDDEL} && !defined $next_hunk_start) { @@ -887,8 +892,8 @@ sub merge_hunk { $o_cnt = $n_cnt = 0; for ($i = 1; $i < @{$prev->{TEXT}}; $i++) { my $line = $prev->{TEXT}[$i]; - if ($line =~ /^\+/) { - $n_cnt++; + if ($line =~ /^[+\\]/) { + $n_cnt++ if ($line =~ /^\+/); push @line, $line; next; } @@ -905,8 +910,8 @@ sub merge_hunk { for ($i = 1; $i < @{$this->{TEXT}}; $i++) { my $line = $this->{TEXT}[$i]; - if ($line =~ /^\+/) { - $n_cnt++; + if ($line =~ /^[+\\]/) { + $n_cnt++ if ($line =~ /^\+/); push @line, $line; next; } diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 83f4c8d6ea..05540ee9ef 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -234,31 +234,46 @@ test_expect_success 'setup patch' ' baseline content +lastline + \ No newline at end of file EOF ' -# Expected output, similar to the patch but w/ diff at the top +# Expected output, diff is similar to the patch but w/ diff at the top test_expect_success 'setup expected' ' - cat >expected <<-EOF - diff --git a/file b/file - index b6f2c08..61b9053 100755 + echo diff --git a/file b/file >expected && + cat patch |sed "/^index/s/ 100644/ 100755/" >>expected && + cat >expected-output <<-EOF --- a/file +++ b/file @@ -1,2 +1,4 @@ +firstline baseline content +lastline + \ No newline at end of file + @@ -1,2 +1,3 @@ + +firstline + baseline + content + @@ -1,2 +2,3 @@ + baseline + content + +lastline + \ No newline at end of file EOF ' # Test splitting the first patch, then adding both -test_expect_success 'add first line works' ' +test_expect_success C_LOCALE_OUTPUT 'add first line works' ' git commit -am "clear local changes" && git apply patch && - (echo s; echo y; echo y) | git add -p file && - git diff --cached > diff && - diff_cmp expected diff + printf "%s\n" s y y | git add -p file 2>error | + sed -n -e "s/^Stage this hunk[^@]*\(@@ .*\)/\1/" \ + -e "/^[-+@ \\\\]"/p >output && + test_must_be_empty error && + git diff --cached >diff && + diff_cmp expected diff && + test_cmp expected-output output ' test_expect_success 'setup expected' ' -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v3 9/9] add -p: don't rely on apply's '--recount' option 2018-02-27 11:03 ` [PATCH v3 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (7 preceding siblings ...) 2018-02-27 11:04 ` [PATCH v3 8/9] add -p: fix counting when splitting and coalescing Phillip Wood @ 2018-02-27 11:04 ` Phillip Wood 8 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-02-27 11:04 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Now that add -p counts patches properly it should be possible to turn off the '--recount' option when invoking 'git apply' Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Notes: I can't think of a reason why this shouldn't be OK but I can't help feeling slightly nervous about it. I've made it a separate patch so it can be easily dropped or reverted if I've missed something. git-add--interactive.perl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 3226c2c4f0..a64c0db57d 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -677,7 +677,7 @@ sub add_untracked_cmd { sub run_git_apply { my $cmd = shift; my $fh; - open $fh, '| git ' . $cmd . " --recount --allow-overlap"; + open $fh, '| git ' . $cmd . " --allow-overlap"; print $fh @_; return close $fh; } -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v4 0/9] 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 ` (6 preceding siblings ...) 2018-02-27 11:03 ` [PATCH v3 0/9] Correct offsets of hunks when one is skipped Phillip Wood @ 2018-03-01 10:50 ` Phillip Wood 2018-03-01 10:50 ` [PATCH v4 1/9] add -i: add function to format hunk header Phillip Wood ` (8 more replies) 2018-03-05 10:56 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Phillip Wood 8 siblings, 9 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-01 10:50 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> I've fixed the second patch to match what was in pu and added some extra code to patch 4 to handle zero sha1 hashes where the length varies. Cover letter to v1: 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. Interdiff to v3: diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 05540ee9ef..a9a9478a29 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -15,6 +15,9 @@ diff_cmp () { do sed -e '/^index/s/[0-9a-f]*[1-9a-f][0-9a-f]*\.\./1234567../' \ -e '/^index/s/\.\.[0-9a-f]*[1-9a-f][0-9a-f]*/..9abcdef/' \ + -e '/^index/s/ 00*\.\./ 0000000../' \ + -e '/^index/s/\.\.00*$/..0000000/' \ + -e '/^index/s/\.\.00* /..0000000 /' \ "$x" >"$x.filtered" done test_cmp "$1.filtered" "$2.filtered" @@ -32,7 +35,7 @@ test_expect_success 'status works (initial)' ' ' test_expect_success 'setup expected' ' - cat >expected <<-EOF + cat >expected <<-\EOF new file mode 100644 index 0000000..d95f3ad --- /dev/null @@ -69,7 +72,7 @@ test_expect_success 'status works (commit)' ' ' test_expect_success 'setup expected' ' - cat >expected <<-EOF + cat >expected <<-\EOF index 180b47c..b6f2c08 100644 --- a/file +++ b/file @@ -93,7 +96,7 @@ test_expect_success 'revert works (commit)' ' test_expect_success 'setup expected' ' - cat >expected <<-EOF + cat >expected <<-\EOF EOF ' @@ -105,7 +108,7 @@ test_expect_success 'dummy edit works' ' ' test_expect_success 'setup patch' ' - cat >patch <<-EOF + cat >patch <<-\EOF @@ -1,1 +1,4 @@ this +patch @@ -129,7 +132,7 @@ test_expect_success 'bad edit rejected' ' ' test_expect_success 'setup patch' ' - cat >patch <<-EOF + cat >patch <<-\EOF this patch is garbage EOF @@ -142,7 +145,7 @@ test_expect_success 'garbage edit rejected' ' ' test_expect_success 'setup patch' ' - cat >patch <<-EOF + cat >patch <<-\EOF @@ -1,0 +1,0 @@ baseline +content @@ -152,7 +155,7 @@ test_expect_success 'setup patch' ' ' test_expect_success 'setup expected' ' - cat >expected <<-EOF + cat >expected <<-\EOF diff --git a/file b/file index b5dd6c9..f910ae9 100644 --- a/file @@ -225,7 +228,7 @@ test_expect_success 'setup again' ' # Write the patch file with a new line at the top and bottom test_expect_success 'setup patch' ' - cat >patch <<-EOF + cat >patch <<-\EOF index 180b47c..b6f2c08 100644 --- a/file +++ b/file @@ -242,7 +245,7 @@ test_expect_success 'setup patch' ' test_expect_success 'setup expected' ' echo diff --git a/file b/file >expected && cat patch |sed "/^index/s/ 100644/ 100755/" >>expected && - cat >expected-output <<-EOF + cat >expected-output <<-\EOF --- a/file +++ b/file @@ -1,2 +1,4 @@ @@ -277,7 +280,7 @@ test_expect_success C_LOCALE_OUTPUT 'add first line works' ' ' test_expect_success 'setup expected' ' - cat >expected <<-EOF + cat >expected <<-\EOF diff --git a/non-empty b/non-empty deleted file mode 100644 index d95f3ad..0000000 @@ -300,7 +303,7 @@ test_expect_success 'deleting a non-empty file' ' ' test_expect_success 'setup expected' ' - cat >expected <<-EOF + cat >expected <<-\EOF diff --git a/empty b/empty deleted file mode 100644 index e69de29..0000000 Phillip Wood (9): add -i: add function to format hunk header t3701: indent here documents t3701: use test_write_lines and write_script t3701: don't hard code sha1 hash values 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 add -p: fix counting when splitting and coalescing add -p: don't rely on apply's '--recount' option git-add--interactive.perl | 106 +++++++++++++---- t/t3701-add-interactive.sh | 291 +++++++++++++++++++++++++-------------------- 2 files changed, 243 insertions(+), 154 deletions(-) -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v4 1/9] add -i: add function to format hunk header 2018-03-01 10:50 ` [PATCH v4 0/9] Correct offsets of hunks when one is skipped Phillip Wood @ 2018-03-01 10:50 ` Phillip Wood 2018-03-01 10:50 ` [PATCH v4 2/9] t3701: indent here documents Phillip Wood ` (7 subsequent siblings) 8 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-01 10:50 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, 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 964c3a7542..8ababa6453 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 related [flat|nested] 79+ messages in thread
* [PATCH v4 2/9] t3701: indent here documents 2018-03-01 10:50 ` [PATCH v4 0/9] Correct offsets of hunks when one is skipped Phillip Wood 2018-03-01 10:50 ` [PATCH v4 1/9] add -i: add function to format hunk header Phillip Wood @ 2018-03-01 10:50 ` Phillip Wood 2018-03-01 10:50 ` [PATCH v4 3/9] t3701: use test_write_lines and write_script Phillip Wood ` (6 subsequent siblings) 8 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-01 10:50 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Indent here documents in line with the current style for tests. While at it, quote the end marker of here-docs that do not use variable interpolation. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Notes: changes since v3: - updated to match what was in pu t/t3701-add-interactive.sh | 174 ++++++++++++++++++++++----------------------- 1 file changed, 87 insertions(+), 87 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 058698df6a..3130dafcf0 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -22,14 +22,14 @@ test_expect_success 'status works (initial)' ' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -new file mode 100644 -index 0000000..d95f3ad ---- /dev/null -+++ b/file -@@ -0,0 +1 @@ -+content -EOF + cat >expected <<-\EOF + new file mode 100644 + index 0000000..d95f3ad + --- /dev/null + +++ b/file + @@ -0,0 +1 @@ + +content + EOF ' test_expect_success 'diff works (initial)' ' @@ -59,14 +59,14 @@ test_expect_success 'status works (commit)' ' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -index 180b47c..b6f2c08 100644 ---- a/file -+++ b/file -@@ -1 +1,2 @@ - baseline -+content -EOF + cat >expected <<-\EOF + index 180b47c..b6f2c08 100644 + --- a/file + +++ b/file + @@ -1 +1,2 @@ + baseline + +content + EOF ' test_expect_success 'diff works (commit)' ' @@ -83,8 +83,8 @@ test_expect_success 'revert works (commit)' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -EOF + cat >expected <<-\EOF + EOF ' test_expect_success 'setup fake editor' ' @@ -100,21 +100,21 @@ test_expect_success 'dummy edit works' ' ' test_expect_success 'setup patch' ' -cat >patch <<EOF -@@ -1,1 +1,4 @@ - this -+patch --does not - apply -EOF + cat >patch <<-\EOF + @@ -1,1 +1,4 @@ + this + +patch + -does not + apply + EOF ' test_expect_success 'setup fake editor' ' echo "#!$SHELL_PATH" >fake_editor.sh && - cat >>fake_editor.sh <<\EOF && -mv -f "$1" oldpatch && -mv -f patch "$1" -EOF + cat >>fake_editor.sh <<-\EOF && + mv -f "$1" oldpatch && + mv -f patch "$1" + EOF chmod a+x fake_editor.sh && test_set_editor "$(pwd)/fake_editor.sh" ' @@ -126,10 +126,10 @@ test_expect_success 'bad edit rejected' ' ' test_expect_success 'setup patch' ' -cat >patch <<EOF -this patch -is garbage -EOF + cat >patch <<-\EOF + this patch + is garbage + EOF ' test_expect_success 'garbage edit rejected' ' @@ -139,28 +139,28 @@ test_expect_success 'garbage edit rejected' ' ' test_expect_success 'setup patch' ' -cat >patch <<EOF -@@ -1,0 +1,0 @@ - baseline -+content -+newcontent -+lines -EOF + cat >patch <<-\EOF + @@ -1,0 +1,0 @@ + baseline + +content + +newcontent + +lines + EOF ' test_expect_success 'setup expected' ' -cat >expected <<EOF -diff --git a/file b/file -index b5dd6c9..f910ae9 100644 ---- a/file -+++ b/file -@@ -1,4 +1,4 @@ - baseline - content --newcontent -+more - lines -EOF + cat >expected <<-\EOF + diff --git a/file b/file + index b5dd6c9..f910ae9 100644 + --- a/file + +++ b/file + @@ -1,4 +1,4 @@ + baseline + content + -newcontent + +more + lines + EOF ' test_expect_success 'real edit works' ' @@ -222,31 +222,31 @@ test_expect_success 'setup again' ' # Write the patch file with a new line at the top and bottom test_expect_success 'setup patch' ' -cat >patch <<EOF -index 180b47c..b6f2c08 100644 ---- a/file -+++ b/file -@@ -1,2 +1,4 @@ -+firstline - baseline - content -+lastline -EOF + cat >patch <<-\EOF + index 180b47c..b6f2c08 100644 + --- a/file + +++ b/file + @@ -1,2 +1,4 @@ + +firstline + baseline + content + +lastline + EOF ' # Expected output, similar to the patch but w/ diff at the top test_expect_success 'setup expected' ' -cat >expected <<EOF -diff --git a/file b/file -index b6f2c08..61b9053 100755 ---- a/file -+++ b/file -@@ -1,2 +1,4 @@ -+firstline - baseline - content -+lastline -EOF + cat >expected <<-\EOF + diff --git a/file b/file + index b6f2c08..61b9053 100755 + --- a/file + +++ b/file + @@ -1,2 +1,4 @@ + +firstline + baseline + content + +lastline + EOF ' # Test splitting the first patch, then adding both @@ -259,15 +259,15 @@ test_expect_success 'add first line works' ' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -diff --git a/non-empty b/non-empty -deleted file mode 100644 -index d95f3ad..0000000 ---- a/non-empty -+++ /dev/null -@@ -1 +0,0 @@ --content -EOF + cat >expected <<-\EOF + diff --git a/non-empty b/non-empty + deleted file mode 100644 + index d95f3ad..0000000 + --- a/non-empty + +++ /dev/null + @@ -1 +0,0 @@ + -content + EOF ' test_expect_success 'deleting a non-empty file' ' @@ -282,11 +282,11 @@ test_expect_success 'deleting a non-empty file' ' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -diff --git a/empty b/empty -deleted file mode 100644 -index e69de29..0000000 -EOF + cat >expected <<-\EOF + diff --git a/empty b/empty + deleted file mode 100644 + index e69de29..0000000 + EOF ' test_expect_success 'deleting an empty file' ' -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v4 3/9] t3701: use test_write_lines and write_script 2018-03-01 10:50 ` [PATCH v4 0/9] Correct offsets of hunks when one is skipped Phillip Wood 2018-03-01 10:50 ` [PATCH v4 1/9] add -i: add function to format hunk header Phillip Wood 2018-03-01 10:50 ` [PATCH v4 2/9] t3701: indent here documents Phillip Wood @ 2018-03-01 10:50 ` Phillip Wood 2018-03-01 10:50 ` [PATCH v4 4/9] t3701: don't hard code sha1 hash values Phillip Wood ` (5 subsequent siblings) 8 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-01 10:50 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Simplify things slightly by using the above helpers. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Notes: changes since v2 - fixed use of test_set_editor to match what was in pu t/t3701-add-interactive.sh | 33 +++++---------------------------- 1 file changed, 5 insertions(+), 28 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 3130dafcf0..836ce346ed 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -87,13 +87,8 @@ test_expect_success 'setup expected' ' EOF ' -test_expect_success 'setup fake editor' ' - >fake_editor.sh && - chmod a+x fake_editor.sh && - test_set_editor "$(pwd)/fake_editor.sh" -' - test_expect_success 'dummy edit works' ' + test_set_editor : && (echo e; echo a) | git add -p && git diff > diff && test_cmp expected diff @@ -110,12 +105,10 @@ test_expect_success 'setup patch' ' ' test_expect_success 'setup fake editor' ' - echo "#!$SHELL_PATH" >fake_editor.sh && - cat >>fake_editor.sh <<-\EOF && + write_script "fake_editor.sh" <<-\EOF && mv -f "$1" oldpatch && mv -f patch "$1" EOF - chmod a+x fake_editor.sh && test_set_editor "$(pwd)/fake_editor.sh" ' @@ -302,18 +295,12 @@ test_expect_success 'deleting an empty file' ' test_expect_success 'split hunk setup' ' git reset --hard && - for i in 10 20 30 40 50 60 - do - echo $i - done >test && + test_write_lines 10 20 30 40 50 60 >test && git add test && test_tick && git commit -m test && - for i in 10 15 20 21 22 23 24 30 40 50 60 - do - echo $i - done >test + test_write_lines 10 15 20 21 22 23 24 30 40 50 60 >test ' test_expect_success 'split hunk "add -p (edit)"' ' @@ -334,17 +321,7 @@ test_expect_success 'split hunk "add -p (edit)"' ' ' test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' - cat >test <<-\EOF && - 5 - 10 - 20 - 21 - 30 - 31 - 40 - 50 - 60 - EOF + test_write_lines 5 10 20 21 30 31 40 50 60 >test && git reset && # test sequence is s(plit), n(o), y(es), e(dit) # q n q q is there to make sure we exit at the end. -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v4 4/9] t3701: don't hard code sha1 hash values 2018-03-01 10:50 ` [PATCH v4 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (2 preceding siblings ...) 2018-03-01 10:50 ` [PATCH v4 3/9] t3701: use test_write_lines and write_script Phillip Wood @ 2018-03-01 10:50 ` Phillip Wood 2018-03-02 15:55 ` SZEDER Gábor 2018-03-01 10:50 ` [PATCH v4 5/9] t3701: add failing test for pathological context lines Phillip Wood ` (4 subsequent siblings) 8 siblings, 1 reply; 79+ messages in thread From: Phillip Wood @ 2018-03-01 10:50 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Use a filter when comparing diffs to fix the value of non-zero hashes in diff index lines so we're not hard coding sha1 hash values in the expected output. This makes it easier to change the expected output if a test is edited as we don't need to worry about the exact hash value and means the tests will work when the hash algorithm is transitioned away from sha1. Thanks-to: Junio C Hamano <gitster@pobox.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Notes: changes since v3: - fix zero hash values to seven digits changes since v2: - fix hash values in index lines rather than removing the line - reworded commit message t/t3701-add-interactive.sh | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 836ce346ed..f95714230b 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -10,6 +10,19 @@ then test_done fi +diff_cmp () { + for x + do + sed -e '/^index/s/[0-9a-f]*[1-9a-f][0-9a-f]*\.\./1234567../' \ + -e '/^index/s/\.\.[0-9a-f]*[1-9a-f][0-9a-f]*/..9abcdef/' \ + -e '/^index/s/ 00*\.\./ 0000000../' \ + -e '/^index/s/\.\.00*$/..0000000/' \ + -e '/^index/s/\.\.00* /..0000000 /' \ + "$x" >"$x.filtered" + done + test_cmp "$1.filtered" "$2.filtered" +} + test_expect_success 'setup (initial)' ' echo content >file && git add file && @@ -35,7 +48,7 @@ test_expect_success 'setup expected' ' test_expect_success 'diff works (initial)' ' (echo d; echo 1) | git add -i >output && sed -ne "/new file/,/content/p" <output >diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'revert works (initial)' ' git add file && @@ -72,7 +85,7 @@ test_expect_success 'setup expected' ' test_expect_success 'diff works (commit)' ' (echo d; echo 1) | git add -i >output && sed -ne "/^index/,/content/p" <output >diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'revert works (commit)' ' git add file && @@ -91,7 +104,7 @@ test_expect_success 'dummy edit works' ' test_set_editor : && (echo e; echo a) | git add -p && git diff > diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'setup patch' ' @@ -159,7 +172,7 @@ test_expect_success 'setup expected' ' test_expect_success 'real edit works' ' (echo e; echo n; echo d) | git add -p && git diff >output && - test_cmp expected output + diff_cmp expected output ' test_expect_success 'skip files similarly as commit -a' ' @@ -171,7 +184,7 @@ test_expect_success 'skip files similarly as commit -a' ' git reset && git commit -am commit && git diff >expected && - test_cmp expected output && + diff_cmp expected output && git reset --hard HEAD^ ' rm -f .gitignore @@ -248,7 +261,7 @@ test_expect_success 'add first line works' ' git apply patch && (echo s; echo y; echo y) | git add -p file && git diff --cached > diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'setup expected' ' @@ -271,7 +284,7 @@ test_expect_success 'deleting a non-empty file' ' rm non-empty && echo y | git add -p non-empty && git diff --cached >diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'setup expected' ' @@ -290,7 +303,7 @@ test_expect_success 'deleting an empty file' ' rm empty && echo y | git add -p empty && git diff --cached >diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'split hunk setup' ' @@ -355,7 +368,7 @@ test_expect_success 'patch mode ignores unmerged entries' ' +changed EOF git diff --cached >diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success TTY 'diffs can be colorized' ' @@ -384,7 +397,7 @@ test_expect_success 'patch-mode via -i prompts for files' ' echo test >expect && git diff --cached --name-only >actual && - test_cmp expect actual + diff_cmp expect actual ' test_expect_success 'add -p handles globs' ' -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v4 4/9] t3701: don't hard code sha1 hash values 2018-03-01 10:50 ` [PATCH v4 4/9] t3701: don't hard code sha1 hash values Phillip Wood @ 2018-03-02 15:55 ` SZEDER Gábor 2018-03-02 16:09 ` Junio C Hamano 0 siblings, 1 reply; 79+ messages in thread From: SZEDER Gábor @ 2018-03-02 15:55 UTC (permalink / raw) To: Phillip Wood Cc: SZEDER Gábor, Git Mailing List, Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood > Use a filter when comparing diffs to fix the value of non-zero hashes > in diff index lines so we're not hard coding sha1 hash values in the > expected output. This makes it easier to change the expected output if > a test is edited as we don't need to worry about the exact hash value > and means the tests will work when the hash algorithm is transitioned > away from sha1. > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > index 836ce346ed..f95714230b 100755 > --- a/t/t3701-add-interactive.sh > +++ b/t/t3701-add-interactive.sh > @@ -10,6 +10,19 @@ then > test_done > fi > > +diff_cmp () { > + for x > + do > + sed -e '/^index/s/[0-9a-f]*[1-9a-f][0-9a-f]*\.\./1234567../' \ > + -e '/^index/s/\.\.[0-9a-f]*[1-9a-f][0-9a-f]*/..9abcdef/' \ > + -e '/^index/s/ 00*\.\./ 0000000../' \ > + -e '/^index/s/\.\.00*$/..0000000/' \ > + -e '/^index/s/\.\.00* /..0000000 /' \ > + "$x" >"$x.filtered" > + done > + test_cmp "$1.filtered" "$2.filtered" > +} t3701 is not the only test script comparing diffs, many other tests do so: $ git grep 'diff --git' t/ |wc -l 835 It's totally inaccurate, but a good ballpark figure. I think this function should be a widely available test helper function in 'test-lib-functions.sh', perhaps renamed to 'test_cmp_diff', so those other tests could use it as well. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v4 4/9] t3701: don't hard code sha1 hash values 2018-03-02 15:55 ` SZEDER Gábor @ 2018-03-02 16:09 ` Junio C Hamano 2018-03-05 10:59 ` Phillip Wood 0 siblings, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2018-03-02 16:09 UTC (permalink / raw) To: SZEDER Gábor Cc: Phillip Wood, Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood SZEDER Gábor <szeder.dev@gmail.com> writes: >> +diff_cmp () { >> + for x >> + do >> + sed -e '/^index/s/[0-9a-f]*[1-9a-f][0-9a-f]*\.\./1234567../' \ >> + -e '/^index/s/\.\.[0-9a-f]*[1-9a-f][0-9a-f]*/..9abcdef/' \ >> + -e '/^index/s/ 00*\.\./ 0000000../' \ >> + -e '/^index/s/\.\.00*$/..0000000/' \ >> + -e '/^index/s/\.\.00* /..0000000 /' \ >> + "$x" >"$x.filtered" >> + done >> + test_cmp "$1.filtered" "$2.filtered" >> +} > > t3701 is not the only test script comparing diffs, many other > tests do so: > > $ git grep 'diff --git' t/ |wc -l > 835 > > It's totally inaccurate, but a good ballpark figure. > > I think this function should be a widely available test helper > function in 'test-lib-functions.sh', perhaps renamed to > 'test_cmp_diff', so those other tests could use it as well. I am on the fence. The above does not redact enough (e.g. rename/copy score should be redacted while comparing) to serve as a generic filter. We could certainly extend it when we make code in other tests use the helper, but then we can do the moving to test-lib-functions when that happens, too. Those who will be writing new tests that need to compare two diff outputs are either (1) people who are careful and try to find existing tests that do the same, to locate the helper we already have to be reused in theirs, or (2) people who don't and roll their own. The latter camp cannot be helped either way, but the former will run "git grep 'diff --git' t/" and find the helper whether it is in this script or in test-lib-functions, I would think. So, I certainly do not mind a reroll to move it to a more generic place, but I do not think I would terribly mind if we leave it in its current place, later to be moved by the first new caller that wants to use it from outside this script. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v4 4/9] t3701: don't hard code sha1 hash values 2018-03-02 16:09 ` Junio C Hamano @ 2018-03-05 10:59 ` Phillip Wood 2018-03-05 12:32 ` SZEDER Gábor 0 siblings, 1 reply; 79+ messages in thread From: Phillip Wood @ 2018-03-05 10:59 UTC (permalink / raw) To: Junio C Hamano, SZEDER Gábor Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood On 02/03/18 16:09, Junio C Hamano wrote: > SZEDER Gábor <szeder.dev@gmail.com> writes: > >>> +diff_cmp () { >>> + for x >>> + do >>> + sed -e '/^index/s/[0-9a-f]*[1-9a-f][0-9a-f]*\.\./1234567../' \ >>> + -e '/^index/s/\.\.[0-9a-f]*[1-9a-f][0-9a-f]*/..9abcdef/' \ >>> + -e '/^index/s/ 00*\.\./ 0000000../' \ >>> + -e '/^index/s/\.\.00*$/..0000000/' \ >>> + -e '/^index/s/\.\.00* /..0000000 /' \ >>> + "$x" >"$x.filtered" >>> + done >>> + test_cmp "$1.filtered" "$2.filtered" >>> +} >> >> t3701 is not the only test script comparing diffs, many other >> tests do so: >> >> $ git grep 'diff --git' t/ |wc -l >> 835 >> >> It's totally inaccurate, but a good ballpark figure. >> >> I think this function should be a widely available test helper >> function in 'test-lib-functions.sh', perhaps renamed to >> 'test_cmp_diff', so those other tests could use it as well. > > I am on the fence. > > The above does not redact enough (e.g. rename/copy score should be > redacted while comparing) to serve as a generic filter. > > We could certainly extend it when we make code in other tests use > the helper, but then we can do the moving to test-lib-functions when > that happens, too. > > Those who will be writing new tests that need to compare two diff > outputs are either (1) people who are careful and try to find > existing tests that do the same, to locate the helper we already > have to be reused in theirs, or (2) people who don't and roll their > own. The latter camp cannot be helped either way, but the former > will run "git grep 'diff --git' t/" and find the helper whether it > is in this script or in test-lib-functions, I would think. > > So, I certainly do not mind a reroll to move it to a more generic > place, but I do not think I would terribly mind if we leave it in > its current place, later to be moved by the first new caller that > wants to use it from outside this script. > I did wonder about putting this function in a library when I first wrote it but decided to wait and see if it is useful instead. As Junio points out it would need to be improved to act as a generic filter, so I'll take the easy option and leave it where it is at the moment. Best Wishes Phillip ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v4 4/9] t3701: don't hard code sha1 hash values 2018-03-05 10:59 ` Phillip Wood @ 2018-03-05 12:32 ` SZEDER Gábor 0 siblings, 0 replies; 79+ messages in thread From: SZEDER Gábor @ 2018-03-05 12:32 UTC (permalink / raw) To: Phillip Wood Cc: Junio C Hamano, Git Mailing List, Brian M. Carlson, Eric Sunshine On Mon, Mar 5, 2018 at 11:59 AM, Phillip Wood <phillip.wood@talktalk.net> wrote: > I did wonder about putting this function in a library when I first wrote > it but decided to wait and see if it is useful instead. As Junio points > out it would need to be improved to act as a generic filter, so I'll > take the easy option and leave it where it is at the moment. Makes sense. I just pointed it out, because I could have used it already in its current form for a test I was working on last week. ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v4 5/9] t3701: add failing test for pathological context lines 2018-03-01 10:50 ` [PATCH v4 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (3 preceding siblings ...) 2018-03-01 10:50 ` [PATCH v4 4/9] t3701: don't hard code sha1 hash values Phillip Wood @ 2018-03-01 10:50 ` Phillip Wood 2018-03-01 10:51 ` [PATCH v4 6/9] add -p: adjust offsets of subsequent hunks when one is skipped Phillip Wood ` (3 subsequent siblings) 8 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-01 10:50 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, 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> --- Notes: changes since v2: - removed test_set_editor as it is already set changes since v1: - changed edit test to use the existing fake editor and to strip the hunk header and some context lines as well as deleting an insertion - style fixes 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 f95714230b..094eeca405 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -531,4 +531,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 && + # check editing can cope with missing header and deleted context lines + # as well as changes to other lines + test_write_lines +b " a" >patch +' + +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 && + # n q q below is in case edit fails + printf "%s\n" e y n q q | + git add -p && + git cat-file blob :a >actual && + test_cmp expected-2 actual +' + test_done -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v4 6/9] add -p: adjust offsets of subsequent hunks when one is skipped 2018-03-01 10:50 ` [PATCH v4 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (4 preceding siblings ...) 2018-03-01 10:50 ` [PATCH v4 5/9] t3701: add failing test for pathological context lines Phillip Wood @ 2018-03-01 10:51 ` Phillip Wood 2018-03-01 10:51 ` [PATCH v4 7/9] add -p: calculate offset delta for edited patches Phillip Wood ` (2 subsequent siblings) 8 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-01 10:51 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, 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 8ababa6453..7a0a5896bb 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 094eeca405..e3150a4e07 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -544,7 +544,7 @@ test_expect_success 'set up pathological context' ' test_write_lines +b " a" >patch ' -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 related [flat|nested] 79+ messages in thread
* [PATCH v4 7/9] add -p: calculate offset delta for edited patches 2018-03-01 10:50 ` [PATCH v4 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (5 preceding siblings ...) 2018-03-01 10:51 ` [PATCH v4 6/9] add -p: adjust offsets of subsequent hunks when one is skipped Phillip Wood @ 2018-03-01 10:51 ` Phillip Wood 2018-03-01 20:14 ` Junio C Hamano 2018-03-01 10:51 ` [PATCH v4 8/9] add -p: fix counting when splitting and coalescing Phillip Wood 2018-03-01 10:51 ` [PATCH v4 9/9] add -p: don't rely on apply's '--recount' option Phillip Wood 8 siblings, 1 reply; 79+ messages in thread From: Phillip Wood @ 2018-03-01 10:51 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, 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> --- Notes: changes since v1 - edited hunks are now recounted before seeing if they apply in preparation for removing '--recount' when invoking 'git apply' - added sentence to commit message about the offset data being lost if an edited hunk is split git-add--interactive.perl | 55 ++++++++++++++++++++++++++++++++++++++-------- t/t3701-add-interactive.sh | 2 +- 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 7a0a5896bb..0df0c2aa06 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -938,13 +938,19 @@ 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) { $n_ofs += $ofs_delta; $_->{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,32 @@ 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, }; + $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}; 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->{DISPLAY} = [color_diff(@{$newtext})]; return $newhunk; } else { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index e3150a4e07..4cc9517eda 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -552,7 +552,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 && # n q q below is in case edit fails printf "%s\n" e y n q q | -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v4 7/9] add -p: calculate offset delta for edited patches 2018-03-01 10:51 ` [PATCH v4 7/9] add -p: calculate offset delta for edited patches Phillip Wood @ 2018-03-01 20:14 ` Junio C Hamano 2018-03-02 10:36 ` Phillip Wood 0 siblings, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2018-03-01 20:14 UTC (permalink / raw) To: Phillip Wood Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > 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> > --- Thanks for clear description of what is going on in the series. > diff --git a/git-add--interactive.perl b/git-add--interactive.perl > index 7a0a5896bb..0df0c2aa06 100755 > --- a/git-add--interactive.perl > +++ b/git-add--interactive.perl > @@ -938,13 +938,19 @@ 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}; The pattern <<conditional>> and <<statement with side effect>>; is something you are newly introducing to this script. I am not sure if we want to see them. I somehow find them harder to read than the more straight-forward and naïve if (<<conditional>>) { <<statement with side effect>>; } > + # If this hunk was edited then adjust the offset delta > + # to reflect the edit. > + $_->{OFS_DELTA} and $ofs_delta += $_->{OFS_DELTA}; Likewise. > +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; > +} OK. > @@ -1114,25 +1144,32 @@ 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}; > ... > + $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}; Ahh, good point. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v4 7/9] add -p: calculate offset delta for edited patches 2018-03-01 20:14 ` Junio C Hamano @ 2018-03-02 10:36 ` Phillip Wood 0 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-02 10:36 UTC (permalink / raw) To: Junio C Hamano Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood On 01/03/18 20:14, Junio C Hamano wrote: > Phillip Wood <phillip.wood@talktalk.net> writes: > >> 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> >> --- > > Thanks for clear description of what is going on in the series. > >> diff --git a/git-add--interactive.perl b/git-add--interactive.perl >> index 7a0a5896bb..0df0c2aa06 100755 >> --- a/git-add--interactive.perl >> +++ b/git-add--interactive.perl >> @@ -938,13 +938,19 @@ 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}; > > The pattern > > <<conditional>> and <<statement with side effect>>; > > is something you are newly introducing to this script. I am not > sure if we want to see them. I somehow find them harder to read > than the more straight-forward and naïve > > if (<<conditional>>) { > <<statement with side effect>>; > } > Fair enough, I think I was suffering from brace fatigue when I wrote it, if you can hold off merging this into next I'll re-roll with "if's" instead of "and's". >> + # If this hunk was edited then adjust the offset delta >> + # to reflect the edit. >> + $_->{OFS_DELTA} and $ofs_delta += $_->{OFS_DELTA}; > > Likewise. > >> +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; >> +} > > OK. > >> @@ -1114,25 +1144,32 @@ 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}; >> ... >> + $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}; > > Ahh, good point. > ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v4 8/9] add -p: fix counting when splitting and coalescing 2018-03-01 10:50 ` [PATCH v4 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (6 preceding siblings ...) 2018-03-01 10:51 ` [PATCH v4 7/9] add -p: calculate offset delta for edited patches Phillip Wood @ 2018-03-01 10:51 ` Phillip Wood 2018-03-01 20:29 ` Junio C Hamano 2018-03-01 10:51 ` [PATCH v4 9/9] add -p: don't rely on apply's '--recount' option Phillip Wood 8 siblings, 1 reply; 79+ messages in thread From: Phillip Wood @ 2018-03-01 10:51 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> When a file has no trailing new line at the end diff records this by appending "\ No newline at end of file" below the last line of the file. This line should not be counted in the hunk header. Fix the splitting and coalescing code to count files without a trailing new line properly and change one of the tests to test splitting without a trailing new line. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- git-add--interactive.perl | 13 +++++++++---- t/t3701-add-interactive.sh | 31 +++++++++++++++++++++++-------- 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 0df0c2aa06..3226c2c4f0 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -793,6 +793,11 @@ sub split_hunk { while (++$i < @$text) { my $line = $text->[$i]; my $display = $display->[$i]; + if ($line =~ /^\\/) { + push @{$this->{TEXT}}, $line; + push @{$this->{DISPLAY}}, $display; + next; + } if ($line =~ /^ /) { if ($this->{ADDDEL} && !defined $next_hunk_start) { @@ -887,8 +892,8 @@ sub merge_hunk { $o_cnt = $n_cnt = 0; for ($i = 1; $i < @{$prev->{TEXT}}; $i++) { my $line = $prev->{TEXT}[$i]; - if ($line =~ /^\+/) { - $n_cnt++; + if ($line =~ /^[+\\]/) { + $n_cnt++ if ($line =~ /^\+/); push @line, $line; next; } @@ -905,8 +910,8 @@ sub merge_hunk { for ($i = 1; $i < @{$this->{TEXT}}; $i++) { my $line = $this->{TEXT}[$i]; - if ($line =~ /^\+/) { - $n_cnt++; + if ($line =~ /^[+\\]/) { + $n_cnt++ if ($line =~ /^\+/); push @line, $line; next; } diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 4cc9517eda..a9a9478a29 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -237,31 +237,46 @@ test_expect_success 'setup patch' ' baseline content +lastline + \ No newline at end of file EOF ' -# Expected output, similar to the patch but w/ diff at the top +# Expected output, diff is similar to the patch but w/ diff at the top test_expect_success 'setup expected' ' - cat >expected <<-\EOF - diff --git a/file b/file - index b6f2c08..61b9053 100755 + echo diff --git a/file b/file >expected && + cat patch |sed "/^index/s/ 100644/ 100755/" >>expected && + cat >expected-output <<-\EOF --- a/file +++ b/file @@ -1,2 +1,4 @@ +firstline baseline content +lastline + \ No newline at end of file + @@ -1,2 +1,3 @@ + +firstline + baseline + content + @@ -1,2 +2,3 @@ + baseline + content + +lastline + \ No newline at end of file EOF ' # Test splitting the first patch, then adding both -test_expect_success 'add first line works' ' +test_expect_success C_LOCALE_OUTPUT 'add first line works' ' git commit -am "clear local changes" && git apply patch && - (echo s; echo y; echo y) | git add -p file && - git diff --cached > diff && - diff_cmp expected diff + printf "%s\n" s y y | git add -p file 2>error | + sed -n -e "s/^Stage this hunk[^@]*\(@@ .*\)/\1/" \ + -e "/^[-+@ \\\\]"/p >output && + test_must_be_empty error && + git diff --cached >diff && + diff_cmp expected diff && + test_cmp expected-output output ' test_expect_success 'setup expected' ' -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v4 8/9] add -p: fix counting when splitting and coalescing 2018-03-01 10:51 ` [PATCH v4 8/9] add -p: fix counting when splitting and coalescing Phillip Wood @ 2018-03-01 20:29 ` Junio C Hamano 2018-03-02 10:48 ` Phillip Wood 0 siblings, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2018-03-01 20:29 UTC (permalink / raw) To: Phillip Wood Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > @@ -887,8 +892,8 @@ sub merge_hunk { > $o_cnt = $n_cnt = 0; > for ($i = 1; $i < @{$prev->{TEXT}}; $i++) { > my $line = $prev->{TEXT}[$i]; > - if ($line =~ /^\+/) { > - $n_cnt++; > + if ($line =~ /^[+\\]/) { > + $n_cnt++ if ($line =~ /^\+/); > push @line, $line; > next; > } Hmmmm, the logic may be correct, but this looks like a result of attempting to minimize the number of changed lines and ending up with a less-than-readble code. "If the line begins with a plus or backslash, do these things, the first of which is done only when the line begins with a plus." The same comment for the other hunk that counts the $this side. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v4 8/9] add -p: fix counting when splitting and coalescing 2018-03-01 20:29 ` Junio C Hamano @ 2018-03-02 10:48 ` Phillip Wood 0 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-02 10:48 UTC (permalink / raw) To: Junio C Hamano Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood On 01/03/18 20:29, Junio C Hamano wrote: > Phillip Wood <phillip.wood@talktalk.net> writes: > >> @@ -887,8 +892,8 @@ sub merge_hunk { >> $o_cnt = $n_cnt = 0; >> for ($i = 1; $i < @{$prev->{TEXT}}; $i++) { >> my $line = $prev->{TEXT}[$i]; >> - if ($line =~ /^\+/) { >> - $n_cnt++; >> + if ($line =~ /^[+\\]/) { >> + $n_cnt++ if ($line =~ /^\+/); >> push @line, $line; >> next; >> } > > Hmmmm, the logic may be correct, but this looks like a result of > attempting to minimize the number of changed lines and ending up > with a less-than-readble code. "If the line begins with a plus or > backslash, do these things, the first of which is done only when > the line begins with a plus." The same comment for the other hunk > that counts the $this side. > Right, I'll re-roll with a separate clause for the "\ No new line .." lines. ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v4 9/9] add -p: don't rely on apply's '--recount' option 2018-03-01 10:50 ` [PATCH v4 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (7 preceding siblings ...) 2018-03-01 10:51 ` [PATCH v4 8/9] add -p: fix counting when splitting and coalescing Phillip Wood @ 2018-03-01 10:51 ` Phillip Wood 2018-03-01 20:30 ` Junio C Hamano 8 siblings, 1 reply; 79+ messages in thread From: Phillip Wood @ 2018-03-01 10:51 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Now that add -p counts patches properly it should be possible to turn off the '--recount' option when invoking 'git apply' Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Notes: I can't think of a reason why this shouldn't be OK but I can't help feeling slightly nervous about it. I've made it a separate patch so it can be easily dropped or reverted if I've missed something. git-add--interactive.perl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 3226c2c4f0..a64c0db57d 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -677,7 +677,7 @@ sub add_untracked_cmd { sub run_git_apply { my $cmd = shift; my $fh; - open $fh, '| git ' . $cmd . " --recount --allow-overlap"; + open $fh, '| git ' . $cmd . " --allow-overlap"; print $fh @_; return close $fh; } -- 2.16.1 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v4 9/9] add -p: don't rely on apply's '--recount' option 2018-03-01 10:51 ` [PATCH v4 9/9] add -p: don't rely on apply's '--recount' option Phillip Wood @ 2018-03-01 20:30 ` Junio C Hamano 2018-03-02 10:49 ` Phillip Wood 0 siblings, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2018-03-01 20:30 UTC (permalink / raw) To: Phillip Wood Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Now that add -p counts patches properly it should be possible to turn > off the '--recount' option when invoking 'git apply' Sounds good ;-) ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v4 9/9] add -p: don't rely on apply's '--recount' option 2018-03-01 20:30 ` Junio C Hamano @ 2018-03-02 10:49 ` Phillip Wood 0 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-02 10:49 UTC (permalink / raw) To: Junio C Hamano Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood On 01/03/18 20:30, Junio C Hamano wrote: > Phillip Wood <phillip.wood@talktalk.net> writes: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> Now that add -p counts patches properly it should be possible to turn >> off the '--recount' option when invoking 'git apply' > > Sounds good ;-) > Lets hope it works! Thanks for your comments, I'll send a re-roll of this series at the beginning of next week Best Wishes Phillip ^ permalink raw reply [flat|nested] 79+ messages in thread
* [PATCH v5 0/9] 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 ` (7 preceding siblings ...) 2018-03-01 10:50 ` [PATCH v4 0/9] Correct offsets of hunks when one is skipped Phillip Wood @ 2018-03-05 10:56 ` Phillip Wood 2018-03-05 10:56 ` [PATCH v5 1/9] add -i: add function to format hunk header Phillip Wood ` (9 more replies) 8 siblings, 10 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-05 10:56 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> I've updated these to clean up the perl style in response to Junio's comments on v4. Cover letter to v1: 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. Interdiff to v4: diff --git a/git-add--interactive.perl b/git-add--interactive.perl index a64c0db57d..f83e7450ad 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -892,8 +892,11 @@ sub merge_hunk { $o_cnt = $n_cnt = 0; for ($i = 1; $i < @{$prev->{TEXT}}; $i++) { my $line = $prev->{TEXT}[$i]; - if ($line =~ /^[+\\]/) { - $n_cnt++ if ($line =~ /^\+/); + if ($line =~ /^\+/) { + $n_cnt++; + push @line, $line; + next; + } elsif ($line =~ /^\\/) { push @line, $line; next; } @@ -910,8 +913,11 @@ sub merge_hunk { for ($i = 1; $i < @{$this->{TEXT}}; $i++) { my $line = $this->{TEXT}[$i]; - if ($line =~ /^[+\\]/) { - $n_cnt++ if ($line =~ /^\+/); + if ($line =~ /^\+/) { + $n_cnt++; + push @line, $line; + next; + } elsif ($line =~ /^\\/) { push @line, $line; next; } @@ -945,7 +951,9 @@ sub coalesce_overlapping_hunks { $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}; + if ($_->{OFS_DELTA}) { + $ofs_delta -= $_->{OFS_DELTA}; + } next; } if ($ofs_delta) { @@ -955,7 +963,9 @@ sub coalesce_overlapping_hunks { } # If this hunk was edited then adjust the offset delta # to reflect the edit. - $_->{OFS_DELTA} and $ofs_delta += $_->{OFS_DELTA}; + if ($_->{OFS_DELTA}) { + $ofs_delta += $_->{OFS_DELTA}; + } if (defined $last_o_ctx && $o_ofs <= $last_o_ctx && !$_->{DIRTY} && Phillip Wood (9): add -i: add function to format hunk header t3701: indent here documents t3701: use test_write_lines and write_script t3701: don't hard code sha1 hash values 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 add -p: fix counting when splitting and coalescing add -p: don't rely on apply's '--recount' option git-add--interactive.perl | 108 +++++++++++++---- t/t3701-add-interactive.sh | 291 +++++++++++++++++++++++++-------------------- 2 files changed, 249 insertions(+), 150 deletions(-) -- 2.16.2 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v5 1/9] add -i: add function to format hunk header 2018-03-05 10:56 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Phillip Wood @ 2018-03-05 10:56 ` Phillip Wood 2018-03-05 10:56 ` [PATCH v5 2/9] t3701: indent here documents Phillip Wood ` (8 subsequent siblings) 9 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-05 10:56 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, 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 964c3a7542..8ababa6453 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.2 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v5 2/9] t3701: indent here documents 2018-03-05 10:56 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Phillip Wood 2018-03-05 10:56 ` [PATCH v5 1/9] add -i: add function to format hunk header Phillip Wood @ 2018-03-05 10:56 ` Phillip Wood 2018-03-05 10:56 ` [PATCH v5 3/9] t3701: use test_write_lines and write_script Phillip Wood ` (7 subsequent siblings) 9 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-05 10:56 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Indent here documents in line with the current style for tests. While at it, quote the end marker of here-docs that do not use variable interpolation. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Notes: changes since v3: - updated to match what was in pu t/t3701-add-interactive.sh | 174 ++++++++++++++++++++++----------------------- 1 file changed, 87 insertions(+), 87 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 058698df6a..3130dafcf0 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -22,14 +22,14 @@ test_expect_success 'status works (initial)' ' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -new file mode 100644 -index 0000000..d95f3ad ---- /dev/null -+++ b/file -@@ -0,0 +1 @@ -+content -EOF + cat >expected <<-\EOF + new file mode 100644 + index 0000000..d95f3ad + --- /dev/null + +++ b/file + @@ -0,0 +1 @@ + +content + EOF ' test_expect_success 'diff works (initial)' ' @@ -59,14 +59,14 @@ test_expect_success 'status works (commit)' ' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -index 180b47c..b6f2c08 100644 ---- a/file -+++ b/file -@@ -1 +1,2 @@ - baseline -+content -EOF + cat >expected <<-\EOF + index 180b47c..b6f2c08 100644 + --- a/file + +++ b/file + @@ -1 +1,2 @@ + baseline + +content + EOF ' test_expect_success 'diff works (commit)' ' @@ -83,8 +83,8 @@ test_expect_success 'revert works (commit)' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -EOF + cat >expected <<-\EOF + EOF ' test_expect_success 'setup fake editor' ' @@ -100,21 +100,21 @@ test_expect_success 'dummy edit works' ' ' test_expect_success 'setup patch' ' -cat >patch <<EOF -@@ -1,1 +1,4 @@ - this -+patch --does not - apply -EOF + cat >patch <<-\EOF + @@ -1,1 +1,4 @@ + this + +patch + -does not + apply + EOF ' test_expect_success 'setup fake editor' ' echo "#!$SHELL_PATH" >fake_editor.sh && - cat >>fake_editor.sh <<\EOF && -mv -f "$1" oldpatch && -mv -f patch "$1" -EOF + cat >>fake_editor.sh <<-\EOF && + mv -f "$1" oldpatch && + mv -f patch "$1" + EOF chmod a+x fake_editor.sh && test_set_editor "$(pwd)/fake_editor.sh" ' @@ -126,10 +126,10 @@ test_expect_success 'bad edit rejected' ' ' test_expect_success 'setup patch' ' -cat >patch <<EOF -this patch -is garbage -EOF + cat >patch <<-\EOF + this patch + is garbage + EOF ' test_expect_success 'garbage edit rejected' ' @@ -139,28 +139,28 @@ test_expect_success 'garbage edit rejected' ' ' test_expect_success 'setup patch' ' -cat >patch <<EOF -@@ -1,0 +1,0 @@ - baseline -+content -+newcontent -+lines -EOF + cat >patch <<-\EOF + @@ -1,0 +1,0 @@ + baseline + +content + +newcontent + +lines + EOF ' test_expect_success 'setup expected' ' -cat >expected <<EOF -diff --git a/file b/file -index b5dd6c9..f910ae9 100644 ---- a/file -+++ b/file -@@ -1,4 +1,4 @@ - baseline - content --newcontent -+more - lines -EOF + cat >expected <<-\EOF + diff --git a/file b/file + index b5dd6c9..f910ae9 100644 + --- a/file + +++ b/file + @@ -1,4 +1,4 @@ + baseline + content + -newcontent + +more + lines + EOF ' test_expect_success 'real edit works' ' @@ -222,31 +222,31 @@ test_expect_success 'setup again' ' # Write the patch file with a new line at the top and bottom test_expect_success 'setup patch' ' -cat >patch <<EOF -index 180b47c..b6f2c08 100644 ---- a/file -+++ b/file -@@ -1,2 +1,4 @@ -+firstline - baseline - content -+lastline -EOF + cat >patch <<-\EOF + index 180b47c..b6f2c08 100644 + --- a/file + +++ b/file + @@ -1,2 +1,4 @@ + +firstline + baseline + content + +lastline + EOF ' # Expected output, similar to the patch but w/ diff at the top test_expect_success 'setup expected' ' -cat >expected <<EOF -diff --git a/file b/file -index b6f2c08..61b9053 100755 ---- a/file -+++ b/file -@@ -1,2 +1,4 @@ -+firstline - baseline - content -+lastline -EOF + cat >expected <<-\EOF + diff --git a/file b/file + index b6f2c08..61b9053 100755 + --- a/file + +++ b/file + @@ -1,2 +1,4 @@ + +firstline + baseline + content + +lastline + EOF ' # Test splitting the first patch, then adding both @@ -259,15 +259,15 @@ test_expect_success 'add first line works' ' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -diff --git a/non-empty b/non-empty -deleted file mode 100644 -index d95f3ad..0000000 ---- a/non-empty -+++ /dev/null -@@ -1 +0,0 @@ --content -EOF + cat >expected <<-\EOF + diff --git a/non-empty b/non-empty + deleted file mode 100644 + index d95f3ad..0000000 + --- a/non-empty + +++ /dev/null + @@ -1 +0,0 @@ + -content + EOF ' test_expect_success 'deleting a non-empty file' ' @@ -282,11 +282,11 @@ test_expect_success 'deleting a non-empty file' ' ' test_expect_success 'setup expected' ' -cat >expected <<EOF -diff --git a/empty b/empty -deleted file mode 100644 -index e69de29..0000000 -EOF + cat >expected <<-\EOF + diff --git a/empty b/empty + deleted file mode 100644 + index e69de29..0000000 + EOF ' test_expect_success 'deleting an empty file' ' -- 2.16.2 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v5 3/9] t3701: use test_write_lines and write_script 2018-03-05 10:56 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Phillip Wood 2018-03-05 10:56 ` [PATCH v5 1/9] add -i: add function to format hunk header Phillip Wood 2018-03-05 10:56 ` [PATCH v5 2/9] t3701: indent here documents Phillip Wood @ 2018-03-05 10:56 ` Phillip Wood 2018-03-05 10:56 ` [PATCH v5 4/9] t3701: don't hard code sha1 hash values Phillip Wood ` (6 subsequent siblings) 9 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-05 10:56 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Simplify things slightly by using the above helpers. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Notes: changes since v2 - fixed use of test_set_editor to match what was in pu t/t3701-add-interactive.sh | 33 +++++---------------------------- 1 file changed, 5 insertions(+), 28 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 3130dafcf0..836ce346ed 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -87,13 +87,8 @@ test_expect_success 'setup expected' ' EOF ' -test_expect_success 'setup fake editor' ' - >fake_editor.sh && - chmod a+x fake_editor.sh && - test_set_editor "$(pwd)/fake_editor.sh" -' - test_expect_success 'dummy edit works' ' + test_set_editor : && (echo e; echo a) | git add -p && git diff > diff && test_cmp expected diff @@ -110,12 +105,10 @@ test_expect_success 'setup patch' ' ' test_expect_success 'setup fake editor' ' - echo "#!$SHELL_PATH" >fake_editor.sh && - cat >>fake_editor.sh <<-\EOF && + write_script "fake_editor.sh" <<-\EOF && mv -f "$1" oldpatch && mv -f patch "$1" EOF - chmod a+x fake_editor.sh && test_set_editor "$(pwd)/fake_editor.sh" ' @@ -302,18 +295,12 @@ test_expect_success 'deleting an empty file' ' test_expect_success 'split hunk setup' ' git reset --hard && - for i in 10 20 30 40 50 60 - do - echo $i - done >test && + test_write_lines 10 20 30 40 50 60 >test && git add test && test_tick && git commit -m test && - for i in 10 15 20 21 22 23 24 30 40 50 60 - do - echo $i - done >test + test_write_lines 10 15 20 21 22 23 24 30 40 50 60 >test ' test_expect_success 'split hunk "add -p (edit)"' ' @@ -334,17 +321,7 @@ test_expect_success 'split hunk "add -p (edit)"' ' ' test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' - cat >test <<-\EOF && - 5 - 10 - 20 - 21 - 30 - 31 - 40 - 50 - 60 - EOF + test_write_lines 5 10 20 21 30 31 40 50 60 >test && git reset && # test sequence is s(plit), n(o), y(es), e(dit) # q n q q is there to make sure we exit at the end. -- 2.16.2 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v5 4/9] t3701: don't hard code sha1 hash values 2018-03-05 10:56 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (2 preceding siblings ...) 2018-03-05 10:56 ` [PATCH v5 3/9] t3701: use test_write_lines and write_script Phillip Wood @ 2018-03-05 10:56 ` Phillip Wood 2018-03-05 10:56 ` [PATCH v5 5/9] t3701: add failing test for pathological context lines Phillip Wood ` (5 subsequent siblings) 9 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-05 10:56 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Use a filter when comparing diffs to fix the value of non-zero hashes in diff index lines so we're not hard coding sha1 hash values in the expected output. This makes it easier to change the expected output if a test is edited as we don't need to worry about the exact hash value and means the tests will work when the hash algorithm is transitioned away from sha1. Thanks-to: Junio C Hamano <gitster@pobox.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Notes: changes since v3: - fix zero hash values to seven digits changes since v2: - fix hash values in index lines rather than removing the line - reworded commit message t/t3701-add-interactive.sh | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 836ce346ed..f95714230b 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -10,6 +10,19 @@ then test_done fi +diff_cmp () { + for x + do + sed -e '/^index/s/[0-9a-f]*[1-9a-f][0-9a-f]*\.\./1234567../' \ + -e '/^index/s/\.\.[0-9a-f]*[1-9a-f][0-9a-f]*/..9abcdef/' \ + -e '/^index/s/ 00*\.\./ 0000000../' \ + -e '/^index/s/\.\.00*$/..0000000/' \ + -e '/^index/s/\.\.00* /..0000000 /' \ + "$x" >"$x.filtered" + done + test_cmp "$1.filtered" "$2.filtered" +} + test_expect_success 'setup (initial)' ' echo content >file && git add file && @@ -35,7 +48,7 @@ test_expect_success 'setup expected' ' test_expect_success 'diff works (initial)' ' (echo d; echo 1) | git add -i >output && sed -ne "/new file/,/content/p" <output >diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'revert works (initial)' ' git add file && @@ -72,7 +85,7 @@ test_expect_success 'setup expected' ' test_expect_success 'diff works (commit)' ' (echo d; echo 1) | git add -i >output && sed -ne "/^index/,/content/p" <output >diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'revert works (commit)' ' git add file && @@ -91,7 +104,7 @@ test_expect_success 'dummy edit works' ' test_set_editor : && (echo e; echo a) | git add -p && git diff > diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'setup patch' ' @@ -159,7 +172,7 @@ test_expect_success 'setup expected' ' test_expect_success 'real edit works' ' (echo e; echo n; echo d) | git add -p && git diff >output && - test_cmp expected output + diff_cmp expected output ' test_expect_success 'skip files similarly as commit -a' ' @@ -171,7 +184,7 @@ test_expect_success 'skip files similarly as commit -a' ' git reset && git commit -am commit && git diff >expected && - test_cmp expected output && + diff_cmp expected output && git reset --hard HEAD^ ' rm -f .gitignore @@ -248,7 +261,7 @@ test_expect_success 'add first line works' ' git apply patch && (echo s; echo y; echo y) | git add -p file && git diff --cached > diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'setup expected' ' @@ -271,7 +284,7 @@ test_expect_success 'deleting a non-empty file' ' rm non-empty && echo y | git add -p non-empty && git diff --cached >diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'setup expected' ' @@ -290,7 +303,7 @@ test_expect_success 'deleting an empty file' ' rm empty && echo y | git add -p empty && git diff --cached >diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success 'split hunk setup' ' @@ -355,7 +368,7 @@ test_expect_success 'patch mode ignores unmerged entries' ' +changed EOF git diff --cached >diff && - test_cmp expected diff + diff_cmp expected diff ' test_expect_success TTY 'diffs can be colorized' ' @@ -384,7 +397,7 @@ test_expect_success 'patch-mode via -i prompts for files' ' echo test >expect && git diff --cached --name-only >actual && - test_cmp expect actual + diff_cmp expect actual ' test_expect_success 'add -p handles globs' ' -- 2.16.2 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v5 5/9] t3701: add failing test for pathological context lines 2018-03-05 10:56 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (3 preceding siblings ...) 2018-03-05 10:56 ` [PATCH v5 4/9] t3701: don't hard code sha1 hash values Phillip Wood @ 2018-03-05 10:56 ` Phillip Wood 2018-03-05 10:56 ` [PATCH v5 6/9] add -p: adjust offsets of subsequent hunks when one is skipped Phillip Wood ` (4 subsequent siblings) 9 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-05 10:56 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, 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> --- Notes: changes since v2: - removed test_set_editor as it is already set changes since v1: - changed edit test to use the existing fake editor and to strip the hunk header and some context lines as well as deleting an insertion - style fixes 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 f95714230b..094eeca405 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -531,4 +531,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 && + # check editing can cope with missing header and deleted context lines + # as well as changes to other lines + test_write_lines +b " a" >patch +' + +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 && + # n q q below is in case edit fails + printf "%s\n" e y n q q | + git add -p && + git cat-file blob :a >actual && + test_cmp expected-2 actual +' + test_done -- 2.16.2 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v5 6/9] add -p: adjust offsets of subsequent hunks when one is skipped 2018-03-05 10:56 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (4 preceding siblings ...) 2018-03-05 10:56 ` [PATCH v5 5/9] t3701: add failing test for pathological context lines Phillip Wood @ 2018-03-05 10:56 ` Phillip Wood 2018-03-05 10:56 ` [PATCH v5 7/9] add -p: calculate offset delta for edited patches Phillip Wood ` (3 subsequent siblings) 9 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-05 10:56 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, 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 8ababa6453..7a0a5896bb 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 094eeca405..e3150a4e07 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -544,7 +544,7 @@ test_expect_success 'set up pathological context' ' test_write_lines +b " a" >patch ' -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.2 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v5 7/9] add -p: calculate offset delta for edited patches 2018-03-05 10:56 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (5 preceding siblings ...) 2018-03-05 10:56 ` [PATCH v5 6/9] add -p: adjust offsets of subsequent hunks when one is skipped Phillip Wood @ 2018-03-05 10:56 ` Phillip Wood 2018-03-05 10:56 ` [PATCH v5 8/9] add -p: fix counting when splitting and coalescing Phillip Wood ` (2 subsequent siblings) 9 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-05 10:56 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, 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> --- Notes: changes since v4: - replaced instances of <<conditional>> and <<statement with side effect>>; with if (<<conditional>>) { <<statement with side effect>>; } changes since v1 - edited hunks are now recounted before seeing if they apply in preparation for removing '--recount' when invoking 'git apply' - added sentence to commit message about the offset data being lost if an edited hunk is split git-add--interactive.perl | 59 +++++++++++++++++++++++++++++++++++++++------- t/t3701-add-interactive.sh | 2 +- 2 files changed, 51 insertions(+), 10 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 7a0a5896bb..f80372d3ad 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -938,13 +938,23 @@ 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. + if ($_->{OFS_DELTA}) { + $ofs_delta -= $_->{OFS_DELTA}; + } next; } if ($ofs_delta) { $n_ofs += $ofs_delta; $_->{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. + if ($_->{OFS_DELTA}) { + $ofs_delta += $_->{OFS_DELTA}; + } if (defined $last_o_ctx && $o_ofs <= $last_o_ctx && !$_->{DIRTY} && @@ -1016,6 +1026,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 +1148,32 @@ 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, }; + $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}; 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->{DISPLAY} = [color_diff(@{$newtext})]; return $newhunk; } else { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index e3150a4e07..4cc9517eda 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -552,7 +552,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 && # n q q below is in case edit fails printf "%s\n" e y n q q | -- 2.16.2 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v5 8/9] add -p: fix counting when splitting and coalescing 2018-03-05 10:56 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (6 preceding siblings ...) 2018-03-05 10:56 ` [PATCH v5 7/9] add -p: calculate offset delta for edited patches Phillip Wood @ 2018-03-05 10:56 ` Phillip Wood 2018-03-05 10:56 ` [PATCH v5 9/9] add -p: don't rely on apply's '--recount' option Phillip Wood 2018-03-05 18:50 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Junio C Hamano 9 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-05 10:56 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> When a file has no trailing new line at the end diff records this by appending "\ No newline at end of file" below the last line of the file. This line should not be counted in the hunk header. Fix the splitting and coalescing code to count files without a trailing new line properly and change one of the tests to test splitting without a trailing new line. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Notes: changes since v4: - use a separate if clause to handle with "\ No newline at end of file" lines rather than crowbarring them into the "+" line handling. git-add--interactive.perl | 11 +++++++++++ t/t3701-add-interactive.sh | 31 +++++++++++++++++++++++-------- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index f80372d3ad..d1b550d4d8 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -793,6 +793,11 @@ sub split_hunk { while (++$i < @$text) { my $line = $text->[$i]; my $display = $display->[$i]; + if ($line =~ /^\\/) { + push @{$this->{TEXT}}, $line; + push @{$this->{DISPLAY}}, $display; + next; + } if ($line =~ /^ /) { if ($this->{ADDDEL} && !defined $next_hunk_start) { @@ -891,6 +896,9 @@ sub merge_hunk { $n_cnt++; push @line, $line; next; + } elsif ($line =~ /^\\/) { + push @line, $line; + next; } last if ($o1_ofs <= $ofs); @@ -909,6 +917,9 @@ sub merge_hunk { $n_cnt++; push @line, $line; next; + } elsif ($line =~ /^\\/) { + push @line, $line; + next; } $ofs++; $o_cnt++; diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 4cc9517eda..a9a9478a29 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -237,31 +237,46 @@ test_expect_success 'setup patch' ' baseline content +lastline + \ No newline at end of file EOF ' -# Expected output, similar to the patch but w/ diff at the top +# Expected output, diff is similar to the patch but w/ diff at the top test_expect_success 'setup expected' ' - cat >expected <<-\EOF - diff --git a/file b/file - index b6f2c08..61b9053 100755 + echo diff --git a/file b/file >expected && + cat patch |sed "/^index/s/ 100644/ 100755/" >>expected && + cat >expected-output <<-\EOF --- a/file +++ b/file @@ -1,2 +1,4 @@ +firstline baseline content +lastline + \ No newline at end of file + @@ -1,2 +1,3 @@ + +firstline + baseline + content + @@ -1,2 +2,3 @@ + baseline + content + +lastline + \ No newline at end of file EOF ' # Test splitting the first patch, then adding both -test_expect_success 'add first line works' ' +test_expect_success C_LOCALE_OUTPUT 'add first line works' ' git commit -am "clear local changes" && git apply patch && - (echo s; echo y; echo y) | git add -p file && - git diff --cached > diff && - diff_cmp expected diff + printf "%s\n" s y y | git add -p file 2>error | + sed -n -e "s/^Stage this hunk[^@]*\(@@ .*\)/\1/" \ + -e "/^[-+@ \\\\]"/p >output && + test_must_be_empty error && + git diff --cached >diff && + diff_cmp expected diff && + test_cmp expected-output output ' test_expect_success 'setup expected' ' -- 2.16.2 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* [PATCH v5 9/9] add -p: don't rely on apply's '--recount' option 2018-03-05 10:56 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (7 preceding siblings ...) 2018-03-05 10:56 ` [PATCH v5 8/9] add -p: fix counting when splitting and coalescing Phillip Wood @ 2018-03-05 10:56 ` Phillip Wood 2018-03-05 18:50 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Junio C Hamano 9 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-05 10:56 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Brian M. Carlson, Eric Sunshine, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Now that add -p counts patches properly it should be possible to turn off the '--recount' option when invoking 'git apply' Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Notes: I can't think of a reason why this shouldn't be OK but I can't help feeling slightly nervous about it. I've made it a separate patch so it can be easily dropped or reverted if I've missed something. git-add--interactive.perl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index d1b550d4d8..f83e7450ad 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -677,7 +677,7 @@ sub add_untracked_cmd { sub run_git_apply { my $cmd = shift; my $fh; - open $fh, '| git ' . $cmd . " --recount --allow-overlap"; + open $fh, '| git ' . $cmd . " --allow-overlap"; print $fh @_; return close $fh; } -- 2.16.2 ^ permalink raw reply related [flat|nested] 79+ messages in thread
* Re: [PATCH v5 0/9] Correct offsets of hunks when one is skipped 2018-03-05 10:56 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Phillip Wood ` (8 preceding siblings ...) 2018-03-05 10:56 ` [PATCH v5 9/9] add -p: don't rely on apply's '--recount' option Phillip Wood @ 2018-03-05 18:50 ` Junio C Hamano 2018-03-06 10:25 ` Phillip Wood 9 siblings, 1 reply; 79+ messages in thread From: Junio C Hamano @ 2018-03-05 18:50 UTC (permalink / raw) To: Phillip Wood Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > I've updated these to clean up the perl style in response to Junio's > comments on v4. I've considered the use of "apply --recount" in this script (eh, rather, the existence of that option in "apply" command itself ;-)) a bug that need to be eventually fixed for a long time, so the copy of earlier rounds I've been carrying in my tree were forked from 'maint'. I'll queue this round on the same base commit as before to replace; I _think_ this is ready for 'next'. Thanks for working on this. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH v5 0/9] Correct offsets of hunks when one is skipped 2018-03-05 18:50 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Junio C Hamano @ 2018-03-06 10:25 ` Phillip Wood 0 siblings, 0 replies; 79+ messages in thread From: Phillip Wood @ 2018-03-06 10:25 UTC (permalink / raw) To: Junio C Hamano Cc: Git Mailing List, Brian M. Carlson, Eric Sunshine, Phillip Wood On 05/03/18 18:50, Junio C Hamano wrote: > Phillip Wood <phillip.wood@talktalk.net> writes: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> I've updated these to clean up the perl style in response to Junio's >> comments on v4. > > I've considered the use of "apply --recount" in this script (eh, > rather, the existence of that option in "apply" command itself ;-)) > a bug that need to be eventually fixed for a long time, so the copy > of earlier rounds I've been carrying in my tree were forked from > 'maint'. Oh I've been basing this on master, I hope that wasn't a problem. > I'll queue this round on the same base commit as before to replace; > I _think_ this is ready for 'next'. Yes I think so (I've not come up with any new ways to break it, lets see if someone else can) > Thanks for working on this. > Thanks, I use add -p quite a lot so it's nice to be able to contribute something back. Best Wishes Phillip ^ permalink raw reply [flat|nested] 79+ messages in thread
end of thread, other threads:[~2018-03-06 10:25 UTC | newest] Thread overview: 79+ 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 2018-02-19 13:01 ` Phillip Wood 2018-02-19 11:29 ` [PATCH v2 0/9] " Phillip Wood 2018-02-19 11:29 ` [PATCH v2 1/9] add -i: add function to format hunk header Phillip Wood 2018-02-20 18:21 ` Junio C Hamano 2018-02-19 11:29 ` [PATCH v2 2/9] t3701: indent here documents Phillip Wood 2018-02-19 18:36 ` Eric Sunshine 2018-02-19 11:29 ` [PATCH v2 3/9] t3701: use test_write_lines and write_script Phillip Wood 2018-02-19 18:47 ` Eric Sunshine 2018-02-20 17:19 ` Junio C Hamano 2018-02-21 11:26 ` Phillip Wood 2018-02-19 11:29 ` [PATCH v2 4/9] t3701: don't hard code sha1 hash values Phillip Wood 2018-02-19 18:52 ` Eric Sunshine 2018-02-20 17:39 ` Junio C Hamano 2018-02-21 11:42 ` Phillip Wood 2018-02-21 16:58 ` Junio C Hamano 2018-02-19 11:29 ` [PATCH v2 5/9] t3701: add failing test for pathological context lines Phillip Wood 2018-02-21 11:28 ` Phillip Wood 2018-02-19 11:29 ` [PATCH v2 6/9] add -p: Adjust offsets of subsequent hunks when one is skipped Phillip Wood 2018-02-19 11:29 ` [PATCH v2 7/9] add -p: calculate offset delta for edited patches Phillip Wood 2018-02-19 11:29 ` [PATCH v2 8/9] add -p: fix counting when splitting and coalescing Phillip Wood 2018-02-19 11:29 ` [PATCH v2 9/9] add -p: don't rely on apply's '--recount' option Phillip Wood 2018-02-20 18:50 ` Junio C Hamano 2018-02-27 11:03 ` [PATCH v3 0/9] Correct offsets of hunks when one is skipped Phillip Wood 2018-02-27 11:03 ` [PATCH v3 1/9] add -i: add function to format hunk header Phillip Wood 2018-02-27 11:03 ` [PATCH v3 2/9] t3701: indent here documents Phillip Wood 2018-02-27 22:35 ` Junio C Hamano 2018-02-28 11:00 ` Phillip Wood 2018-02-28 15:37 ` Junio C Hamano 2018-03-01 10:57 ` Phillip Wood 2018-03-01 15:58 ` Junio C Hamano 2018-02-27 11:03 ` [PATCH v3 3/9] t3701: use test_write_lines and write_script Phillip Wood 2018-02-27 11:03 ` [PATCH v3 4/9] t3701: don't hard code sha1 hash values Phillip Wood 2018-02-27 22:42 ` Junio C Hamano 2018-02-28 11:03 ` Phillip Wood 2018-02-28 11:10 ` Phillip Wood 2018-02-27 11:04 ` [PATCH v3 5/9] t3701: add failing test for pathological context lines Phillip Wood 2018-02-27 11:04 ` [PATCH v3 6/9] add -p: Adjust offsets of subsequent hunks when one is skipped Phillip Wood 2018-02-27 11:04 ` [PATCH v3 7/9] add -p: calculate offset delta for edited patches Phillip Wood 2018-02-27 11:04 ` [PATCH v3 8/9] add -p: fix counting when splitting and coalescing Phillip Wood 2018-02-27 11:04 ` [PATCH v3 9/9] add -p: don't rely on apply's '--recount' option Phillip Wood 2018-03-01 10:50 ` [PATCH v4 0/9] Correct offsets of hunks when one is skipped Phillip Wood 2018-03-01 10:50 ` [PATCH v4 1/9] add -i: add function to format hunk header Phillip Wood 2018-03-01 10:50 ` [PATCH v4 2/9] t3701: indent here documents Phillip Wood 2018-03-01 10:50 ` [PATCH v4 3/9] t3701: use test_write_lines and write_script Phillip Wood 2018-03-01 10:50 ` [PATCH v4 4/9] t3701: don't hard code sha1 hash values Phillip Wood 2018-03-02 15:55 ` SZEDER Gábor 2018-03-02 16:09 ` Junio C Hamano 2018-03-05 10:59 ` Phillip Wood 2018-03-05 12:32 ` SZEDER Gábor 2018-03-01 10:50 ` [PATCH v4 5/9] t3701: add failing test for pathological context lines Phillip Wood 2018-03-01 10:51 ` [PATCH v4 6/9] add -p: adjust offsets of subsequent hunks when one is skipped Phillip Wood 2018-03-01 10:51 ` [PATCH v4 7/9] add -p: calculate offset delta for edited patches Phillip Wood 2018-03-01 20:14 ` Junio C Hamano 2018-03-02 10:36 ` Phillip Wood 2018-03-01 10:51 ` [PATCH v4 8/9] add -p: fix counting when splitting and coalescing Phillip Wood 2018-03-01 20:29 ` Junio C Hamano 2018-03-02 10:48 ` Phillip Wood 2018-03-01 10:51 ` [PATCH v4 9/9] add -p: don't rely on apply's '--recount' option Phillip Wood 2018-03-01 20:30 ` Junio C Hamano 2018-03-02 10:49 ` Phillip Wood 2018-03-05 10:56 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Phillip Wood 2018-03-05 10:56 ` [PATCH v5 1/9] add -i: add function to format hunk header Phillip Wood 2018-03-05 10:56 ` [PATCH v5 2/9] t3701: indent here documents Phillip Wood 2018-03-05 10:56 ` [PATCH v5 3/9] t3701: use test_write_lines and write_script Phillip Wood 2018-03-05 10:56 ` [PATCH v5 4/9] t3701: don't hard code sha1 hash values Phillip Wood 2018-03-05 10:56 ` [PATCH v5 5/9] t3701: add failing test for pathological context lines Phillip Wood 2018-03-05 10:56 ` [PATCH v5 6/9] add -p: adjust offsets of subsequent hunks when one is skipped Phillip Wood 2018-03-05 10:56 ` [PATCH v5 7/9] add -p: calculate offset delta for edited patches Phillip Wood 2018-03-05 10:56 ` [PATCH v5 8/9] add -p: fix counting when splitting and coalescing Phillip Wood 2018-03-05 10:56 ` [PATCH v5 9/9] add -p: don't rely on apply's '--recount' option Phillip Wood 2018-03-05 18:50 ` [PATCH v5 0/9] Correct offsets of hunks when one is skipped Junio C Hamano 2018-03-06 10:25 ` Phillip Wood
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).