| Message ID | 43a0592a462cf68bcfdc54373da2319431c3c1ca.1740149837.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | add -p: a couple of hunk splitting fixes | expand |
On 25/02/21 02:57PM, Phillip Wood via GitGitGadget wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > 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 <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 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 > >
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > 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.
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; 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; 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