[PATCH 0/2] add -p: a couple of hunk splitting fixes

[PATCH 0/2] add -p: a couple of hunk splitting fixes

[Phillip Wood via GitGitGadget]

This series fixes a couple of infelicities when splitting hunks that have
already been selected or edited which I noticed a while ago when preparing
the test for 'pw/add-patch-with-suppress-blank-empty'.

Phillip Wood (2):
  add -p: mark split hunks as undecided
  add-patch: update hunk splitability after editing

 add-patch.c                | 15 +++++++++++++--
 t/t3701-add-interactive.sh | 31 +++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 2 deletions(-)


base-commit: 03944513488db4a81fdb4c21c3b515e4cb260b05
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1863%2Fphillipwood%2Fadd-p-split-hunks-are-undecided-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1863/phillipwood/add-p-split-hunks-are-undecided-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1863
-- 
gitgitgadget

[PATCH 1/2] add -p: mark split hunks as undecided

[Phillip Wood via GitGitGadget]

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

When a hunk is split each of the new hunks inherits whether it is
selected or not from the original hunk. This means that if a selected
hunk is split all of the new hunks are selected and the user is not asked
whether or not they want to select the new hunks. This is unfortunate as
the user is presumably splitting the original hunk because they only
want to select some sub-set of it. Fix this by marking all the new hunks
as "undecided" so that we prompt the user to decide whether to select
them or not.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 add-patch.c                |  3 ++-
 t/t3701-add-interactive.sh | 10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/add-patch.c b/add-patch.c
index 95c67d8c80c..f44f98275cc 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -953,6 +953,7 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
 			* sizeof(*hunk));
 	hunk = file_diff->hunk + hunk_index;
 	hunk->splittable_into = 1;
+	hunk->use = UNDECIDED_HUNK;
 	memset(hunk + 1, 0, (splittable_into - 1) * sizeof(*hunk));
 
 	header = &hunk->header;
@@ -1054,7 +1055,7 @@ next_hunk_line:
 
 		hunk++;
 		hunk->splittable_into = 1;
-		hunk->use = hunk[-1].use;
+		hunk->use = UNDECIDED_HUNK;
 		header = &hunk->header;
 
 		header->old_count = header->new_count = context_line_count;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index b8a05d95f3f..760f3d0d30f 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1230,4 +1230,14 @@ test_expect_success 'hunk splitting works with diff.suppressBlankEmpty' '
 	test_cmp expect actual
 '
 
+test_expect_success 'splitting previous hunk marks split hunks as undecided' '
+	test_write_lines a " " b c d e f g h i j k >file &&
+	git add file &&
+	test_write_lines x " " b y d e f g h i j x >file &&
+	test_write_lines n K s n y q | git add -p file &&
+	git cat-file blob :file >actual &&
+	test_write_lines a " " b y d e f g h i j k >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
gitgitgadget

[PATCH 2/2] add-patch: update hunk splitability after editing

[Phillip Wood via GitGitGadget]

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

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.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 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;
 		}
 
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 &&
+	(
+		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
-- 
gitgitgadget

Re: [PATCH 1/2] add -p: mark split hunks as undecided

[Justin Tobler]

On 25/02/21 02:57PM, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> When a hunk is split each of the new hunks inherits whether it is
> selected or not from the original hunk. This means that if a selected
> hunk is split all of the new hunks are selected and the user is not asked
> whether or not they want to select the new hunks. This is unfortunate as
> the user is presumably splitting the original hunk because they only
> want to select some sub-set of it. Fix this by marking all the new hunks
> as "undecided" so that we prompt the user to decide whether to select
> them or not.

Ok, each hunk may have {UNDECIDED,SKIP,USE}_HUNK set to denote its
current "use" state. When splitting a hunk, the new hunks always use the
previous hunk's value. This means that, if the hunk being split is
already set to skip or use, the new hunks from the split will inherit
the same value.

If a user wants to split a hunk, they likely intend to select only a
portion of the hunk. Setting each of the new hunks to same value may not
be the most intuitive behavior in this case. Resetting the hunk "use"
value results the user being prompted for each of these hunks again.

If you have a very large hunk that would get split into many smaller
hunks, this does mean that you will have to explicitly set the value for
each now. If the user only wanted to change a small portion, this could
be a bit tedious. I'm not sure this is a big setback though.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  add-patch.c                |  3 ++-
>  t/t3701-add-interactive.sh | 10 ++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index 95c67d8c80c..f44f98275cc 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -953,6 +953,7 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
>  			* sizeof(*hunk));
>  	hunk = file_diff->hunk + hunk_index;
>  	hunk->splittable_into = 1;
> +	hunk->use = UNDECIDED_HUNK;

Ok, we reset the current hunk to be undecided. Makes sense

>  	memset(hunk + 1, 0, (splittable_into - 1) * sizeof(*hunk));
>  
>  	header = &hunk->header;
> @@ -1054,7 +1055,7 @@ next_hunk_line:
>  
>  		hunk++;
>  		hunk->splittable_into = 1;
> -		hunk->use = hunk[-1].use;
> +		hunk->use = UNDECIDED_HUNK;

Here each of the new hunks are explicitly set to be undecided. Since we
always override the initial hunk to be undecided, I think the new hunks
would already be set undecided as well. I don't think it hurts to be
explicit though.

-Justin

>  		header = &hunk->header;
>  
>  		header->old_count = header->new_count = context_line_count;
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index b8a05d95f3f..760f3d0d30f 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -1230,4 +1230,14 @@ test_expect_success 'hunk splitting works with diff.suppressBlankEmpty' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'splitting previous hunk marks split hunks as undecided' '
> +	test_write_lines a " " b c d e f g h i j k >file &&
> +	git add file &&
> +	test_write_lines x " " b y d e f g h i j x >file &&
> +	test_write_lines n K s n y q | git add -p file &&
> +	git cat-file blob :file >actual &&
> +	test_write_lines a " " b y d e f g h i j k >expect &&
> +	test_cmp expect actual
> +'
> +
>  test_done
> -- 
> gitgitgadget
> 
> 

Re: [PATCH 2/2] add-patch: update hunk splitability after editing

[Justin Tobler]

On 25/02/21 02:57PM, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> 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 <phillip.wood@dunelm.org.uk>
> ---
>  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

Re: [PATCH 1/2] add -p: mark split hunks as undecided

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When a hunk is split each of the new hunks inherits whether it is
> selected or not from the original hunk. This means that if a selected
> hunk is split all of the new hunks are selected and the user is not asked
> whether or not they want to select the new hunks. This is unfortunate as
> the user is presumably splitting the original hunk because they only
> want to select some sub-set of it. Fix this by marking all the new hunks
> as "undecided" so that we prompt the user to decide whether to select
> them or not.

Good.  I am very sure that the design of the current behaviour goes
back to the very original version of "add -p" with hunk splitting I
invented; I simply never considered a workflow where people may
first select and say "oops, let me take it back and redo it".  What
I am getting at is that I do not think the current behaviour is
something I designed it to be with too much thought, and debeting if
it makes sense or it would be better to force them to be undecided
is probably a good thing to do now.

Having said that, I have one small concern about forcing them to be
undecided.  This now allows it to

 1. Add the whole hunk 
 2. Go back (with K) to that already chosen hunk
 3. Split

and makes the resulting minihunks more obvious, as you do not have
to use the uppercase J/K to visit them.

But if one is very used to do this intentionally (as opposed to
"oops, let me take it back"), this would be a usability regression.
"Ah, here is a big hunk with 10 changes, most of which I like, but
one of the lines I do not want to include" in which case I may do
the "Add the hunk to grab 10 changes, visit that decided-to-be-used
hunk, split, and then visit the one minihunk that I want to eject
and say 'n'".  This makes the workflow simpler and more stupid by
requiring the 9 minihunks to be chosen individually after splitting.

