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

[Junio C Hamano <gitster@pobox.com>]

"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