add -p: a couple of hunk splitting fixes

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

From: Phillip Wood <[email protected]>

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.

---
 add-patch.c                |  3 ++-
 t/t3701-add-interactive.sh | 10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

index 95c67d8c80c..f44f98275cc 100644
@@ -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;

index b8a05d95f3f..760f3d0d30f 100755
@@ -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

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.

---
 add-patch.c                | 12 +++++++++++-
 t/t3701-add-interactive.sh | 21 +++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

index f44f98275cc..982745373df 100644
@@ -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;
 		}
 

index 760f3d0d30f..cb81bfe64c8 100755
@@ -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

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.

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?

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.

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" ;-)

Hi Junio and Justin

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

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

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

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

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.

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

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'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.

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

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 share the sentiment, especially the latter.

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.

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'd prefer not to add configuration for tweaking such a small thing (this applies to "should selection stick across splitting?", too).

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).

Nice catch.

Thanks.

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.

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

Best Wishes

Phillip

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.

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.

Hi Junio

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

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

Best Wishes

Phillip

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/[email protected]

+ ## 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.

@@ 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

From: Phillip Wood <[email protected]>

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/[email protected]

---
 Documentation/BreakingChanges.adoc |  5 +++++
 add-patch.c                        |  7 +++++++
 t/t3701-add-interactive.sh         | 10 ++++++++++
 3 files changed, 22 insertions(+)

index f8d2eba061..f62f75898f 100644
@@ -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).

index 302e6ba7d9..32157e31ed 100644
@@ -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;

index 04d2a19835..43a856e0c0 100755
@@ -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

From: Phillip Wood <[email protected]>

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.

---
 add-patch.c                | 12 +++++++++++-
 t/t3701-add-interactive.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

index 32157e31ed..0754f54a38 100644
@@ -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;
 		}
 

index 43a856e0c0..77f99e9ecb 100755
@@ -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

That is great, but ...

... 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?

Hi Junio

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'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/[email protected]/

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.

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.

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/[email protected]
     +    to navigate back to them in order to change their selected state.
      
          Signed-off-by: Phillip Wood <[email protected]>
      
     - ## 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

From: Phillip Wood <[email protected]>

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.

---
 add-patch.c                |  3 ++-
 t/t3701-add-interactive.sh | 10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

index 302e6ba7d9..61f42de9ea 100644
@@ -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;

index 04d2a19835..a6829fd085 100755
@@ -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

From: Phillip Wood <[email protected]>

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.

---
 add-patch.c                | 12 +++++++++++-
 t/t3701-add-interactive.sh | 21 +++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

index 61f42de9ea..bcc2d7666f 100644
@@ -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;
 		}
 

index a6829fd085..13739a4582 100755
@@ -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

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.

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.

Hi Junio

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'

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

Phillip

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.

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.

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