So, I dunno.

Re: [PATCH 2/2] add-patch: update hunk splitability after editing

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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 <phillip.wood@dunelm.org.uk>
> ---
>  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

Re: [PATCH 1/2] add -p: mark split hunks as undecided

[phillip.wood123]

Hi Junio and Justin

[I'm replying to both of you via Junio's email as you both raised the 
same question]

On 21/02/2025 21:31, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> When a hunk is split each of the new hunks inherits whether it is
>> selected or not from the original hunk. This means that if a selected
>> hunk is split all of the new hunks are selected and the user is not asked
>> whether or not they want to select the new hunks. This is unfortunate as
>> the user is presumably splitting the original hunk because they only
>> want to select some sub-set of it. Fix this by marking all the new hunks
>> as "undecided" so that we prompt the user to decide whether to select
>> them or not.
> 
> Good.  I am very sure that the design of the current behaviour goes
> back to the very original version of "add -p" with hunk splitting I
> invented; I simply never considered a workflow where people may
> first select and say "oops, let me take it back and redo it".  What
> I am getting at is that I do not think the current behaviour is
> something I designed it to be with too much thought, and debeting if
> it makes sense or it would be better to force them to be undecided
> is probably a good thing to do now.
> 
> Having said that, I have one small concern about forcing them to be
> undecided.  This now allows it to
> 
>   1. Add the whole hunk
>   2. Go back (with K) to that already chosen hunk
>   3. Split
> 
> and makes the resulting minihunks more obvious, as you do not have
> to use the uppercase J/K to visit them.
> 
> But if one is very used to do this intentionally (as opposed to
> "oops, let me take it back"), this would be a usability regression.
> "Ah, here is a big hunk with 10 changes, most of which I like, but
> one of the lines I do not want to include" in which case I may do
> the "Add the hunk to grab 10 changes, visit that decided-to-be-used
> hunk, split, and then visit the one minihunk that I want to eject
> and say 'n'".  This makes the workflow simpler and more stupid by
> requiring the 9 minihunks to be chosen individually after splitting.

If the user wants to deselect the 10th mini-hunk then they have to wade 
through them all with or without this patch. If they want to deselect an 
earlier one then they will now have to do more work.

Currently after a selected hunk is split we always prompt the user to 
make a decision on the first mini-hunk even though it is marked as 
selected when it is split. This seems inconsistent and confused me when 
I first tried splitting a selected hunk which is why I wrote this patch. 
I can see that in some circumstances this patch does make more work for 
the user, but I do think it makes it easier to understand what happens 
when hunk is split.

Best Wishes

Phillip

> So, I dunno.

Re: [PATCH 1/2] add -p: mark split hunks as undecided

[Junio C Hamano]

phillip.wood123@gmail.com writes:

>> "Ah, here is a big hunk with 10 changes, most of which I like, but
>> one of the lines I do not want to include" in which case I may do
>> the "Add the hunk to grab 10 changes, visit that decided-to-be-used
>> hunk, split, and then visit the one minihunk that I want to eject
>> and say 'n'".  This makes the workflow simpler and more stupid by
>> requiring the 9 minihunks to be chosen individually after splitting.
>
> If the user wants to deselect the 10th mini-hunk then they have to
> wade through them all with or without this patch. If they want to
> deselect an earlier one then they will now have to do more work.

Or directly jump to it with "/go-to-the-one-with-this-string"?

> Currently after a selected hunk is split we always prompt the user to
> make a decision on the first mini-hunk even though it is marked as
> selected when it is split. This seems inconsistent and confused me
> when I first tried splitting a selected hunk which is why I wrote this
> patch.

Hmph, so there is an obvious alternative "fix" to the inconsistency,
i.e., after splitting, move to the first unselected hunk?

> I can see that in some circumstances this patch does make more
> work for the user, but I do think it makes it easier to understand
> what happens when hunk is split.

And the alternative may resolve the inconsistency and make it less
work for the users?  I dunno.

This is totally orthogonal to this "split" issue and outside the
scope of this topic, but one thing that I do not like the design of
"add -p", which most likely was inherited from the very initial
iteration before it was rewritten in C, is that we never ask
reconfirmation once all the hunks got decided.  With 3 hunks, after
choosing hunk #1 by mistake, I can still go back and correct the
mistake even if I noticed the mistake after making decision on the
hunk #2 (thanks to the fact that hunk #3 hasn't been decided), but
if the hunk #3 is missing, going back and correcting #1 becomes
impossible as the program exits immediatly after deciding on #2.
But I guess this depends not just on the user but on occasion.
After all, re-running "reset/add -p" after such a mistake is not so
huge a deal anyway.

Thanks.

Re: [PATCH 1/2] add -p: mark split hunks as undecided

[phillip.wood123]

On 26/02/2025 16:49, Junio C Hamano wrote:
> phillip.wood123@gmail.com writes:
> 
>>> "Ah, here is a big hunk with 10 changes, most of which I like, but
>>> one of the lines I do not want to include" in which case I may do
>>> the "Add the hunk to grab 10 changes, visit that decided-to-be-used
>>> hunk, split, and then visit the one minihunk that I want to eject
>>> and say 'n'".  This makes the workflow simpler and more stupid by
>>> requiring the 9 minihunks to be chosen individually after splitting.
>>
>> If the user wants to deselect the 10th mini-hunk then they have to
>> wade through them all with or without this patch. If they want to
>> deselect an earlier one then they will now have to do more work.
> 
> Or directly jump to it with "/go-to-the-one-with-this-string"?

Oh, I'd forgotten '/' searches all the hunks rather than just the 
undecided ones.

>> Currently after a selected hunk is split we always prompt the user to
>> make a decision on the first mini-hunk even though it is marked as
>> selected when it is split. This seems inconsistent and confused me
>> when I first tried splitting a selected hunk which is why I wrote this
>> patch.
> 
> Hmph, so there is an obvious alternative "fix" to the inconsistency,
> i.e., after splitting, move to the first unselected hunk?

We could do that but I think it would be even more confusing than the 
current behavior as it would make it harder to change the state of the 
mini-hunks. At least with the current behavior one can use 'J' to move 
through them immediately after splitting the original hunk. If we move 
to the next undecided hunk one has to know where the newly-created 
mini-hunks are relative to that.

>> I can see that in some circumstances this patch does make more
>> work for the user, but I do think it makes it easier to understand
>> what happens when hunk is split.
> 
> And the alternative may resolve the inconsistency and make it less
> work for the users?  I dunno.

I'm not sure either. I dislike the way it works at the moment and find 
it confusing but if there are a lot of people relying on it then I'd be 
reluctant to change it. Unfortunately we don't have any way to know if 
anyone is relying on the current behavior without changing it and seeing 
if anyone complains. Given it is a bit of a corner case I'm not sure 
whether it is worth spending much more time on it.

> This is totally orthogonal to this "split" issue and outside the
> scope of this topic, but one thing that I do not like the design of
> "add -p", which most likely was inherited from the very initial
> iteration before it was rewritten in C, is that we never ask
> reconfirmation once all the hunks got decided.  With 3 hunks, after
> choosing hunk #1 by mistake, I can still go back and correct the
> mistake even if I noticed the mistake after making decision on the
> hunk #2 (thanks to the fact that hunk #3 hasn't been decided), but
> if the hunk #3 is missing, going back and correcting #1 becomes
> impossible as the program exits immediatly after deciding on #2.
> But I guess this depends not just on the user but on occasion.
> After all, re-running "reset/add -p" after such a mistake is not so
> huge a deal anyway.

