[PATCH 0/4] merge: cleanups and fix

[PATCH 0/4] merge: cleanups and fix

[Felipe Contreras]

Investigating a crash with the proposed zdiff3 [1] I found some easy
improvements in the merge/xmerge code and one crash fix.

These should not cause any functional changes, even the fix doesn't
affect any current code-paths.

[1] https://lore.kernel.org/git/YMYnVWSEgxvKRU9j@coredump.intra.peff.net/

Felipe Contreras (4):
  merge: simplify initialization
  merge: fix Yoda conditions
  xdiff/xmerge: simplify xdl_recs_copy_0
  xdiff/xmerge: fix chg0 initialization

 builtin/merge-file.c |  8 ++------
 xdiff/xmerge.c       | 30 ++++++++++++++++--------------
 2 files changed, 18 insertions(+), 20 deletions(-)

-- 
2.32.0

[PATCH 1/4] merge: simplify initialization

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/merge-file.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index 06a2f90c48..0186f4156a 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -28,7 +28,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	const char *names[3] = { NULL, NULL, NULL };
 	mmfile_t mmfs[3];
 	mmbuffer_t result = {NULL, 0};
-	xmparam_t xmp = {{0}};
+	xmparam_t xmp = { .level = XDL_MERGE_ZEALOUS_ALNUM };
 	int ret = 0, i = 0, to_stdout = 0;
 	int quiet = 0;
 	struct option options[] = {
@@ -48,10 +48,6 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
-	xmp.level = XDL_MERGE_ZEALOUS_ALNUM;
-	xmp.style = 0;
-	xmp.favor = 0;
-
 	if (startup_info->have_repository) {
 		/* Read the configuration file */
 		git_config(git_xmerge_config, NULL);
-- 
2.32.0

Re: [PATCH 1/4] merge: simplify initialization

[Ævar Arnfjörð Bjarmason]

On Sun, Jun 13 2021, Felipe Contreras wrote:

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/merge-file.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/builtin/merge-file.c b/builtin/merge-file.c
> index 06a2f90c48..0186f4156a 100644
> --- a/builtin/merge-file.c
> +++ b/builtin/merge-file.c
> @@ -28,7 +28,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
>  	const char *names[3] = { NULL, NULL, NULL };
>  	mmfile_t mmfs[3];
>  	mmbuffer_t result = {NULL, 0};
> -	xmparam_t xmp = {{0}};
> +	xmparam_t xmp = { .level = XDL_MERGE_ZEALOUS_ALNUM };
>  	int ret = 0, i = 0, to_stdout = 0;
>  	int quiet = 0;
>  	struct option options[] = {
> @@ -48,10 +48,6 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
>  		OPT_END(),
>  	};
>  
> -	xmp.level = XDL_MERGE_ZEALOUS_ALNUM;
> -	xmp.style = 0;
> -	xmp.favor = 0;
> -
>  	if (startup_info->have_repository) {
>  		/* Read the configuration file */
>  		git_config(git_xmerge_config, NULL);

Looks good, maybe we can fix the similar code in ll_xdl_merge() while
we're at it?

Also, not a problem in your commit, but we check for that constant in
only one place, as:

    XDL_MERGE_ZEALOUS < level

Urgh, do you know if there's some reason we're not doing level ==
XDL_MERGE_ZEALOUS_ALNUM, or at least level >= XDL_MERGE_ZEALOUS_ALNUM
there?

Re: [PATCH 1/4] merge: simplify initialization

[Felipe Contreras]

Ævar Arnfjörð Bjarmason wrote:
> 
> On Sun, Jun 13 2021, Felipe Contreras wrote:
> 
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  builtin/merge-file.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/builtin/merge-file.c b/builtin/merge-file.c
> > index 06a2f90c48..0186f4156a 100644
> > --- a/builtin/merge-file.c
> > +++ b/builtin/merge-file.c
> > @@ -28,7 +28,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
> >  	const char *names[3] = { NULL, NULL, NULL };
> >  	mmfile_t mmfs[3];
> >  	mmbuffer_t result = {NULL, 0};
> > -	xmparam_t xmp = {{0}};
> > +	xmparam_t xmp = { .level = XDL_MERGE_ZEALOUS_ALNUM };
> >  	int ret = 0, i = 0, to_stdout = 0;
> >  	int quiet = 0;
> >  	struct option options[] = {
> > @@ -48,10 +48,6 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
> >  		OPT_END(),
> >  	};
> >  
> > -	xmp.level = XDL_MERGE_ZEALOUS_ALNUM;
> > -	xmp.style = 0;
> > -	xmp.favor = 0;
> > -
> >  	if (startup_info->have_repository) {
> >  		/* Read the configuration file */
> >  		git_config(git_xmerge_config, NULL);
> 
> Looks good, maybe we can fix the similar code in ll_xdl_merge() while
> we're at it?

Yeah, but since code cleanups for the sake of cleaning up are frowned
upon [1] it's not clear how we would get these merged in, since Elijah
has gunned down the obvious fix in patch 4.

Now the cleanups are orphaned.

> Also, not a problem in your commit, but we check for that constant in
> only one place, as:
> 
>     XDL_MERGE_ZEALOUS < level

My mind can't process that, so...

  level > XDL_MERGE_ZEALOUS

> Urgh, do you know if there's some reason we're not doing level ==
> XDL_MERGE_ZEALOUS_ALNUM, or at least level >= XDL_MERGE_ZEALOUS_ALNUM
> there?

Nope. To the best of my knowledge that's what the code actually meant.

In general the code in xdiff/* seems to be following a very different
style than the rest of git's core.

Cheers.

[1] https://lore.kernel.org/git/xmqqbl87zyra.fsf@gitster.g/

-- 
Felipe Contreras

[PATCH 2/4] merge: fix Yoda conditions

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/merge-file.c | 2 +-
 xdiff/xmerge.c       | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index 0186f4156a..6affccaf19 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -51,7 +51,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	if (startup_info->have_repository) {
 		/* Read the configuration file */
 		git_config(git_xmerge_config, NULL);
-		if (0 <= git_xmerge_style)
+		if (git_xmerge_style >= 0)
 			xmp.style = git_xmerge_style;
 	}
 
diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 1659edb453..ab3448ca88 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -487,7 +487,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,
 		 * "diff3 -m" output does not make sense for anything
 		 * more aggressive than XDL_MERGE_EAGER.
 		 */
-		if (XDL_MERGE_EAGER < level)
+		if (level > XDL_MERGE_EAGER)
 			level = XDL_MERGE_EAGER;
 	}
 
@@ -603,10 +603,10 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,
 	if (!changes)
 		changes = c;
 	/* refine conflicts */
-	if (XDL_MERGE_ZEALOUS <= level &&
+	if (level >= XDL_MERGE_ZEALOUS &&
 	    (xdl_refine_conflicts(xe1, xe2, changes, xpp) < 0 ||
 	     xdl_simplify_non_conflicts(xe1, changes,
-					XDL_MERGE_ZEALOUS < level) < 0)) {
+					level > XDL_MERGE_ZEALOUS) < 0)) {
 		xdl_cleanup_merge(changes);
 		return -1;
 	}
-- 
2.32.0

[PATCH 3/4] xdiff/xmerge: simplify xdl_recs_copy_0

[Felipe Contreras]

Remove one unnecessary indentation level.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 xdiff/xmerge.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index ab3448ca88..b5b3f56f92 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -122,19 +122,19 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int nee
 	for (i = 0; i < count; size += recs[i++]->size)
 		if (dest)
 			memcpy(dest + size, recs[i]->ptr, recs[i]->size);
-	if (add_nl) {
-		i = recs[count - 1]->size;
-		if (i == 0 || recs[count - 1]->ptr[i - 1] != '\n') {
-			if (needs_cr) {
-				if (dest)
-					dest[size] = '\r';
-				size++;
-			}
-
+	if (!add_nl)
+		return size;
+	i = recs[count - 1]->size;
+	if (i == 0 || recs[count - 1]->ptr[i - 1] != '\n') {
+		if (needs_cr) {
 			if (dest)
-				dest[size] = '\n';
+				dest[size] = '\r';
 			size++;
 		}
+
+		if (dest)
+			dest[size] = '\n';
+		size++;
 	}
 	return size;
 }
-- 
2.32.0

[PATCH 4/4] xdiff/xmerge: fix chg0 initialization

[Felipe Contreras]

chg0 is needed when style is XDL_MERGE_DIFF3, xdl_refine_conflicts()
currently is only called when level >= XDL_MERGE_ZEALOUS, which cannot
happen since the level is capped to XDL_MERGE_EAGER.

However, if at some point in the future we decide to not cap diff3, or
introduce a similar uncapped mode, an uninitialized chg0 would cause a
crash.

Let's initialize it to be safe.

Cc: Jeff King <peff@peff.net>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 xdiff/xmerge.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index b5b3f56f92..d9b3a0dccd 100644
--- 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;
-- 
2.32.0

Re: [PATCH 4/4] xdiff/xmerge: fix chg0 initialization

[Elijah Newren]

On Sun, Jun 13, 2021 at 4:04 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> chg0 is needed when style is XDL_MERGE_DIFF3, xdl_refine_conflicts()
> currently is only called when level >= XDL_MERGE_ZEALOUS, which cannot
> happen since the level is capped to XDL_MERGE_EAGER.
>
> However, if at some point in the future we decide to not cap diff3, or
> introduce a similar uncapped mode, an uninitialized chg0 would cause a
> crash.

If this cannot happen now, and is needed by your other posted patch,
why aren't these two patches either combined or posted as part of the
same series?  (I think a separate submission _only_ makes sense if you
explicitly withdraw the other patch from consideration, otherwise it
feels dangerous to submit patches this way).

> Let's initialize it to be safe.

Is it being safe, or is it hacking around a hypothetical future
segfault?  Are these values reasonable, or did you just initialize
them to known garbage rather than letting them be unknown garbage?
Are there other values that need to be changed for the xdiff data
structures to be consistent?  Will future code readers be helped by
this initialization, or will it introduce inconsistencies (albeit
initialized ones) in existing data structures that make it harder for
future readers to reason about?

I'm somewhat worried by the cavalier posting of the zdiff3 patch[1],
and am worried you're continuing more of the same here, especially
since in your previous post[2] you said that you didn't know whether
this particular change made sense and haven't posted anything yet to
state why it does.  Peff already pointed out that you reposted the
zdiff3 patch that had knonw segfaults without even stating as much[3].
That's one thing that needs to be corrected, but I think there are
more that should be pointed out.  Peff linked to the old thread which
is how you found out about the patches, but the old thread also
included things that Junio wanted to see if zdiff3 were to be added as
an option[4], and discussed some concerns about the implementation
that would need to be addressed[5].  Given the fact that Peff liked
the original zdiff3 patch and tried it for over a month and took time
to report on it and argue for such a feature, I would have expected
that when you reposted the zdiff3 patch that you would have provided
some more detailed explanation of how it works and why it's right (and
included some fixes with it rather than separate), and that you would
have included multiple new testcases to the testsuite both to make
sure we don't cause any obvious bugs and to provide some samples of
how the option functions, and also likely include in the cover letter
some kind of explanation of additional testing done to try to assure
us that the new mode is safe to use (e.g. "I ran this on hundreds of
linux kernel commits, with X% of them showing a difference.  They
typically looked like <Y>" or "I've run with this for a month without
issue, and occasionally have re-checked a merge afterwards and found
that the conflicts were much easier to view because of...").  Short of
all that, I would have at least expected the patches to be posted as
RFC and stating any known shortcomings and additional work you planned
on doing after gathering some feedback.

You didn't do any of that, making me wonder whether this patch is a
solid fix, or whether it represents just hacking around the first edge
case you ran into and posting a shot in the dark.  It could well be
either; how are we to know whether we should trust these patches?

(Having read the old zdiff3 thread after Peff pointed to it, I really
like the idea of such an option.  I'd like to see it exist and I would
use it.  But I think it needs to be introduced appropriately,
otherwise it makes it even less likely we'll end up with such a
thing.)

[1] https://lore.kernel.org/git/20210613143155.836591-1-felipe.contreras@gmail.com/
[2] https://lore.kernel.org/git/60c677a2c2d24_f5651208cf@natae.notmuch/
[3] https://lore.kernel.org/git/YMbexfeUG78yBix4@coredump.intra.peff.net/
[4] https://lore.kernel.org/git/7vip42gfjc.fsf@alter.siamese.dyndns.org/
[5] https://lore.kernel.org/git/20130307180157.GA6604@sigill.intra.peff.net/

> Cc: Jeff King <peff@peff.net>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  xdiff/xmerge.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
> index b5b3f56f92..d9b3a0dccd 100644
> --- 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;
> --
> 2.32.0

Re: [PATCH 4/4] xdiff/xmerge: fix chg0 initialization

[Felipe Contreras]

Elijah Newren wrote:
> On Sun, Jun 13, 2021 at 4:04 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > chg0 is needed when style is XDL_MERGE_DIFF3, xdl_refine_conflicts()
> > currently is only called when level >= XDL_MERGE_ZEALOUS, which cannot
> > happen since the level is capped to XDL_MERGE_EAGER.
> >
> > However, if at some point in the future we decide to not cap diff3, or
> > introduce a similar uncapped mode, an uninitialized chg0 would cause a
> > crash.
> 
> If this cannot happen now, and is needed by your other posted patch,
> why aren't these two patches either combined or posted as part of the
> same series?

Because I found the fix *after* I sent the patch, and after Jeff posted
a way to reproduce the issue he experienced in the past.

v2 of the series has the fix included, but I was waiting for feedback on
v1.

> (I think a separate submission _only_ makes sense if you
> explicitly withdraw the other patch from consideration, otherwise it
> feels dangerous to submit patches this way).

More than 50% of my patches get either completely ignored, or commented,
and then ignored. The vast majority of them don't have a valid reason
for rejection.

I thought this series had a better chance of being merged before the
zdiff3 series, and the chg0 is orthogonal to the zdiff3 series anyway
(in my view).

Plus, I don't see the harm in sending the same patch in two different
series. Especially if the chances of both series being merged any time
soon is very low.

> > Let's initialize it to be safe.
> 
> Is it being safe, or is it hacking around a hypothetical future
> segfault?

It's being safe.

> Are these values reasonable,

I spent about two hours of my own free time--with no corporation backing
up my git work--to familiarize myself with the code and the values are
as reasonable as I could make them.

If m->chg0 is 0, xdl_orig_copy will ignore the chunk, so m->i0 is
irrelevant, but just to be consistent I copied the original i0.

Once again: the only value that matters is m->chg0, and it's completely
safe to set it to 0.

Much more reasonable than garbage, like
1774234 which we currently have sometimes.

> or did you just initialize them to known garbage rather than letting
> them be unknown garbage?

No.

> Are there other values that need to be changed for the xdiff data
> structures to be consistent?

No.

> Will future code readers be helped by this initialization,

Yes.

> or will it introduce inconsistencies (albeit initialized ones) in
> existing data structures that make it harder for future readers to
> reason about?

No.

> I'm somewhat worried by the cavalier posting of the zdiff3 patch[1],

When Uwe sent the patch [1] nobody said it was a "cavalier posting".
Jeff King said "I think the patch is correct".

> and am worried you're continuing more of the same here, especially
> since in your previous post[2] you said that you didn't know whether
> this particular change made sense and haven't posted anything yet to
> state why it does.

Nobody pays me to work on git. I comment when I have time. Yesterday was
my birthday and I spent my free time doing other things.

When I sent that mail it was "Sun, 13 Jun 2021 16:24:50 -0500", when I
sent the patch it was "Sun, 13 Jun 2021 17:58:36 -0500". After spending
more than an hour and a half reading the code, and trying different
approaches I became more confident that there was no better approach. At
least not one that was easily found.

I am 99% certain that m->chg0 = 0 is safe, and produces a reasonable
output.

What I didn't know at 16:24 is if there was something better we could
do. But by 17:58 I became much more certain that there wasn't, although
I'm still not sure.

Degrees of confidence vary with time.

> Peff already pointed out that you reposted the zdiff3 patch that had
> knonw segfaults without even stating as much[3].

He knew that. I didn't.

> That's one thing that needs to be corrected, but I think there are
> more that should be pointed out.  Peff linked to the old thread which
> is how you found out about the patches, but the old thread also
> included things that Junio wanted to see if zdiff3 were to be added as
> an option[4],

That thread included many things, including links to other threads.

> and discussed some concerns about the implementation that would need
> to be addressed[5].

That link is a mail from Jeff stating that he disagrees with Junio in at
least one point, and said:

  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.

> Given the fact that Peff liked the original zdiff3 patch and tried it
> for over a month and took time to report on it and argue for such a
> feature, I would have expected that when you reposted the zdiff3 patch
> that you would have provided some more detailed explanation of how it
> works and why it's right (and included some fixes with it rather than
> separate),

I didn't want to override Uwe's wording, especially since both Junio and
Jeff often complain about my commit messages, and it isn't rare that
they end up rewritten.

> and that you would have included multiple new testcases to the
> testsuite both to make sure we don't cause any obvious bugs and to
> provide some samples of how the option functions,

It is not uncommon for v1 of a patch series to be missing something.
Uwe's patch series [1] did not include tests either, and nobody focused
on that. It is completely reasonable to assume that a reboot of the
series wouldn't initially focus on that either.

That being said, I did test zdiff3 with the whole testing framework, and
I did explain how.

> and also likely include in the cover letter some kind of explanation
> of additional testing done to try to assure us that the new mode is
> safe to use (e.g. "I ran this on hundreds of linux kernel commits,
> with X% of them showing a difference.  They typically looked like <Y>"
> or "I've run with this for a month without issue, and occasionally
> have re-checked a merge afterwards and found that the conflicts were
> much easier to view because of...").

I typically don't send cover letters for single patches (just like Uwe
didn't [1]), but I did add an explanation of what tests I ran below the
commit message of the patch, as is customary.

> Short of all that, I would have at least expected the patches to be
> posted as RFC and stating any known shortcomings and additional work
> you planned on doing after gathering some feedback.

I had not planned to do any additional work, as I don't usually plan how
I spend my free time. I happened to have free time when I saw Jeff mail
about a way to reproduce and I felt motivated to investigate further.
One thing lead to another and fortunately I found the fix quickly
enough.

> You didn't do any of that, making me wonder whether this patch is a
> solid fix, or whether it represents just hacking around the first edge
> case you ran into and posting a shot in the dark.  It could well be
> either; how are we to know whether we should trust these patches?

By assuming good faith [2], and simply asking questions. I don't think
assuming the worst and calling into question the competence of a
contributor is a good approach.


Morevoer, this is not *my* patch, this is a patch from the community, to
the community, and it's in the best interest of the community that it
gets developed *collaboratively*.

I took Uwe's patch and added my two cents, not for me, but for the
community. I don't need zdiff3, I'm perfectly fine with diff3, but I
would like a better default conflict style than merge, for the
community. That's why I spend a considerable amount of time reading the
old threads, and familiarizing myself with the xdiff/xmerge code of
which I basically knew nothing about.

This is something I wish more people did, and then perhaps we wouldn't
need to wait 8 years for a simple crash fix for a feature many people
probably would like, which again, I'm 99% certain is correct.


To be crystal clear: this is not *my itch*, I did the patch
altrusticially for the community. The fix correct--at least to the best
knowledge of everyone involved so far. If you want to deride the hours I
spent working for the community for free which resulted in a patch that
most likely is a net positive, feel free, I think this doesn't help the
community, which needs more of this kind of work, not less.

Cheers.

[1] https://lore.kernel.org/git/1362602202-29749-1-git-send-email-u.kleine-koenig@pengutronix.de/
[2] https://en.wikipedia.org/wiki/Wikipedia:Assume_good_faith

-- 
Felipe Contreras

Re: [PATCH 4/4] xdiff/xmerge: fix chg0 initialization

[Elijah Newren]

On Tue, Jun 15, 2021 at 12:51 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Elijah Newren wrote:
> > On Sun, Jun 13, 2021 at 4:04 PM Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> > >
...
> > I'm somewhat worried by the cavalier posting of the zdiff3 patch[1],
>
> When Uwe sent the patch [1] nobody said it was a "cavalier posting".
> Jeff King said "I think the patch is correct".

The difference here is that Jeff reported segfaults after Uwe sent the
patch.  Uwe could not have read those reports before he posted the
patch.  You did read those emails before re-posting the patch.

> > Peff already pointed out that you reposted the zdiff3 patch that had
> > knonw segfaults without even stating as much[3].
>
> He knew that. I didn't.

You knew he had reported it as a possibility.  That behooved you to
try to investigate, either by asking him for details to try to
reproduce, or attempt it on a large range of merges, and then to
report the results when you reposted the patch.  Or, at the absolute
minimum, to at least report Peff's report along with your reposting of
the patch.

> > and that you would have included multiple new testcases to the
> > testsuite both to make sure we don't cause any obvious bugs and to
> > provide some samples of how the option functions,
>
> It is not uncommon for v1 of a patch series to be missing something.
> Uwe's patch series [1] did not include tests either, and nobody focused
> on that. It is completely reasonable to assume that a reboot of the
> series wouldn't initially focus on that either.

Not when folks spent several emails in response to the original about
the finer points of how splitting should or should not work.  Nor when
folks had reported segfaults with the original patch.  With that
background, I'd say it's completely unreasonable to assume that a
reboot of the series comes without any tests unless marked as RFC.

> > You didn't do any of that, making me wonder whether this patch is a
> > solid fix, or whether it represents just hacking around the first edge
> > case you ran into and posting a shot in the dark.  It could well be
> > either; how are we to know whether we should trust these patches?
>
> By assuming good faith [2], and simply asking questions. I don't think
> assuming the worst and calling into question the competence of a
> contributor is a good approach.

I did not assume the worst with your patches.  I assumed good faith
until I was provided with strong counter-evidence as perhaps best
summarized at https://lore.kernel.org/git/YMbexfeUG78yBix4@coredump.intra.peff.net/.
That made it clear something was heavily problematic.  If you hadn't
reposted patches for which there were reported segfaults without
saying anything about those reports, I would have asked you none of
those pointed questions.  In fact, I probably wouldn't have asked them
if your original response to Peff were along the lines of "sorry,
won't do that again" rather than "I don't know how I was supposed to
investigate the few segfaults you mentioned. All you said is that you
never tracked the bug."  It was precisely that cavalier attitude that
made me question this patch in this manner.

> If you want to deride the hours I
> spent working for the community for free which resulted in a patch that
> most likely is a net positive, feel free, I think this doesn't help the
> community, which needs more of this kind of work, not less.

I was not deriding your work.  I was asking deep and pointed questions
due to the cavalier manner in which you were posting, and which you
seem to be doubling down on.  I would say that after
https://lore.kernel.org/git/YMhx2BFlwUxZ2aFJ@coredump.intra.peff.net/,
I'd have to agree with Peff that you have displayed a level of
carelessness with which I am not comfortable for the project.  That
kind of carelessness leads to skepticism in others, moreso when you
double down on it.  It makes me think that if I review any of your
future patches, I need to check them far more rigorously because you
are willing to eschew what I view as even the most basic of
responsibilities in ensuring you are submitting valid patches, and
even worse, you are unwilling to change how you approach the project
even when those are pointed out to you.