git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Phillip Wood <phillip.wood@talktalk.net>
To: Jochen Sprickerhof <jochen@sprickerhof.de>, phillip.wood@dunelm.org.uk
Cc: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Subject: Re: [PATCH] add -p: coalesce hunks before testing applicability
Date: Thu, 13 Sep 2018 11:20:28 +0100	[thread overview]
Message-ID: <d6a8f77b-0a83-90ae-a7fb-a3954ac3b346@talktalk.net> (raw)
In-Reply-To: <20180903190114.GC17416@vis>

Hi Jochen

On 03/09/2018 20:01, Jochen Sprickerhof wrote:
> Hi Phillip,
> 
> * Phillip Wood <phillip.wood@talktalk.net> [2018-08-30 14:47]:
>> 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).
> 
> Agreed here. It would be enough to include the first hunk in the test to
> make it fail again. Still I would see the patch as going in the right
> direction as we need something like coalesce_overlapping_hunks() to make
> the hunks applicable after the edit.

Yes in the long term we want to be able to coalesce edited hunks, but I
think it is confusing to call coalesce_overlapping_hunks() at the moment
as it will not coalesce the edited hunks.

>> 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).
> 
> I'm not sure here. If we use the same test from t3701, do s(plit),
> y(es), e(dit), it would fail later on. Can you come up with an example?

I think that if you split a hunk, edit the first subhunk, transforming a
trailing context line to a deletion then try if you try to stage the
second subhunk it will fail. With your patch the edit will succeed as
the second subhunk is skipped when testing the edited patch. Then when
you try to stage the second subhunk it will fail as it's leading context
will contradict the trailing lines of the edited subhunk. With the old
method the edit failed but didn't store up trouble for the future.

>> 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).
> 
> We set USE=1 for $newhunk already, or where would you set it?

To match the old test it needs to be set on the hunks we've skipped or
haven't got to yet so they're all in the patch that's tested after
editing a hunk.

Best Wishes

Phillip

> 
> Cheers Jochen


  reply	other threads:[~2018-09-13 10:20 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
2018-08-30 14:51     ` Junio C Hamano
2018-09-03 19:01     ` Jochen Sprickerhof
2018-09-13 10:20       ` Phillip Wood [this message]
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=d6a8f77b-0a83-90ae-a7fb-a3954ac3b346@talktalk.net \
    --to=phillip.wood@talktalk.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jochen@sprickerhof.de \
    --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).