I can see the problem and asking for conformation before quitting would 
have been nice if we'd done it from the start. I'm not sure it is worth 
the disruption of changing it when one can re-run "reset/add -p" quite 
easily though. I guess we could add an opt-in cofing that eventually 
becomes the default.

While we're talking about tangential issues it would be nice if when a 
user revisited a hunk we told them its current state. At the moment 
there is no way to tell if a hunk has been selected or not. Related to 
that the help for 'J' and 'K' talk about leaving the current hunk 
undecided when what they actually do is leave the current state unchanged.

Best Wishes

Phillip

Re: [PATCH 1/2] add -p: mark split hunks as undecided

[Junio C Hamano]

phillip.wood123@gmail.com writes:

>>> Currently after a selected hunk is split we always prompt the user to
>>> make a decision on the first mini-hunk even though it is marked as
>>> selected when it is split. This seems inconsistent and confused me
>>> when I first tried splitting a selected hunk which is why I wrote this
>>> patch.
>> Hmph, so there is an obvious alternative "fix" to the inconsistency,
>> i.e., after splitting, move to the first unselected hunk?
>
> We could do that but I think it would be even more confusing than the
> current behavior as it would make it harder to change the state of the
> mini-hunks. At least with the current behavior one can use 'J' to move
> through them immediately after splitting the original hunk. If we move
> to the next undecided hunk one has to know where the newly-created
> mini-hunks are relative to that.

True.  After all, going back to an already selected hunk and then
splitting the hunk is a clear indication that the user wants to
visit some of them to change their state.  Moving them back to
"undecided" (not "deselected") instead of leaving them marked as
"selected" (which is the current behaviour) looks like a better
behaviour and I wish I knew about the possibility in late 2006 when
I added the hunk splitting.

> I'm not sure either. I dislike the way it works at the moment and find
> it confusing but if there are a lot of people relying on it then I'd
> be reluctant to change it.

I share the sentiment, especially the latter.

> Unfortunately we don't have any way to know
> if anyone is relying on the current behavior without changing it and
> seeing if anyone complains. Given it is a bit of a corner case I'm not
> sure whether it is worth spending much more time on it.

Given our user base has grown quite a bit over the years, it almost
is a given that any change to existing behaviour is a regression to
somebody.  Certainly a safe material for Git 3.0 but I do not know
if it is safe enough for 2.50 for example.  The strategy to leave it
longer in 'next' did not work well to catch potential issues for
another topic during this cycle, but we could try it out again.

> I can see the problem and asking for conformation before quitting
> would have been nice if we'd done it from the start. I'm not sure it
> is worth the disruption of changing it when one can re-run "reset/add
> -p" quite easily though.

Yup.  That matches my assessment of it.  I brought it up because I
see this "selection should not stick across splitting" falls into
the same "it would have been nice if it were that way from the
beginning" bucket.

> I guess we could add an opt-in cofing that
> eventually becomes the default.

I'd prefer not to add configuration for tweaking such a small thing
(this applies to "should selection stick across splitting?", too).

> While we're talking about tangential issues it would be nice if when a
> user revisited a hunk we told them its current state. At the moment
> there is no way to tell if a hunk has been selected or not.

The user came back with 'J' or 'K' probably because the hunk was
skipped in their earlier navigation with 'k' or 'j', so users may be
using it as a workaround, but I agree there should be an indicator
for the (unselected, selected, undecided).

> Related to that the help for 'J' and 'K' talk about leaving the
> current hunk undecided when what they actually do is leave the
> current state unchanged.

Nice catch.

Thanks.

Re: [PATCH 1/2] add -p: mark split hunks as undecided

[Phillip Wood]

On 27/02/2025 18:36, Junio C Hamano wrote:
> phillip.wood123@gmail.com writes:
> 
>>>> Currently after a selected hunk is split we always prompt the user to
>>>> make a decision on the first mini-hunk even though it is marked as
>>>> selected when it is split. This seems inconsistent and confused me
>>>> when I first tried splitting a selected hunk which is why I wrote this
>>>> patch.
>>> Hmph, so there is an obvious alternative "fix" to the inconsistency,
>>> i.e., after splitting, move to the first unselected hunk?
>>
>> We could do that but I think it would be even more confusing than the
>> current behavior as it would make it harder to change the state of the
>> mini-hunks. At least with the current behavior one can use 'J' to move
>> through them immediately after splitting the original hunk. If we move
>> to the next undecided hunk one has to know where the newly-created
>> mini-hunks are relative to that.
> 
> True.  After all, going back to an already selected hunk and then
> splitting the hunk is a clear indication that the user wants to
> visit some of them to change their state.  Moving them back to
> "undecided" (not "deselected") instead of leaving them marked as
> "selected" (which is the current behaviour) looks like a better
> behaviour and I wish I knew about the possibility in late 2006 when
> I added the hunk splitting.
> 
>> I'm not sure either. I dislike the way it works at the moment and find
>> it confusing but if there are a lot of people relying on it then I'd
>> be reluctant to change it.
> 
> I share the sentiment, especially the latter.
> 
>> Unfortunately we don't have any way to know
>> if anyone is relying on the current behavior without changing it and
>> seeing if anyone complains. Given it is a bit of a corner case I'm not
>> sure whether it is worth spending much more time on it.
> 
> Given our user base has grown quite a bit over the years, it almost
> is a given that any change to existing behaviour is a regression to
> somebody.  Certainly a safe material for Git 3.0 but I do not know
> if it is safe enough for 2.50 for example.  The strategy to leave it
> longer in 'next' did not work well to catch potential issues for
> another topic during this cycle, but we could try it out again.

I'll drop this patch for now. There was some talk a while ago about 
adding a mechanism to select "git 3.0" features at build or run time. If 
we add something like that I'll resubmit with this change guarded by 
that feature.

>> I can see the problem and asking for conformation before quitting
>> would have been nice if we'd done it from the start. I'm not sure it
>> is worth the disruption of changing it when one can re-run "reset/add
>> -p" quite easily though.
> 
> Yup.  That matches my assessment of it.  I brought it up because I
> see this "selection should not stick across splitting" falls into
> the same "it would have been nice if it were that way from the
> beginning" bucket.
> 
>> I guess we could add an opt-in cofing that
>> eventually becomes the default.
> 
> I'd prefer not to add configuration for tweaking such a small thing
> (this applies to "should selection stick across splitting?", too).

Perhaps we should make the confirm-before-quitting thing a "git 3.0" 
feature as well?

Best Wishes

Phillip

>> While we're talking about tangential issues it would be nice if when a
>> user revisited a hunk we told them its current state. At the moment
>> there is no way to tell if a hunk has been selected or not.
> 
> The user came back with 'J' or 'K' probably because the hunk was
> skipped in their earlier navigation with 'k' or 'j', so users may be
> using it as a workaround, but I agree there should be an indicator
> for the (unselected, selected, undecided).
> 
>> Related to that the help for 'J' and 'K' talk about leaving the
>> current hunk undecided when what they actually do is leave the
>> current state unchanged.
> 
> Nice catch.
> 
> Thanks.

Re: [PATCH 1/2] add -p: mark split hunks as undecided

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> ... There was some talk a while ago about
> adding a mechanism to select "git 3.0" features at build or run
> time. If we add something like that I'll resubmit with this change
> guarded by that feature.

Documentation/BreakingChanges says that we can hide it behind
WITH_BREAKING_CHANGES compile-time switch, and that is part of
2.49-rc0 already.  The linux-breaking-changes GitHub Actions CI job
runs with it defined.

> Perhaps we should make the confirm-before-quitting thing a "git 3.0"
> feature as well?

