Hi Dscho
Sorry it's taken me so long to get round to replying to this
On 22/03/2019 14:06, Johannes Schindelin wrote:
Hi,
On Thu, 13 Sep 2018, Phillip Wood wrote:
On 03/09/2018 20:01, Jochen Sprickerhof wrote:
* Phillip Wood <phillip.wood@xxxxxxxxxxxx> [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.
Agreed. I actually have code to coalesce even edited hunks, but it is all
in C.
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.
Indeed, this is a problem I also stumbled over.
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.
The way I fixed this in the C code is by teaching the equivalent of the
`coalesce_overlapping_hunks()` function to simply ignore the equivalent of
`$hunk->{USE}`: the function signature takes an additional `use_all`
parameter, which will override the `use` field.
That sounds like a good solution. Thanks for working on the conversion
to C, I'll try and find time look at the code on github.
Best Wishes
Phillip
Furthermore, my C code actually does the coalescing as part of the
`reassemble_patch()` function, feeding the output directly into the
`stdin` of the `git apply` process (with, or without `--check`).
And crucially, my C code does not try to assemble a new `hunks` array, but
simply works in-place, reverting the changes if the hunk edits result in a
patch that does not apply. The Perl version probably does not need that
part, as it is pretty careless with memory (as Perl encourages to do).
See for yourself:
https://github.com/dscho/git/commit/6f8ac4809280f2cd018683ab5199b004ada2350e#diff-f58d2179be56b196b9f35c6d24799a8eR337
Ciao,
Dscho
P.S.: Yes, this is part of my work to complete Slavica's "`git add -i`
in C" project. There are quite a few loose ends to tie, but I can already
use it myself to call `git add -p`, which is what I care most about, as it
regularly takes more than one second to spin up Perl on Windows, which is
friggin' annoying, I tell ya.
