Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add -p: a couple of hunk splitting fixes #1863

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add -p: a couple of hunk splitting fixes #1863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion add-patch.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1181,19 +1181,29 @@ static ssize_t recount_edited_hunk(struct add_p_state *s, struct hunk *hunk,
{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Justin Tobler wrote (reply to this):

On 25/02/21 02:57PM, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <[email protected]>
> 
> When the users edits a hunk if they change deletion lines to context
> lines or vice versa then the number of hunks that the edited hunk can be
> split into may differ from the unedited hunk and so we need to update
> hunk->splittable_into. In practice users are unlikely to hit this bug as
> it is doubtful that a user who has edited a hunk will split it
> afterwards.

If I'm understanding this correctly, the issue here is that, when a patch
is editted, the number of hunks in can be split into may change, but is
not reevaluated. This could lead to issue if the editted hunk is
subsequently split.

This issue would also apply to addition lines being changed to context
lines as well correct?

> 
> Signed-off-by: Phillip Wood <[email protected]>
> ---
>  add-patch.c                | 12 +++++++++++-
>  t/t3701-add-interactive.sh | 21 +++++++++++++++++++++
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index f44f98275cc..982745373df 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1182,19 +1182,29 @@ static ssize_t recount_edited_hunk(struct add_p_state *s, struct hunk *hunk,
>  {
>  	struct hunk_header *header = &hunk->header;
>  	size_t i;
> +	char ch, marker = ' ';
>  
> +	hunk->splittable_into = 0;
>  	header->old_count = header->new_count = 0;
>  	for (i = hunk->start; i < hunk->end; ) {
> -		switch(normalize_marker(&s->plain.buf[i])) {
> +		ch = normalize_marker(&s->plain.buf[i]);
> +		switch (ch) {
>  		case '-':
>  			header->old_count++;
> +			if (marker == ' ')
> +				hunk->splittable_into++;
> +			marker = ch;
>  			break;
>  		case '+':
>  			header->new_count++;
> +			if (marker == ' ')
> +				hunk->splittable_into++;
> +			marker = ch;
>  			break;
>  		case ' ':
>  			header->old_count++;
>  			header->new_count++;
> +			marker = ch;
>  			break;
>  		}

Ok with this we now recount the potential number of hunks a hunk can be
split into. Whenever there is a change from a context line to a
addition or deletion line, a separate hunk could be formed.

-Justin

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Phillip Wood via GitGitGadget" <[email protected]> writes:

> From: Phillip Wood <[email protected]>
>
> When the users edits a hunk if they change deletion lines to context
> lines or vice versa then the number of hunks that the edited hunk can be
> split into may differ from the unedited hunk and so we need to update
> hunk->splittable_into. In practice users are unlikely to hit this bug as
> it is doubtful that a user who has edited a hunk will split it
> afterwards.

Heh, when I did the original "add -i/-p", I said "it is doubtful
that a user who has selected a hunk will split it afterwards" ;-)

> Signed-off-by: Phillip Wood <[email protected]>
> ---
>  add-patch.c                | 12 +++++++++++-
>  t/t3701-add-interactive.sh | 21 +++++++++++++++++++++
>  2 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index f44f98275cc..982745373df 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1182,19 +1182,29 @@ static ssize_t recount_edited_hunk(struct add_p_state *s, struct hunk *hunk,
>  {
>  	struct hunk_header *header = &hunk->header;
>  	size_t i;
> +	char ch, marker = ' ';
>  
> +	hunk->splittable_into = 0;
>  	header->old_count = header->new_count = 0;
>  	for (i = hunk->start; i < hunk->end; ) {
> -		switch(normalize_marker(&s->plain.buf[i])) {
> +		ch = normalize_marker(&s->plain.buf[i]);
> +		switch (ch) {
>  		case '-':
>  			header->old_count++;
> +			if (marker == ' ')
> +				hunk->splittable_into++;
> +			marker = ch;
>  			break;
>  		case '+':
>  			header->new_count++;
> +			if (marker == ' ')
> +				hunk->splittable_into++;
> +			marker = ch;
>  			break;
>  		case ' ':
>  			header->old_count++;
>  			header->new_count++;
> +			marker = ch;
>  			break;
>  		}

OK.

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 760f3d0d30f..cb81bfe64c8 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -1240,4 +1240,25 @@ test_expect_success 'splitting previous hunk marks split hunks as undecided' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'splitting edited hunk' '
> +	# Before the first hunk is edited it can be split into two
> +	# hunks, after editing it can be split into three hunks.
> +
> +	write_script fake-editor.sh <<-\EOF &&
> +	sed "s/^ c/-c/" "$1" >"$1.tmp" &&
> +	mv "$1.tmp" "$1"
> +	EOF
> +
> +	test_write_lines a b c d e f g h i j k l m n>file &&
> +	git add file &&
> +	test_write_lines A b c d E f g h i j k l M n >file &&

Missing SP before ">file" on the earlier line.

> +	(
> +		test_set_editor "$(pwd)/fake-editor.sh" &&
> +		test_write_lines e K s j y n y q | git add -p file
> +	) &&
> +	git cat-file blob :file >actual &&
> +	test_write_lines a b d e f g h i j k l M n >expect &&
> +	test_cmp expect actual
> +'
> +
>  test_done

struct hunk_header *header = &hunk->header;
size_t i;
char ch, marker = ' ';

hunk->splittable_into = 0;
header->old_count = header->new_count = 0;
for (i = hunk->start; i < hunk->end; ) {
switch(normalize_marker(&s->plain.buf[i])) {
ch = normalize_marker(&s->plain.buf[i]);
switch (ch) {
case '-':
header->old_count++;
if (marker == ' ')
hunk->splittable_into++;
marker = ch;
break;
case '+':
header->new_count++;
if (marker == ' ')
hunk->splittable_into++;
marker = ch;
break;
case ' ':
header->old_count++;
header->new_count++;
marker = ch;
break;
}

Expand Down
21 changes: 21 additions & 0 deletions t/t3701-add-interactive.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1230,4 +1230,25 @@ test_expect_success 'hunk splitting works with diff.suppressBlankEmpty' '
test_cmp expect actual
'

test_expect_success 'splitting edited hunk' '
# Before the first hunk is edited it can be split into two
# hunks, after editing it can be split into three hunks.

write_script fake-editor.sh <<-\EOF &&
sed "s/^ c/-c/" "$1" >"$1.tmp" &&
mv "$1.tmp" "$1"
EOF

test_write_lines a b c d e f g h i j k l m n >file &&
git add file &&
test_write_lines A b c d E f g h i j k l M n >file &&
(
test_set_editor "$(pwd)/fake-editor.sh" &&
test_write_lines e K s n K n y q | git add -p file
) &&
git cat-file blob :file >actual &&
test_write_lines a b d e f g h i j k l M n >expect &&
test_cmp expect actual
'

test_done
Loading