From: Phillip Wood <phillip.wood@talktalk.net>
To: Junio C Hamano <gitster@pobox.com>,
Jochen Sprickerhof <git@jochen.sprickerhof.de>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] add -p: coalesce hunks before testing applicability
Date: Thu, 30 Aug 2018 14:47:04 +0100 [thread overview]
Message-ID: <e5b2900a-0558-d3bf-8ea1-d526b078bbc2@talktalk.net> (raw)
In-Reply-To: <xmqq36uygyau.fsf@gitster-ct.c.googlers.com>
Dear Jochen/Junio
On 28/08/18 19:07, Junio C Hamano wrote:
> Jochen Sprickerhof <git@jochen.sprickerhof.de> writes:
>
>> When a hunk was split before being edited manually, it does not apply
>> anymore cleanly. Apply coalesce_overlapping_hunks() first to make it
>> work. Enable test for it as well.
>>
>> Signed-off-by: Jochen Sprickerhof <git@jochen.sprickerhof.de>
>> ---
>> git-add--interactive.perl | 8 ++++----
>> t/t3701-add-interactive.sh | 2 +-
>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
>> index 36f38ced9..c9f434e4a 100755
>> --- a/git-add--interactive.perl
>> +++ b/git-add--interactive.perl
>> @@ -1195,10 +1195,10 @@ sub edit_hunk_loop {
>> # delta from the original unedited hunk.
>> $hunk->{OFS_DELTA} and
>> $newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA};
>> - if (diff_applies($head,
>> - @{$hunks}[0..$ix-1],
>> - $newhunk,
>> - @{$hunks}[$ix+1..$#{$hunks}])) {
>> + my @hunk = @{$hunks};
>> + splice (@hunk, $ix, 1, $newhunk);
>> + @hunk = coalesce_overlapping_hunks(@hunk);
>> + if (diff_applies($head, @hunk)) {
>> $newhunk->{DISPLAY} = [color_diff(@{$newtext})];
>> return $newhunk;
>> }
>
> OK. I think I understand how this may be needed in certain forms of
> edit.
When $newhunk is created it is marked as dirty to prevent
coalesce_overlapping_hunks() from coalescing it. This patch does not
change that. What is happening is that by calling
coalesce_overlapping_hunks() the hunks that are not currently selected
are filtered out and any hunks that can be coalesced are (I think that
in the test that starts passing with this patch the only change is the
filtering as there's only a single hunk selected).
This is a subtle change to the test for the applicability of an edited
hunk. Previously when all the hunks were used to create the test patch
we could be certain that if the test patch applied then if the user
later selected any unselected hunk or deselected any selected hunk then
that operation would succeed. I'm not sure that is true now (but I
haven't thought about it for very long). We could restore the old test
condition and coalesce the hunks by copying all the hunks and setting
$hunk->{USE}=1 when creating the test patch if that turns out to be
useful (it would be interesting to see if the test still passes with
that change).
Best Wishes
Phillip
I do not think coalesce would reliably work against arbitrary
> kind of edit, but the function is called at the end of the loop of
> the caller of this function anyway, so it is not like this is making
> anything worse at all. Let's queue it and try it out.
>
> Thanks.
>
> All of the following is a tangent for future/further work, and
> should not be done as part of your patch. While this change may
> work around the immediate issue of diff_applies() returning "no", it
> makes it feel a bit iffy to keep the interface to return $newhunk.
>
> With the current interface, the function is saying the caller "here
> is a text that shows a new hunk, when spliced back into the @hunk
> array and coalesced together with others, would apply cleanly to the
> current index". But the corrected code is already doing a trial
> splice with trial coalescing anyway, so perhaps it may make more
> sense to update the interface into this function for the caller to
> say "the user asks to edit $ix'th hunk in @$hunks" and the function
> to answer "Here is the edited result in @$hunks; I've made sure it
> would cleanly apply".
>
> Incidentally, that would make it possible in the future to allow
> edit_hunk_loop to be told "the user wants to edit this $ix'th hunk",
> allow the editor to split that hunk into multiple hunks, and return
> the result by stuffing them (not just a single $newhunk) into the
> @hunk array. The caller's loop could then further select or join
> these hunks---such an interaction is impossible with the current
> interface that allows edit_hunk_loop to take a single hunk and
> return a single newhunk.
>
> But again, all of the above is a tangent for future/further work,
> and should not be done as part of your patch.
>
next prev parent reply other threads:[~2018-08-30 13:47 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-28 8:58 [PATCH] add -p: coalesce hunks before testing applicability Jochen Sprickerhof
2018-08-28 18:07 ` Junio C Hamano
2018-08-30 13:47 ` Phillip Wood [this message]
2018-08-30 14:51 ` Junio C Hamano
2018-09-03 19:01 ` Jochen Sprickerhof
2018-09-13 10:20 ` Phillip Wood
2018-09-23 17:16 ` Jochen Sprickerhof
2019-03-22 14:06 ` Johannes Schindelin
2019-06-02 14:17 ` Phillip Wood
2019-06-03 13:40 ` Johannes Schindelin
2019-06-03 14:59 ` Phillip Wood
2019-06-04 13:32 ` Johannes Schindelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e5b2900a-0558-d3bf-8ea1-d526b078bbc2@talktalk.net \
--to=phillip.wood@talktalk.net \
--cc=git@jochen.sprickerhof.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).