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

["Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>]

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