Re: [PATCH v3 1/2] add -p: mark split hunks as undecided
"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.
Show 46 quoted lines
> 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
