Felipe Contreras [off-list ref] writes:
I found the problem, m->chg0 was not initialized in xdl_refine_conflicts.
I'm not familiar with the area so I don't know if the following makes
sense, but it fixes the crash:
Unlike the remainder of the xdiff/ directory, xdiff/xmerge.c was
Dscho's brainchild if I am not mistaken, so I'm CCing him for
input.
quoted hunk
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -333,7 +333,7 @@ static int xdl_refine_conflicts(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m,
mmfile_t t1, t2;
xdfenv_t xe;
xdchange_t *xscr, *x;
- int i1 = m->i1, i2 = m->i2;
+ int i0 = m->i0, i1 = m->i1, i2 = m->i2;
/* let's handle just the conflicts */
if (m->mode)@@ -384,6 +384,8 @@ static int xdl_refine_conflicts(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m,
m->next = m2;
m = m2;
m->mode = 0;
+ m->i0 = i0;
+ m->chg0 = 0;
m->i1 = xscr->i1 + i1;
m->chg1 = xscr->chg1;
m->i2 = xscr->i2 + i2;
On Mon, Jun 14, 2021 at 7:07 PM Junio C Hamano [off-list ref] wrote:
Felipe Contreras [off-list ref] writes:
quoted
I found the problem, m->chg0 was not initialized in xdl_refine_conflicts.
I'm not familiar with the area so I don't know if the following makes
sense, but it fixes the crash:
Unlike the remainder of the xdiff/ directory, xdiff/xmerge.c was
Dscho's brainchild if I am not mistaken, so I'm CCing him for
input.
This is going to sound harsh, but people shouldn't waste (any more)
time reviewing the patches in this thread or the "merge: cleanups and
fix" series submitted elsewhere. They should all just be rejected.
I do not think it is reasonable to expect reviewers to spend time
responding to re-posted patches when:
* no attempt was made to make sure they were up-to-date with current
code beyond compiling (see below)
* no attempt was made to address missing items pointed out in
response to the original submission[1]
* no attempt was made to handle or even test particular cases
pointed out in response to the original submission (see [1] and below)
* the patches were posted despite knowing they caused segfaults, and
without even stating as much![2]
* the segfault "fixes" are submitted as a separate series from the
patch introducing the segfault[3], raising the risk that one gets
picked up without the other.
In my opinion, these submissions were egregiously cavalier. I'll
submit a patch (or perhaps a few) soon that has a functioning zdiff3.
However, since I've already put in the time to understand it, let me
explain what is wrong with this patch. This particular change is in
the area of the code that splits conflict regions when there are
portions of the sides (not the base) that match. Doing such splitting
makes sense with "merge" conflictStyle since the base is never shown;
this splitting can allow pulling the common lines out of the conflict
region. However, with diff3 or zdiff3, the original text does not
match the sides and by splitting the conflict region, we are forced to
decide how or where to split the original text among the various
conflict (and non-conflict?) regions. This is pretty haphazard, and
the effect of this patch is to assign all of the original text to the
first conflict region in the split, and make all other regions have
empty base text.
This exact scenario was discussed by you and Peff back when zdiff3 was
originally introduced in the thread where Felipe got the patch that he
started this thread with. In that thread, Peff explained how zdiff3
should only try to move common lines at the beginning or end of the
conflict hunk outside the conflict region, without doing any splitting
of the conflict region (this particular issue took about 1/3 to 1/2 of
the original thread, but I think [4] has a good hilight).
Additionally, a quick grep through the code showed that there are
additional places in bash/zsh completion that need to be fixed to use
the new option besides the locations modified in the original zdiff3
patch. See [1] and [2] for various other things overlooked.
[1] https://lore.kernel.org/git/CABPp-BGZ2H1MVgw9RvSdogLMdqsX3n89NkkDYDa2VM3TRHn7tg@mail.gmail.com/
[2] https://lore.kernel.org/git/YMbexfeUG78yBix4@coredump.intra.peff.net/
[3] https://lore.kernel.org/git/20210613225836.1009569-5-felipe.contreras@gmail.com/
[4] https://lore.kernel.org/git/20130307180157.GA6604@sigill.intra.peff.net/
Elijah Newren wrote:
On Mon, Jun 14, 2021 at 7:07 PM Junio C Hamano [off-list ref] wrote:
quoted
Felipe Contreras [off-list ref] writes:
quoted
I found the problem, m->chg0 was not initialized in xdl_refine_conflicts.
I'm not familiar with the area so I don't know if the following makes
sense, but it fixes the crash:
Unlike the remainder of the xdiff/ directory, xdiff/xmerge.c was
Dscho's brainchild if I am not mistaken, so I'm CCing him for
input.
This is going to sound harsh, but people shouldn't waste (any more)
time reviewing the patches in this thread or the "merge: cleanups and
fix" series submitted elsewhere. They should all just be rejected.
I do not think it is reasonable to expect reviewers to spend time
responding to re-posted patches when:
* no attempt was made to make sure they were up-to-date with current
code beyond compiling (see below)
What makes you think so?
* no attempt was made to address missing items pointed out in
response to the original submission[1]
The original submission caused a discussion with no resolution, and
edned with Jeff saying he wanted to try real use-cases and that that he
wanted to use it in practice for a while.
The purpose v1 of this series was to respark the discussion and see if
any of the original parties had changed their minds.
People do change their minds after 8 years.
* no attempt was made to handle or even test particular cases
pointed out in response to the original submission (see [1] and below)
Those were sent *after* the series, except [4], which clearly states the
*opposite* of there being a deal-breaker:
But again, we don't do this splitting now. So I don't think it's
something that should make or break a decision to have zdiff3. Without
the splitting, I can see it being quite useful.
* the patches were posted despite knowing they caused segfaults, and
without even stating as much![2]
Whomever *knew* that, it wasn't me.
* the segfault "fixes" are submitted as a separate series from the
patch introducing the segfault[3], raising the risk that one gets
picked up without the other.
My v2 includes the patch.
Just because a patch is in one series that doesn't preclude it from
being in another series. `git merge` and `git rebase` are smart enough
to handle such cases.
There is no risk of that happening (unless there's plans of merging v1
as-is).
In my opinion, these submissions were egregiously cavalier.
If you make unwarranted assumptions everything is possible.
I'll submit a patch (or perhaps a few) soon that has a functioning
zdiff3.
I already have a functioning zdiff3.
However, since I've already put in the time to understand it, let me
explain what is wrong with this patch. This particular change is in
the area of the code that splits conflict regions when there are
portions of the sides (not the base) that match. Doing such splitting
makes sense with "merge" conflictStyle since the base is never shown;
this splitting can allow pulling the common lines out of the conflict
region. However, with diff3 or zdiff3, the original text does not
match the sides and by splitting the conflict region, we are forced to
decide how or where to split the original text among the various
conflict (and non-conflict?) regions. This is pretty haphazard, and
the effect of this patch is to assign all of the original text to the
first conflict region in the split, and make all other regions have
empty base text.
Yes, that is *one* opinion. The jury is still out on what is the best
approach. Junio and Jeff did not agree on that.
The whole point of "zdiff3" was to have something closer to "merge",
even if it wasn't 100% correct. Your approach maybe more correct, but
correctness was never the point.
Either way, I have more rewarding things to focus on, so good luck with
that.
[1] https://lore.kernel.org/git/CABPp-BGZ2H1MVgw9RvSdogLMdqsX3n89NkkDYDa2VM3TRHn7tg@mail.gmail.com/
[2] https://lore.kernel.org/git/YMbexfeUG78yBix4@coredump.intra.peff.net/
[3] https://lore.kernel.org/git/20210613225836.1009569-5-felipe.contreras@gmail.com/
[4] https://lore.kernel.org/git/20130307180157.GA6604@sigill.intra.peff.net/
--
Felipe Contreras
On Tue, Jun 15, 2021 at 2:16 AM Felipe Contreras
[off-list ref] wrote:
Elijah Newren wrote:
quoted
On Mon, Jun 14, 2021 at 7:07 PM Junio C Hamano [off-list ref] wrote:
quoted
Felipe Contreras [off-list ref] writes:
quoted
I found the problem, m->chg0 was not initialized in xdl_refine_conflicts.
I'm not familiar with the area so I don't know if the following makes
sense, but it fixes the crash:
Unlike the remainder of the xdiff/ directory, xdiff/xmerge.c was
Dscho's brainchild if I am not mistaken, so I'm CCing him for
input.
This is going to sound harsh, but people shouldn't waste (any more)
time reviewing the patches in this thread or the "merge: cleanups and
fix" series submitted elsewhere. They should all just be rejected.
I do not think it is reasonable to expect reviewers to spend time
responding to re-posted patches when:
* no attempt was made to make sure they were up-to-date with current
code beyond compiling (see below)
What makes you think so?
I did a simple grep to see where "diff3" was mentioned in the codebase
to see if any of those needed a "zdiff3". Among the things I found
was that although the original patch updated git-completion.bash,
there were additional locations within a current git-completion.bash
that referred to "diff3" that should also have a "zdiff3". I know you
understand that part of the code.
quoted
* no attempt was made to address missing items pointed out in
response to the original submission[1]
The original submission caused a discussion with no resolution
The discussion ended with no resolution in part because there were
multiple items discussed that would need to be addressed. Including
the one reiterated at the end of the discussion.
, and
edned with Jeff saying he wanted to try real use-cases and that that he
wanted to use it in practice for a while.
That wasn't the end of the discussion. The email you are referencing
occurred here: https://lore.kernel.org/git/20130307185046.GA11622@sigill.intra.peff.net/.
The end of the discussion was Junio quoting himself in order to
reiterate that "As long as we clearly present the users what the
option does and what its implications are, it is not bad to have such
an option, I think." See
https://lore.kernel.org/git/7vip42gfjc.fsf@alter.siamese.dyndns.org/
and check the timestamps in the threadlist.
quoted
* no attempt was made to handle or even test particular cases
pointed out in response to the original submission (see [1] and below)
Those were sent *after* the series, except [4], which clearly states the
*opposite* of there being a deal-breaker:
But again, we don't do this splitting now. So I don't think it's
something that should make or break a decision to have zdiff3. Without
the splitting, I can see it being quite useful.
This statement from Peff was incorrect; the zdiff3 patches made the
code do splitting of conflict hunks. I would normally understand if
perhaps you didn't know his statement was incorrect and wouldn't have
had a way to know, *except* for the fact that this exact patch we are
commenting on that you posted is modifying the code that does conflict
hunk splitting.
Further, you stated at
https://lore.kernel.org/git/60c8758c80e13_e633208f7@natae.notmuch/
that you wanted to see conflict hunk splitting in a zdiff3 mode and
expected it. So clearly conflict hunk splitting is relevant to you
even if it wasn't to Peff.
Peff and Junio spent several emails discussing conflict hunk splitting
in the original thread (with Junio raising the question multiple times
showing it was a concern of his), and Peff spent several emails
discussing that topic even assuming that code was never triggered. In
contrast to Peff, you know that conflict hunk splitting is relevant
since you wanted it to occur, you saw the old thread where they
discussed that topic and length, and yet you made no attempt to
include a testcase (perhaps even using the one they discussed) to show
how the splitting works? I find that negligent.
quoted
* the patches were posted despite knowing they caused segfaults, and
without even stating as much![2]
Whomever *knew* that, it wasn't me.
You knew that Peff had reported they caused segfaults. He pointed it
out after making you aware of the zdiff3 patch; see
https://lore.kernel.org/git/YMI+R5LFTj7ezlZE@coredump.intra.peff.net/.
You also acknowledged having known of Peff's reports before reposting
the patches at https://lore.kernel.org/git/60c82a622ae66_e5292087f@natae.notmuch/
You may be correct to point out that you only knew Peff had reported
segfaults, rather than having verified for yourself that there were
segfaults. But the fact that you took no action on the knowledge you
did have, neither trying to verify, nor asking if the segfaults still
occurred, nor even relaying those reports when reposting the patch, is
exactly the problem at stake here. I find the lack of action with
respect to the segfault report to be reckless.
quoted
* the segfault "fixes" are submitted as a separate series from the
patch introducing the segfault[3], raising the risk that one gets
picked up without the other.
My v2 includes the patch.
Ah, so your plan was to post a v2 with the fix as well as *also* post
that fix elsewhere? Okay, that makes me feel better about this item,
so I retract it.
quoted
In my opinion, these submissions were egregiously cavalier.
If you make unwarranted assumptions everything is possible.
Which assumptions? That you were splitting the segfault fixes into a
separate series and not also including them with the patch that
introduces the segfault? That does seem unusual and would have been
nice if you had communicated your plans somewhere so others wouldn't
have to worry about that particular issue, but I agree that your
explanation does invalidate the fifth item from my list as a concern.
Unfortunately, that still leaves the other four. It's unfair to
reviewers to post patches if you have not done due diligence. I've
read other patches of yours and commented that I thought they looked
good, so I'm not just trying to pick on you. You clearly have talent.
With regards to the zdiff3 patches, I've stated above why I think you
haven't done your due diligence. Sometimes people make mistakes;
that's something that can be corrected. What I find egregious here is
that even when Peff and I have pointed out how more due diligence is
expected and needed, you've dug in to explain why you think your
course of action was reasonable (both here and in
https://lore.kernel.org/git/60c82a622ae66_e5292087f@natae.notmuch/).
That in my mind raises your submissions from careless to glaringly
cavalier. Further, it makes me suspect we may continue to see you
repeat such behavior. That worries me.