I do not feel too strongly either way.  Sometimes I wish it asked
for the final confirmation after all hunks are decided.  Most of the
time I do not feel that way, which almost always is after saying 'q'
to finish the selection.  So I dunno, but my thinking right now is
that I lean a bit toward negative than positive.

In any case, I think we should indicate the (selected, deselected,
undecided) for the current hunk the user is being asked about, which
we talked about. As a workaround, we can do 'g' command to see the
list of hunks and check the indicator (+/ /-) for each hunk.

Re: [PATCH 1/2] add -p: mark split hunks as undecided

[Phillip Wood]

Hi Junio

On 28/02/2025 17:06, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> ... There was some talk a while ago about
>> adding a mechanism to select "git 3.0" features at build or run
>> time. If we add something like that I'll resubmit with this change
>> guarded by that feature.
> 
> Documentation/BreakingChanges says that we can hide it behind
> WITH_BREAKING_CHANGES compile-time switch, and that is part of
> 2.49-rc0 already.  The linux-breaking-changes GitHub Actions CI job
> runs with it defined.

Thanks I'd missed that being merged. I'll re-roll with the changes in 
this patch guarded by WITH_BREAKING_CHANGES.

>> Perhaps we should make the confirm-before-quitting thing a "git 3.0"
>> feature as well?
> 
> I do not feel too strongly either way.  Sometimes I wish it asked
> for the final confirmation after all hunks are decided.  Most of the
> time I do not feel that way, which almost always is after saying 'q'
> to finish the selection.  So I dunno, but my thinking right now is
> that I lean a bit toward negative than positive.
> 
> In any case, I think we should indicate the (selected, deselected,
> undecided) for the current hunk the user is being asked about, which
> we talked about. As a workaround, we can do 'g' command to see the
> list of hunks and check the indicator (+/ /-) for each hunk.

I'll try and take a look at that in the next release cycle

Best Wishes

Phillip

[PATCH v2 0/2] add -p: a couple of hunk splitting fixes

[Phillip Wood via GitGitGadget]

Thanks to Justin and Junio for their comments in V1. Sorry for the long
delay in re-rolling - I thought I'd sent these ages ago and then discovered
that they weren't upstream and realized I had not, in fact, sent them
after-all.

Changes since V1:

 * Patch 1: The new hunks created by splitting a hunk are now only marked as
   "undecided" when WITH_BREAKING_CHANGES is enabled.

 * Patch 2: Reworded commit message and added a space before a redirection
   in the test

V1 Cover Letter:

This series fixes a couple of infelicities when splitting hunks that have
already been selected or edited which I noticed a while ago when preparing
the test for 'pw/add-patch-with-suppress-blank-empty'.

Phillip Wood (2):
  add -p: mark split hunks as undecided
  add-patch: update hunk splitability after editing

 Documentation/BreakingChanges.adoc |  5 +++++
 add-patch.c                        | 19 +++++++++++++++-
 t/t3701-add-interactive.sh         | 36 ++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 1 deletion(-)


base-commit: 4975ec3473b4bc61bc8a3df1ef29d0b7e7959e87
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1863%2Fphillipwood%2Fadd-p-split-hunks-are-undecided-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1863/phillipwood/add-p-split-hunks-are-undecided-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1863

Range-diff vs v1:

 1:  43a0592a46 ! 1:  3e2ec7b37f add -p: mark split hunks as undecided
     @@ Metadata
       ## Commit message ##
          add -p: mark split hunks as undecided
      
     -    When a hunk is split each of the new hunks inherits whether it is
     -    selected or not from the original hunk. This means that if a selected
     -    hunk is split all of the new hunks are selected and the user is not asked
     -    whether or not they want to select the new hunks. This is unfortunate as
     -    the user is presumably splitting the original hunk because they only
     -    want to select some sub-set of it. Fix this by marking all the new hunks
     -    as "undecided" so that we prompt the user to decide whether to select
     -    them or not.
     +    When a hunk is split, each of the new hunks inherits whether it is
     +    selected or not from the original hunk. If a selected hunk is split
     +    all of the new hunks are marked as "selected" and the user is only
     +    prompted with the first of the split hunks. The user is not asked
     +    whether or not they wish to select the rest of the new hunks. This
     +    means that if they wish to deselect any of the new hunks apart from
     +    the first one they have to navigate back to the hunk they want to
     +    deselect before they can deselect it. This is unfortunate as the user
     +    is presumably splitting the original hunk because they only want to
     +    select some sub-set of it.
     +
     +    Instead mark all the new hunks as "undecided" so that the user is
     +    prompted whether they wish to select each one in turn. In the case
     +    where the user only wants to change the selection of the first of
     +    the split hunks they will now have to do more work re-selecting the
     +    remaining split hunks. However, changing the selection of any of the
     +    other newly created hunks is now much simpler as the user no-longer has
     +    to navigate back to them in order to change their selected state. Due
     +    to concerns that users may be relying on the current behaviour [1]
     +    this change is guarded by WITH_BREAKING_CHANGES.
     +
     +    [1] https://lore.kernel.org/git/xmqqjz9b6xr1.fsf@gitster.g
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
     + ## Documentation/BreakingChanges.adoc ##
     +@@ Documentation/BreakingChanges.adoc: A prerequisite for this change is that the ecosystem is ready to support the
     + "reftable" format. Most importantly, alternative implementations of Git like
     + JGit, libgit2 and Gitoxide need to support it.
     + 
     ++* The behavior of "git add -p" has been changed so that splitting a
     ++  hunk that has already been marked as selected or unselected will now
     ++  prompt the user to select each of the new hunks created by the
     ++  split instead of them inheriting their state from the original hunk.
     ++
     + === Removals
     + 
     + * Support for grafting commits has long been superseded by git-replace(1).
     +
       ## add-patch.c ##
      @@ add-patch.c: static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
       			* sizeof(*hunk));
       	hunk = file_diff->hunk + hunk_index;
       	hunk->splittable_into = 1;
     ++#ifdef WITH_BREAKING_CHANGES
      +	hunk->use = UNDECIDED_HUNK;
     ++#endif
       	memset(hunk + 1, 0, (splittable_into - 1) * sizeof(*hunk));
       
       	header = &hunk->header;
     @@ add-patch.c: next_hunk_line:
       
       		hunk++;
       		hunk->splittable_into = 1;
     --		hunk->use = hunk[-1].use;
     ++#ifdef WITH_BREAKING_CHANGES
      +		hunk->use = UNDECIDED_HUNK;
     ++#else
     + 		hunk->use = hunk[-1].use;
     ++#endif
       		header = &hunk->header;
       
       		header->old_count = header->new_count = context_line_count;
      
       ## t/t3701-add-interactive.sh ##
     -@@ t/t3701-add-interactive.sh: test_expect_success 'hunk splitting works with diff.suppressBlankEmpty' '
     - 	test_cmp expect actual
     - '
     +@@ t/t3701-add-interactive.sh: do
     + 	'
     + done
       
     -+test_expect_success 'splitting previous hunk marks split hunks as undecided' '
     ++test_expect_success WITH_BREAKING_CHANGES 'splitting previous hunk marks split hunks as undecided' '
      +	test_write_lines a " " b c d e f g h i j k >file &&
      +	git add file &&
      +	test_write_lines x " " b y d e f g h i j x >file &&
 2:  35ef0ee2b9 ! 2:  3a831b1a2d add-patch: update hunk splitability after editing
     @@ Metadata
       ## Commit message ##
          add-patch: update hunk splitability after editing
      
     -    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, when the user edits a hunk, they change deletion lines into
     +    context lines or vice versa, then the number of hunks that the edited
     +    hunk can be split into may differ from the unedited hunk. This means
     +    that so we should recalculate `hunk->splittable_into` after the hunk
     +    has been edited. 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.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
     @@ add-patch.c: static ssize_t recount_edited_hunk(struct add_p_state *s, struct hu
       
      
       ## t/t3701-add-interactive.sh ##
     -@@ t/t3701-add-interactive.sh: test_expect_success 'splitting previous hunk marks split hunks as undecided' '
     +@@ t/t3701-add-interactive.sh: test_expect_success WITH_BREAKING_CHANGES 'splitting previous hunk marks split h
       	test_cmp expect actual
       '
       
     @@ t/t3701-add-interactive.sh: test_expect_success 'splitting previous hunk marks s
      +	mv "$1.tmp" "$1"
      +	EOF
      +
     -+	test_write_lines a b c d e f g h i j k l m n>file &&
     ++	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 j y n y q | git add -p file
     ++		if test_have_prereq WITH_BREAKING_CHANGES
     ++		then
     ++			test_write_lines e K s j y n y q
     ++		else
     ++			test_write_lines e K s n K n y q
     ++		fi | 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 &&

-- 
gitgitgadget

[PATCH v2 1/2] add -p: mark split hunks as undecided

[Phillip Wood via GitGitGadget]

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

When a hunk is split, each of the new hunks inherits whether it is
selected or not from the original hunk. If a selected hunk is split
all of the new hunks are marked as "selected" and the user is only
prompted with the first of the split hunks. The user is not asked
whether or not they wish to select the rest of the new hunks. This
means that if they wish to deselect any of the new hunks apart from
the first one they have to navigate back to the hunk they want to
deselect before they can deselect it. This is unfortunate as the user
is presumably splitting the original hunk because they only want to
select some sub-set of it.

Instead mark all the new hunks as "undecided" so that the user is
prompted whether they wish to select each one in turn. In the case
where the user only wants to change the selection of the first of
the split hunks they will now have to do more work re-selecting the
remaining split hunks. However, changing the selection of any of the
other newly created hunks is now much simpler as the user no-longer has
to navigate back to them in order to change their selected state. Due
to concerns that users may be relying on the current behaviour [1]
this change is guarded by WITH_BREAKING_CHANGES.

[1] https://lore.kernel.org/git/xmqqjz9b6xr1.fsf@gitster.g

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/BreakingChanges.adoc |  5 +++++
 add-patch.c                        |  7 +++++++
 t/t3701-add-interactive.sh         | 10 ++++++++++
 3 files changed, 22 insertions(+)

diff --git a/Documentation/BreakingChanges.adoc b/Documentation/BreakingChanges.adoc
index f8d2eba061..f62f75898f 100644
--- a/Documentation/BreakingChanges.adoc
+++ b/Documentation/BreakingChanges.adoc
@@ -165,6 +165,11 @@ A prerequisite for this change is that the ecosystem is ready to support the
 "reftable" format. Most importantly, alternative implementations of Git like
 JGit, libgit2 and Gitoxide need to support it.
 
+* The behavior of "git add -p" has been changed so that splitting a
+  hunk that has already been marked as selected or unselected will now
+  prompt the user to select each of the new hunks created by the
+  split instead of them inheriting their state from the original hunk.
+
 === Removals
 
 * Support for grafting commits has long been superseded by git-replace(1).
diff --git a/add-patch.c b/add-patch.c
index 302e6ba7d9..32157e31ed 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -956,6 +956,9 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
 			* sizeof(*hunk));
 	hunk = file_diff->hunk + hunk_index;
 	hunk->splittable_into = 1;
