add -p: a couple of hunk splitting fixes #1863
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 <[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. Signed-off-by: Phillip Wood <[email protected]>
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -953,6 +953,7 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff, | |||
| * sizeof(*hunk)); | |||
There was a problem hiding this comment.
On the Git mailing list, Justin Tobler wrote (reply to this):
On 25/02/21 02:57PM, Phillip Wood via GitGitGadget wrote:
> 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.
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 <[email protected]>
> ---
> 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
>
>
|
User |
| @@ -1181,19 +1182,29 @@ static ssize_t recount_edited_hunk(struct add_p_state *s, struct hunk *hunk, | |||
| { | |||
There was a problem hiding this comment.
On the Git mailing list, Justin Tobler wrote (reply to this):
On 25/02/21 02:57PM, Phillip Wood via GitGitGadget wrote:
> 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.
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 <[email protected]>
> ---
> 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
| @@ -953,6 +953,7 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff, | |||
| * sizeof(*hunk)); | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Phillip Wood via GitGitGadget" <[email protected]> writes:
> 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.
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.
| @@ -1181,19 +1182,29 @@ static ssize_t recount_edited_hunk(struct add_p_state *s, struct hunk *hunk, | |||
| { | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Phillip Wood via GitGitGadget" <[email protected]> writes:
> 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.
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 <[email protected]>
> ---
> 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'.
cc: Justin Tobler [email protected]