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
|
/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}
add-patch.c
Outdated
| @@ -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
add-patch.c
Outdated
| @@ -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.There was a problem hiding this comment.
On the Git mailing list, [email protected] wrote (reply to this):
Hi Junio and Justin
[I'm replying to both of you via Junio's email as you both raised the same question]
On 21/02/2025 21:31, Junio C Hamano wrote:
> "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.
If the user wants to deselect the 10th mini-hunk then they have to wade through them all with or without this patch. If they want to deselect an earlier one then they will now have to do more work.
Currently after a selected hunk is split we always prompt the user to make a decision on the first mini-hunk even though it is marked as selected when it is split. This seems inconsistent and confused me when I first tried splitting a selected hunk which is why I wrote this patch. I can see that in some circumstances this patch does make more work for the user, but I do think it makes it easier to understand what happens when hunk is split.
Best Wishes
Phillip
> So, I dunno.
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
[email protected] writes:
>> "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.
>
> If the user wants to deselect the 10th mini-hunk then they have to
> wade through them all with or without this patch. If they want to
> deselect an earlier one then they will now have to do more work.
Or directly jump to it with "/go-to-the-one-with-this-string"?
> Currently after a selected hunk is split we always prompt the user to
> make a decision on the first mini-hunk even though it is marked as
> selected when it is split. This seems inconsistent and confused me
> when I first tried splitting a selected hunk which is why I wrote this
> patch.
Hmph, so there is an obvious alternative "fix" to the inconsistency,
i.e., after splitting, move to the first unselected hunk?
> I can see that in some circumstances this patch does make more
> work for the user, but I do think it makes it easier to understand
> what happens when hunk is split.
And the alternative may resolve the inconsistency and make it less
work for the users? I dunno.
This is totally orthogonal to this "split" issue and outside the
scope of this topic, but one thing that I do not like the design of
"add -p", which most likely was inherited from the very initial
iteration before it was rewritten in C, is that we never ask
reconfirmation once all the hunks got decided. With 3 hunks, after
choosing hunk #1 by mistake, I can still go back and correct the
mistake even if I noticed the mistake after making decision on the
hunk #2 (thanks to the fact that hunk #3 hasn't been decided), but
if the hunk #3 is missing, going back and correcting #1 becomes
impossible as the program exits immediatly after deciding on #2.
But I guess this depends not just on the user but on occasion.
After all, re-running "reset/add -p" after such a mistake is not so
huge a deal anyway.
Thanks.There was a problem hiding this comment.
On the Git mailing list, [email protected] wrote (reply to this):
On 26/02/2025 16:49, Junio C Hamano wrote:
> [email protected] writes:
> >>> "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.
>>
>> If the user wants to deselect the 10th mini-hunk then they have to
>> wade through them all with or without this patch. If they want to
>> deselect an earlier one then they will now have to do more work.
> > Or directly jump to it with "/go-to-the-one-with-this-string"?
Oh, I'd forgotten '/' searches all the hunks rather than just the undecided ones.
>> Currently after a selected hunk is split we always prompt the user to
>> make a decision on the first mini-hunk even though it is marked as
>> selected when it is split. This seems inconsistent and confused me
>> when I first tried splitting a selected hunk which is why I wrote this
>> patch.
> > Hmph, so there is an obvious alternative "fix" to the inconsistency,
> i.e., after splitting, move to the first unselected hunk?
We could do that but I think it would be even more confusing than the current behavior as it would make it harder to change the state of the mini-hunks. At least with the current behavior one can use 'J' to move through them immediately after splitting the original hunk. If we move to the next undecided hunk one has to know where the newly-created mini-hunks are relative to that.
>> I can see that in some circumstances this patch does make more
>> work for the user, but I do think it makes it easier to understand
>> what happens when hunk is split.
> > And the alternative may resolve the inconsistency and make it less
> work for the users? I dunno.
I'm not sure either. I dislike the way it works at the moment and find it confusing but if there are a lot of people relying on it then I'd be reluctant to change it. Unfortunately we don't have any way to know if anyone is relying on the current behavior without changing it and seeing if anyone complains. Given it is a bit of a corner case I'm not sure whether it is worth spending much more time on it.
> This is totally orthogonal to this "split" issue and outside the
> scope of this topic, but one thing that I do not like the design of
> "add -p", which most likely was inherited from the very initial
> iteration before it was rewritten in C, is that we never ask
> reconfirmation once all the hunks got decided. With 3 hunks, after
> choosing hunk #1 by mistake, I can still go back and correct the
> mistake even if I noticed the mistake after making decision on the
> hunk #2 (thanks to the fact that hunk #3 hasn't been decided), but
> if the hunk #3 is missing, going back and correcting #1 becomes
> impossible as the program exits immediatly after deciding on #2.
> But I guess this depends not just on the user but on occasion.
> After all, re-running "reset/add -p" after such a mistake is not so
> huge a deal anyway.
I can see the problem and asking for conformation before quitting would have been nice if we'd done it from the start. I'm not sure it is worth the disruption of changing it when one can re-run "reset/add -p" quite easily though. I guess we could add an opt-in cofing that eventually becomes the default.
While we're talking about tangential issues it would be nice if when a user revisited a hunk we told them its current state. At the moment there is no way to tell if a hunk has been selected or not. Related to that the help for 'J' and 'K' talk about leaving the current hunk undecided when what they actually do is leave the current state unchanged.
Best Wishes
PhillipThere was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
[email protected] writes:
>>> Currently after a selected hunk is split we always prompt the user to
>>> make a decision on the first mini-hunk even though it is marked as
>>> selected when it is split. This seems inconsistent and confused me
>>> when I first tried splitting a selected hunk which is why I wrote this
>>> patch.
>> Hmph, so there is an obvious alternative "fix" to the inconsistency,
>> i.e., after splitting, move to the first unselected hunk?
>
> We could do that but I think it would be even more confusing than the
> current behavior as it would make it harder to change the state of the
> mini-hunks. At least with the current behavior one can use 'J' to move
> through them immediately after splitting the original hunk. If we move
> to the next undecided hunk one has to know where the newly-created
> mini-hunks are relative to that.
True. After all, going back to an already selected hunk and then
splitting the hunk is a clear indication that the user wants to
visit some of them to change their state. Moving them back to
"undecided" (not "deselected") instead of leaving them marked as
"selected" (which is the current behaviour) looks like a better
behaviour and I wish I knew about the possibility in late 2006 when
I added the hunk splitting.
> I'm not sure either. I dislike the way it works at the moment and find
> it confusing but if there are a lot of people relying on it then I'd
> be reluctant to change it.
I share the sentiment, especially the latter.
> Unfortunately we don't have any way to know
> if anyone is relying on the current behavior without changing it and
> seeing if anyone complains. Given it is a bit of a corner case I'm not
> sure whether it is worth spending much more time on it.
Given our user base has grown quite a bit over the years, it almost
is a given that any change to existing behaviour is a regression to
somebody. Certainly a safe material for Git 3.0 but I do not know
if it is safe enough for 2.50 for example. The strategy to leave it
longer in 'next' did not work well to catch potential issues for
another topic during this cycle, but we could try it out again.
> I can see the problem and asking for conformation before quitting
> would have been nice if we'd done it from the start. I'm not sure it
> is worth the disruption of changing it when one can re-run "reset/add
> -p" quite easily though.
Yup. That matches my assessment of it. I brought it up because I
see this "selection should not stick across splitting" falls into
the same "it would have been nice if it were that way from the
beginning" bucket.
> I guess we could add an opt-in cofing that
> eventually becomes the default.
I'd prefer not to add configuration for tweaking such a small thing
(this applies to "should selection stick across splitting?", too).
> While we're talking about tangential issues it would be nice if when a
> user revisited a hunk we told them its current state. At the moment
> there is no way to tell if a hunk has been selected or not.
The user came back with 'J' or 'K' probably because the hunk was
skipped in their earlier navigation with 'k' or 'j', so users may be
using it as a workaround, but I agree there should be an indicator
for the (unselected, selected, undecided).
> Related to that the help for 'J' and 'K' talk about leaving the
> current hunk undecided when what they actually do is leave the
> current state unchanged.
Nice catch.
Thanks.There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
On 27/02/2025 18:36, Junio C Hamano wrote:
> [email protected] writes:
> >>>> Currently after a selected hunk is split we always prompt the user to
>>>> make a decision on the first mini-hunk even though it is marked as
>>>> selected when it is split. This seems inconsistent and confused me
>>>> when I first tried splitting a selected hunk which is why I wrote this
>>>> patch.
>>> Hmph, so there is an obvious alternative "fix" to the inconsistency,
>>> i.e., after splitting, move to the first unselected hunk?
>>
>> We could do that but I think it would be even more confusing than the
>> current behavior as it would make it harder to change the state of the
>> mini-hunks. At least with the current behavior one can use 'J' to move
>> through them immediately after splitting the original hunk. If we move
>> to the next undecided hunk one has to know where the newly-created
>> mini-hunks are relative to that.
> > True. After all, going back to an already selected hunk and then
> splitting the hunk is a clear indication that the user wants to
> visit some of them to change their state. Moving them back to
> "undecided" (not "deselected") instead of leaving them marked as
> "selected" (which is the current behaviour) looks like a better
> behaviour and I wish I knew about the possibility in late 2006 when
> I added the hunk splitting.
> >> I'm not sure either. I dislike the way it works at the moment and find
>> it confusing but if there are a lot of people relying on it then I'd
>> be reluctant to change it.
> > I share the sentiment, especially the latter.
> >> Unfortunately we don't have any way to know
>> if anyone is relying on the current behavior without changing it and
>> seeing if anyone complains. Given it is a bit of a corner case I'm not
>> sure whether it is worth spending much more time on it.
> > Given our user base has grown quite a bit over the years, it almost
> is a given that any change to existing behaviour is a regression to
> somebody. Certainly a safe material for Git 3.0 but I do not know
> if it is safe enough for 2.50 for example. The strategy to leave it
> longer in 'next' did not work well to catch potential issues for
> another topic during this cycle, but we could try it out again.
I'll drop this patch for now. There was some talk a while ago about adding a mechanism to select "git 3.0" features at build or run time. If we add something like that I'll resubmit with this change guarded by that feature.
>> I can see the problem and asking for conformation before quitting
>> would have been nice if we'd done it from the start. I'm not sure it
>> is worth the disruption of changing it when one can re-run "reset/add
>> -p" quite easily though.
> > Yup. That matches my assessment of it. I brought it up because I
> see this "selection should not stick across splitting" falls into
> the same "it would have been nice if it were that way from the
> beginning" bucket.
> >> I guess we could add an opt-in cofing that
>> eventually becomes the default.
> > I'd prefer not to add configuration for tweaking such a small thing
> (this applies to "should selection stick across splitting?", too).
Perhaps we should make the confirm-before-quitting thing a "git 3.0" feature as well?
Best Wishes
Phillip
>> While we're talking about tangential issues it would be nice if when a
>> user revisited a hunk we told them its current state. At the moment
>> there is no way to tell if a hunk has been selected or not.
> > The user came back with 'J' or 'K' probably because the hunk was
> skipped in their earlier navigation with 'k' or 'j', so users may be
> using it as a workaround, but I agree there should be an indicator
> for the (unselected, selected, undecided).
> >> Related to that the help for 'J' and 'K' talk about leaving the
>> current hunk undecided when what they actually do is leave the
>> current state unchanged.
> > Nice catch.
> > Thanks.
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Phillip Wood <[email protected]> writes:
> ... There was some talk a while ago about
> adding a mechanism to select "git 3.0" features at build or run
> time. If we add something like that I'll resubmit with this change
> guarded by that feature.
Documentation/BreakingChanges says that we can hide it behind
WITH_BREAKING_CHANGES compile-time switch, and that is part of
2.49-rc0 already. The linux-breaking-changes GitHub Actions CI job
runs with it defined.
> Perhaps we should make the confirm-before-quitting thing a "git 3.0"
> feature as well?
I do not feel too strongly either way. Sometimes I wish it asked
for the final confirmation after all hunks are decided. Most of the
time I do not feel that way, which almost always is after saying 'q'
to finish the selection. So I dunno, but my thinking right now is
that I lean a bit toward negative than positive.
In any case, I think we should indicate the (selected, deselected,
undecided) for the current hunk the user is being asked about, which
we talked about. As a workaround, we can do 'g' command to see the
list of hunks and check the indicator (+/ /-) for each hunk.There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi Junio
On 28/02/2025 17:06, Junio C Hamano wrote:
> Phillip Wood <[email protected]> writes:
> >> ... There was some talk a while ago about
>> adding a mechanism to select "git 3.0" features at build or run
>> time. If we add something like that I'll resubmit with this change
>> guarded by that feature.
> > Documentation/BreakingChanges says that we can hide it behind
> WITH_BREAKING_CHANGES compile-time switch, and that is part of
> 2.49-rc0 already. The linux-breaking-changes GitHub Actions CI job
> runs with it defined.
Thanks I'd missed that being merged. I'll re-roll with the changes in this patch guarded by WITH_BREAKING_CHANGES.
>> Perhaps we should make the confirm-before-quitting thing a "git 3.0"
>> feature as well?
> > I do not feel too strongly either way. Sometimes I wish it asked
> for the final confirmation after all hunks are decided. Most of the
> time I do not feel that way, which almost always is after saying 'q'
> to finish the selection. So I dunno, but my thinking right now is
> that I lean a bit toward negative than positive.
> > In any case, I think we should indicate the (selected, deselected,
> undecided) for the current hunk the user is being asked about, which
> we talked about. As a workaround, we can do 'g' command to see the
> list of hunks and check the indicator (+/ /-) for each hunk.
I'll try and take a look at that in the next release cycle
Best Wishes
Phillip
| @@ -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
phillipwood
force-pushed
the
add-p-split-hunks-are-undecided
branch
from
35ef0ee to
3b23ea8
Compare
|
User |
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]>
phillipwood
force-pushed
the
add-p-split-hunks-are-undecided
branch
from
3b23ea8 to
1878421
Compare
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.
Thanks to Justin and Junio for their comments in V1. I've dropped the first patch due to concerns that it may break existing workflows. The second patch is unchanged apart from a style fix suggested by Junio.
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'.
cc: Justin Tobler [email protected]
cc: Junio C Hamano [email protected]
cc: Phillip Wood [email protected]