+#ifdef WITH_BREAKING_CHANGES
+	hunk->use = UNDECIDED_HUNK;
+#endif
 	memset(hunk + 1, 0, (splittable_into - 1) * sizeof(*hunk));
 
 	header = &hunk->header;
@@ -1057,7 +1060,11 @@ next_hunk_line:
 
 		hunk++;
 		hunk->splittable_into = 1;
+#ifdef WITH_BREAKING_CHANGES
+		hunk->use = UNDECIDED_HUNK;
+#else
 		hunk->use = hunk[-1].use;
+#endif
 		header = &hunk->header;
 
 		header->old_count = header->new_count = context_line_count;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 04d2a19835..43a856e0c0 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1301,4 +1301,14 @@ do
 	'
 done
 
+test_expect_success WITH_BREAKING_CHANGES 'splitting previous hunk marks split hunks as undecided' '
+	test_write_lines a " " b c d e f g h i j k >file &&
+	git add file &&
+	test_write_lines x " " b y d e f g h i j x >file &&
+	test_write_lines n K s n y q | git add -p file &&
+	git cat-file blob :file >actual &&
+	test_write_lines a " " b y d e f g h i j k >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
gitgitgadget

[PATCH v2 2/2] add-patch: update hunk splitability after editing

[Phillip Wood via GitGitGadget]

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

If, when the user edits a hunk, they change deletion lines into
context lines or vice versa, then the number of hunks that the edited
hunk can be split into may differ from the unedited hunk. This means
that so we should recalculate `hunk->splittable_into` after the hunk
has been edited. 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.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 add-patch.c                | 12 +++++++++++-
 t/t3701-add-interactive.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/add-patch.c b/add-patch.c
index 32157e31ed..0754f54a38 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1191,19 +1191,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;
 		}
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 43a856e0c0..77f99e9ecb 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1311,4 +1311,30 @@ test_expect_success WITH_BREAKING_CHANGES 'splitting previous hunk marks split h
 	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" &&
+		if test_have_prereq WITH_BREAKING_CHANGES
+		then
+			test_write_lines e K s j y n y q
+		else
+			test_write_lines e K s n K n y q
+		fi | 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
-- 
gitgitgadget

Re: [PATCH v2 1/2] add -p: mark split hunks as undecided

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When a hunk is split, each of the new hunks inherits whether it is
> selected or not from the original hunk. If a selected hunk is split
> all of the new hunks are marked as "selected" and the user is only
> prompted with the first of the split hunks. The user is not asked
> whether or not they wish to select the rest of the new hunks. This
> means that if they wish to deselect any of the new hunks apart from
> the first one they have to navigate back to the hunk they want to
> deselect before they can deselect it. This is unfortunate as the user
> is presumably splitting the original hunk because they only want to
> select some sub-set of it.
>
> Instead mark all the new hunks as "undecided" so that the user is
> prompted whether they wish to select each one in turn. In the case
> where the user only wants to change the selection of the first of
> the split hunks they will now have to do more work re-selecting the
> remaining split hunks. However, changing the selection of any of the
> other newly created hunks is now much simpler as the user no-longer has
> to navigate back to them in order to change their selected state.

That is great, but ...

> Due
> to concerns that users may be relying on the current behaviour [1]
> this change is guarded by WITH_BREAKING_CHANGES.

... this does not really sound like a good candidate for "before
version X we used to do this, but after X we no longer do so".

Unless it is a mere bugfix, in which case such a change does not
deserve a huge-version-bump switchover like this.

On the other hand, assuming it is not a bugfix but introducing a
different behaviour, where both the original and the new ones are
useful depending on the situation, wouldn't it be better to give
users choices at runtime instead, with a configuration variable at
least, but possibly with a interactive command to choose which
behaviour is used on demand?

Re: [PATCH v2 1/2] add -p: mark split hunks as undecided

[Phillip Wood]

Hi Junio

On 15/09/2025 18:44, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> When a hunk is split, each of the new hunks inherits whether it is
>> selected or not from the original hunk. If a selected hunk is split
>> all of the new hunks are marked as "selected" and the user is only
>> prompted with the first of the split hunks. The user is not asked
>> whether or not they wish to select the rest of the new hunks. This
>> means that if they wish to deselect any of the new hunks apart from
>> the first one they have to navigate back to the hunk they want to
>> deselect before they can deselect it. This is unfortunate as the user
>> is presumably splitting the original hunk because they only want to
>> select some sub-set of it.
>>
>> Instead mark all the new hunks as "undecided" so that the user is
>> prompted whether they wish to select each one in turn. In the case
>> where the user only wants to change the selection of the first of
>> the split hunks they will now have to do more work re-selecting the
>> remaining split hunks. However, changing the selection of any of the
>> other newly created hunks is now much simpler as the user no-longer has
>> to navigate back to them in order to change their selected state.
> 
> That is great, but ...
> 
>> Due
>> to concerns that users may be relying on the current behaviour [1]
>> this change is guarded by WITH_BREAKING_CHANGES.
> 
> ... this does not really sound like a good candidate for "before
> version X we used to do this, but after X we no longer do so".
> 
> Unless it is a mere bugfix, in which case such a change does not
> deserve a huge-version-bump switchover like this.

To me, the current behavior is strange enough to be considered a bug but 
when we discussed this before you were not so sure and said [1]

     Given our user base has grown quite a bit over the years, it
     almost is a given that any change to existing behaviour is a
     regression to somebody.  Certainly a safe material for Git 3.0
     but I do not know if it is safe enough for 2.50 for example.
     The strategy to leave it longer in 'next' did not work well
     to catch potential issues for another topic during this cycle,
     but we could try it out again.

So I re-rolled using WITH_BREAKING_CHANGES. If you're happy just to 
change the behavior unconditionally I can go back to V1

> On the other hand, assuming it is not a bugfix but introducing a
> different behaviour, where both the original and the new ones are
> useful depending on the situation, wouldn't it be better to give
> users choices at runtime instead, with a configuration variable at
> least, but possibly with a interactive command to choose which
> behaviour is used on demand?

I'm not really convinced the current behavior is that useful and it is 
such an esoteric use of 'add -p' that I'm not sure it makes sense to add 
a config setting for it.

Thanks

Phillip

[1] https://lore.kernel.org/git/xmqqjz9b6xr1.fsf@gitster.g/

Re: [PATCH v2 1/2] add -p: mark split hunks as undecided

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> To me, the current behavior is strange enough to be considered a bug
> but when we discussed this before you were not so sure and said [1]
>
>     Given our user base has grown quite a bit over the years, it
>     almost is a given that any change to existing behaviour is a
>     regression to somebody.  Certainly a safe material for Git 3.0
>     but I do not know if it is safe enough for 2.50 for example.
>     The strategy to leave it longer in 'next' did not work well
>     to catch potential issues for another topic during this cycle,
>     but we could try it out again.
>
> So I re-rolled using WITH_BREAKING_CHANGES. If you're happy just to
> change the behavior unconditionally I can go back to V1

I am *NOT* happy with breaking changes.

Either you cut off those who you do not believe exist because the
behaviour is so strange and useless (i.e. immediate "this is a
bugfix, there is no mechanism to keep the broken behaviour even for
a short period"), which is much better than that from my eyes if I
were to be convinced that the old behaviour is nonsense, or you
support them with configuration without any future deprecation,
which is also fine by me.

The breaking changes mechanism is to be used only when we are
convinced that the old way is nonsense, we are fairly sure that no
sane user is using it, and we have concensus that the old way MUST
be abandoned and any existing users MUST be retrained to use the new
ways, and only to buy/give some time to these users that needs
retraining.

Have we got that firm sense in the community that the old behaviour
qualifies for breaking change mechanism?  I don't, at least not yet.

>> On the other hand, assuming it is not a bugfix but introducing a
>> different behaviour, where both the original and the new ones are
>> useful depending on the situation, wouldn't it be better to give
>> users choices at runtime instead, with a configuration variable at
>> least, but possibly with a interactive command to choose which
>> behaviour is used on demand?
>
> I'm not really convinced the current behavior is that useful and it is
> such an esoteric use of 'add -p' that I'm not sure it makes sense to
> add a config setting for it.

If so, let's break it first and see if anybody screams.  Touching
the breaking changes mechanism is way too early before doing that, I
think.

[PATCH v3 0/2] add -p: a couple of hunk splitting fixes

[Phillip Wood via GitGitGadget]

Thanks to Junio for his comments on V2. I have removed the dependency on
WITH_BREAKING_CHANGES in favor of "lets change it and see if anyone screams"

Changes since V2: Remove dependency on WITH_BREAKING_CHANGES and change the
behavior unconditionally. This takes us back to V1 with (hopefully) better
commit messages and a style fix in the tests.

V2 Cover Letter: Thanks to Justin and Junio for their comments in V1. Sorry
for the long delay in re-rolling - I thought I'd sent these ages ago and
then discovered that they weren't upstream and realized I had not, in fact,
sent them after-all.

Changes since V1:

 * Patch 1: The new hunks created by splitting a hunk are now only marked as
   "undecided" when WITH_BREAKING_CHANGES is enabled.

 * Patch 2: Reworded commit message and added a space before a redirection
   in the test

V1 Cover Letter:

This series fixes a couple of infelicities when splitting hunks that have
already been selected or edited which I noticed a while ago when preparing
the test for 'pw/add-patch-with-suppress-blank-empty'.

Phillip Wood (2):
  add -p: mark split hunks as undecided
  add-patch: update hunk splitability after editing

 add-patch.c                | 15 +++++++++++++--
 t/t3701-add-interactive.sh | 31 +++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 2 deletions(-)


base-commit: 4975ec3473b4bc61bc8a3df1ef29d0b7e7959e87
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1863%2Fphillipwood%2Fadd-p-split-hunks-are-undecided-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1863/phillipwood/add-p-split-hunks-are-undecided-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1863

Range-diff vs v2:

 1:  3e2ec7b37f ! 1:  4935dde399 add -p: mark split hunks as undecided
     @@ Commit message
          the split hunks they will now have to do more work re-selecting the
          remaining split hunks. However, changing the selection of any of the
          other newly created hunks is now much simpler as the user no-longer has
     -    to navigate back to them in order to change their selected state. Due
     -    to concerns that users may be relying on the current behaviour [1]
     -    this change is guarded by WITH_BREAKING_CHANGES.
     -
     -    [1] https://lore.kernel.org/git/xmqqjz9b6xr1.fsf@gitster.g
     +    to navigate back to them in order to change their selected state.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
     - ## Documentation/BreakingChanges.adoc ##
     -@@ Documentation/BreakingChanges.adoc: A prerequisite for this change is that the ecosystem is ready to support the
     - "reftable" format. Most importantly, alternative implementations of Git like
     - JGit, libgit2 and Gitoxide need to support it.
     - 
     -+* The behavior of "git add -p" has been changed so that splitting a
     -+  hunk that has already been marked as selected or unselected will now
     -+  prompt the user to select each of the new hunks created by the
     -+  split instead of them inheriting their state from the original hunk.
     -+
     - === Removals
     - 
     - * Support for grafting commits has long been superseded by git-replace(1).
     -
       ## add-patch.c ##
      @@ add-patch.c: static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
       			* sizeof(*hunk));
       	hunk = file_diff->hunk + hunk_index;
       	hunk->splittable_into = 1;
     -+#ifdef WITH_BREAKING_CHANGES
      +	hunk->use = UNDECIDED_HUNK;
     -+#endif
       	memset(hunk + 1, 0, (splittable_into - 1) * sizeof(*hunk));
       
       	header = &hunk->header;
     @@ add-patch.c: next_hunk_line:
       
       		hunk++;
       		hunk->splittable_into = 1;
     -+#ifdef WITH_BREAKING_CHANGES
     +-		hunk->use = hunk[-1].use;
      +		hunk->use = UNDECIDED_HUNK;
     -+#else
     - 		hunk->use = hunk[-1].use;
     -+#endif
       		header = &hunk->header;
       
       		header->old_count = header->new_count = context_line_count;
     @@ t/t3701-add-interactive.sh: do
       	'
       done
       
     -+test_expect_success WITH_BREAKING_CHANGES 'splitting previous hunk marks split hunks as undecided' '
     ++test_expect_success 'splitting previous hunk marks split hunks as undecided' '
      +	test_write_lines a " " b c d e f g h i j k >file &&
      +	git add file &&
      +	test_write_lines x " " b y d e f g h i j x >file &&
 2:  3a831b1a2d ! 2:  390686dbb3 add-patch: update hunk splitability after editing
     @@ add-patch.c: static ssize_t recount_edited_hunk(struct add_p_state *s, struct hu
       
      
       ## t/t3701-add-interactive.sh ##
     -@@ t/t3701-add-interactive.sh: test_expect_success WITH_BREAKING_CHANGES 'splitting previous hunk marks split h
     +@@ t/t3701-add-interactive.sh: test_expect_success 'splitting previous hunk marks split hunks as undecided' '
       	test_cmp expect actual
       '
       
     @@ t/t3701-add-interactive.sh: test_expect_success WITH_BREAKING_CHANGES 'splitting
      +	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" &&
     -+		if test_have_prereq WITH_BREAKING_CHANGES
     -+		then
     -+			test_write_lines e K s j y n y q
     -+		else
     -+			test_write_lines e K s n K n y q
     -+		fi | git add -p file
     ++		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 &&

-- 
gitgitgadget

[PATCH v3 1/2] add -p: mark split hunks as undecided

[Phillip Wood via GitGitGadget]

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

When a hunk is split, each of the new hunks inherits whether it is
selected or not from the original hunk. If a selected hunk is split
all of the new hunks are marked as "selected" and the user is only
prompted with the first of the split hunks. The user is not asked
whether or not they wish to select the rest of the new hunks. This
means that if they wish to deselect any of the new hunks apart from
the first one they have to navigate back to the hunk they want to
deselect before they can deselect it. This is unfortunate as the user
is presumably splitting the original hunk because they only want to
select some sub-set of it.

Instead mark all the new hunks as "undecided" so that the user is
prompted whether they wish to select each one in turn. In the case
where the user only wants to change the selection of the first of
the split hunks they will now have to do more work re-selecting the
remaining split hunks. However, changing the selection of any of the
other newly created hunks is now much simpler as the user no-longer has
to navigate back to them in order to change their selected state.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 add-patch.c                |  3 ++-
 t/t3701-add-interactive.sh | 10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/add-patch.c b/add-patch.c
index 302e6ba7d9..61f42de9ea 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -956,6 +956,7 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
 			* sizeof(*hunk));
 	hunk = file_diff->hunk + hunk_index;
 	hunk->splittable_into = 1;
+	hunk->use = UNDECIDED_HUNK;
 	memset(hunk + 1, 0, (splittable_into - 1) * sizeof(*hunk));
 
 	header = &hunk->header;
@@ -1057,7 +1058,7 @@ next_hunk_line:
 
 		hunk++;
 		hunk->splittable_into = 1;
-		hunk->use = hunk[-1].use;
+		hunk->use = UNDECIDED_HUNK;
 		header = &hunk->header;
 
 		header->old_count = header->new_count = context_line_count;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 04d2a19835..a6829fd085 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1301,4 +1301,14 @@ do
 	'
 done
 
+test_expect_success 'splitting previous hunk marks split hunks as undecided' '
+	test_write_lines a " " b c d e f g h i j k >file &&
+	git add file &&
+	test_write_lines x " " b y d e f g h i j x >file &&
+	test_write_lines n K s n y q | git add -p file &&
+	git cat-file blob :file >actual &&
+	test_write_lines a " " b y d e f g h i j k >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
gitgitgadget

[PATCH v3 2/2] add-patch: update hunk splitability after editing

[Phillip Wood via GitGitGadget]

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

If, when the user edits a hunk, they change deletion lines into
context lines or vice versa, then the number of hunks that the edited
hunk can be split into may differ from the unedited hunk. This means
that so we should recalculate `hunk->splittable_into` after the hunk
has been edited. 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.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 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 61f42de9ea..bcc2d7666f 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1185,19 +1185,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;
 		}
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index a6829fd085..13739a4582 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1311,4 +1311,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 &&
+	(
+		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
-- 
gitgitgadget

Re: [PATCH v3 1/2] add -p: mark split hunks as undecided

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> ... In the case
> where the user only wants to change the selection of the first of
> the split hunks they will now have to do more work re-selecting the
> remaining split hunks.

In general, that is not just "first", as you can jump ahead, or just
keep typing "J" which is not all that hard, until you find the hunk
you are looking for, at which point you flip its status.  So I would
say that it is more work if you realized a small part of the change
needs to be deselected after you selected a big hunk, and want to
recover by splitting this already selected big hunk and selectively
deselect these small number of subhunks.

If you explicitly decided not to use a hunk (instead of leaving it
undecided) and then chose to split it, in order to resurrect only a
few part, the end-user experience would be affected equally by this
change.  The user is shown each one of them and needs to select.

> However, changing the selection of any of the
> other newly created hunks is now much simpler as the user no-longer has
> to navigate back to them in order to change their selected state.

So I do not exactly buy the above argument.

The traditional rule that applies when you split a hunk you have
already decided what to do with, these hunks will remember your
earlier decision so that you can selectively change them by going
through them.  The new rule says that by splitting an already
decided hunk, you are discarding your earlier decision and redoing
the selection from scratch.

But I think the new behaviour shines in one very important point.

When you are looking at a hunk, you cannot easily tell if you
already chose to use the hunk or not.  So if you select a hunk,
navigate back with "K", and then split it with "s", you may be
seeing the first minihunk.  You cannot tell if that is already
selected, or you need to say "y" to select it.  Or if you need to
say "n" to deselect it, or simply saying "j" is enough.

And the new behaviour sidesteps that particular usability glitch of
the selection UI.  By simplifying the rule to say that splitting
will cancel all the previous decisions you made on the minihunks
created out of the hunk, things will become much simpler.  You'll be
able to view all these minihunks again and choose their fate afresh.
You do not have to remember (and this is the hard part---there is no
strong indication of the current selected/deselected/undecided
status, unless you remember you came with "j/k" not "J/K" in which
case you are looking at an undecided one) what you decided to do
earlier when you look at each hunk, and the split hunks remembering
your earlier decision does get in your way navigating.

So I like the updated behaviour very much, but I am reluctant to
pretend as if we are siding one camps of folks who think that
splitting a selected hunk is done with an intention to deselect most
of the minihunks most of the time, playing favors.  I think that
is a wrong way to frame the problem this patch solved.

In any case, I no longer have problems with the updated behaviour
with these two patches.  Thanks for working on them.

Will queue.



> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  add-patch.c                |  3 ++-
>  t/t3701-add-interactive.sh | 10 ++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index 302e6ba7d9..61f42de9ea 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -956,6 +956,7 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
>  			* sizeof(*hunk));
>  	hunk = file_diff->hunk + hunk_index;
>  	hunk->splittable_into = 1;
> +	hunk->use = UNDECIDED_HUNK;
>  	memset(hunk + 1, 0, (splittable_into - 1) * sizeof(*hunk));
>  
>  	header = &hunk->header;
> @@ -1057,7 +1058,7 @@ next_hunk_line:
>  
>  		hunk++;
>  		hunk->splittable_into = 1;
> -		hunk->use = hunk[-1].use;
> +		hunk->use = UNDECIDED_HUNK;
>  		header = &hunk->header;
>  
>  		header->old_count = header->new_count = context_line_count;
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 04d2a19835..a6829fd085 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -1301,4 +1301,14 @@ do
>  	'
>  done
>  
> +test_expect_success 'splitting previous hunk marks split hunks as undecided' '
> +	test_write_lines a " " b c d e f g h i j k >file &&
> +	git add file &&
> +	test_write_lines x " " b y d e f g h i j x >file &&
> +	test_write_lines n K s n y q | git add -p file &&
> +	git cat-file blob :file >actual &&
> +	test_write_lines a " " b y d e f g h i j k >expect &&
> +	test_cmp expect actual
> +'
> +
>  test_done

Re: [PATCH v3 1/2] add -p: mark split hunks as undecided

[Phillip Wood]

Hi Junio

On 25/09/2025 19:21, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> ... In the case
>> where the user only wants to change the selection of the first of
>> the split hunks they will now have to do more work re-selecting the
>> remaining split hunks.
> 
> In general, that is not just "first", as you can jump ahead, or just
> keep typing "J" which is not all that hard, until you find the hunk
> you are looking for, at which point you flip its status.

Fair enough, I think that only applies to a single split hunk though. 
One you select or deselect it you can't walk though the remaining split 
hunks J anymore because you'll have been dumped at the next undecided 
hunk after entering 'y' or 'n'

> [...]
> So I like the updated behaviour very much, but I am reluctant to
> pretend as if we are siding one camps of folks who think that
> splitting a selected hunk is done with an intention to deselect most
> of the minihunks most of the time, playing favors.  I think that
> is a wrong way to frame the problem this patch solved.
> 
> In any case, I no longer have problems with the updated behaviour
> with these two patches.  Thanks for working on them.
> 
> Will queue.

Thanks, do you want a different commit message or are you happy to take 
them as-is?

Phillip

> 
> 
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   add-patch.c                |  3 ++-
>>   t/t3701-add-interactive.sh | 10 ++++++++++
>>   2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/add-patch.c b/add-patch.c
>> index 302e6ba7d9..61f42de9ea 100644
>> --- a/add-patch.c
>> +++ b/add-patch.c
>> @@ -956,6 +956,7 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
>>   			* sizeof(*hunk));
>>   	hunk = file_diff->hunk + hunk_index;
>>   	hunk->splittable_into = 1;
>> +	hunk->use = UNDECIDED_HUNK;
>>   	memset(hunk + 1, 0, (splittable_into - 1) * sizeof(*hunk));
>>   
>>   	header = &hunk->header;
>> @@ -1057,7 +1058,7 @@ next_hunk_line:
>>   
>>   		hunk++;
>>   		hunk->splittable_into = 1;
>> -		hunk->use = hunk[-1].use;
>> +		hunk->use = UNDECIDED_HUNK;
>>   		header = &hunk->header;
>>   
>>   		header->old_count = header->new_count = context_line_count;
>> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
>> index 04d2a19835..a6829fd085 100755
>> --- a/t/t3701-add-interactive.sh
>> +++ b/t/t3701-add-interactive.sh
>> @@ -1301,4 +1301,14 @@ do
>>   	'
>>   done
>>   
>> +test_expect_success 'splitting previous hunk marks split hunks as undecided' '
>> +	test_write_lines a " " b c d e f g h i j k >file &&
>> +	git add file &&
>> +	test_write_lines x " " b y d e f g h i j x >file &&
>> +	test_write_lines n K s n y q | git add -p file &&
>> +	git cat-file blob :file >actual &&
>> +	test_write_lines a " " b y d e f g h i j k >expect &&
>> +	test_cmp expect actual
>> +'
>> +
>>   test_done

Re: [PATCH v3 1/2] add -p: mark split hunks as undecided

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Junio
>
> On 25/09/2025 19:21, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> 
>>> ... In the case
>>> where the user only wants to change the selection of the first of
>>> the split hunks they will now have to do more work re-selecting the
>>> remaining split hunks.
>> In general, that is not just "first", as you can jump ahead, or just
>> keep typing "J" which is not all that hard, until you find the hunk
>> you are looking for, at which point you flip its status.
>
> Fair enough, I think that only applies to a single split hunk
> though. One you select or deselect it you can't walk though the
> remaining split hunks J anymore because you'll have been dumped at the
> next undecided hunk after entering 'y' or 'n'

If you have a single hunk for a file, you cannot split after you
selected that single hunk in the first place, and the change we are
discussing would not help the situation.

If you have two hunks, you select one (now the other one is shown),
you can "K" back to the selected one, "s"plit it, and then say 'y/n'
to deal with the first of the split hunks, which would take you to
that "originally second" hunk.  You can "K" back again to the last
of the minihunk.

I think the room for improvement of the UI we are seeing is mostly
outside the theme of this change under discussion, but enumerating
them may help somebody pick them up as #leftoverbits in the future.

 * There is no way to rework with a file once you decided sel/desel
   on all its hunks.  Even when you have multiple modified files,
   you cannot switch from the current file to another file whose
   hunks have already been decided.

   Perhaps introduce "Y" and "N", that allows a choice like "y" and
   "n", but does not auto-advance?  I think the current code takes
   "N" and performs "n", so this would be a backward incompatible
   change that needs to be designed better.  Perhaps when no hunks
   are in undecided state, show a prompt "What now?" that allows you
   to still navigate with "J" and "K", or go to next file?

 * There is no way to see what state a hunk that is on the screen is
   in.  If you remember that you came with "j" or "k" or deciding on
   all hunks in another file and you are automatically taken to this
   file, then the hunk is undecided, but if you came here with "J"
   or "K", you cannot tell if you decided to select it or deselect it.

   Perhaps tweak the "(3/5) Stage this hunk [choices]?" prompt and
   give this information?

The change under discussion somewhot helps the first one but only
when you have at least one undecided hunk in the file.

The latter problem is greatly alleviated with the change under
discussion on this thread.

>> So I like the updated behaviour very much, but I am reluctant to
>> pretend as if we are siding one camps of folks who think that
>> splitting a selected hunk is done with an intention to deselect most
>> of the minihunks most of the time, playing favors.  I think that
>> is a wrong way to frame the problem this patch solved.
>> In any case, I no longer have problems with the updated behaviour
>> with these two patches.  Thanks for working on them.
>> Will queue.
>
> Thanks, do you want a different commit message or are you happy to
> take them as-is?

Hmph, what would an improved commit log message would say?  Reword
the second UI problem above and explain how this change improves the
situation?

Thanks.

Re: [PATCH v3 1/2] add -p: mark split hunks as undecided

[Phillip Wood]

On 26/09/2025 18:37, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>> On 25/09/2025 19:21, Junio C Hamano wrote:
>>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>> So I like the updated behaviour very much, but I am reluctant to
>>> pretend as if we are siding one camps of folks who think that
>>> splitting a selected hunk is done with an intention to deselect most
>>> of the minihunks most of the time, playing favors.  I think that
>>> is a wrong way to frame the problem this patch solved.
>>> In any case, I no longer have problems with the updated behaviour
>>> with these two patches.  Thanks for working on them.
>>> Will queue.
>>
>> Thanks, do you want a different commit message or are you happy to
>> take them as-is?
> 
> Hmph, what would an improved commit log message would say?  Reword
> the second UI problem above and explain how this change improves the
> situation?

Yes. Having said that I'm about to go off the list until the middle of 
the week after next so maybe we should settle for what we've got.

Thanks

Phillip