Re: [PATCH v2] merge-tree: fix segmentation fault in read-only repositories

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Wed, 21 Sep 2022, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > +	const struct object_id *tree_oid;
> >
> >  	parent1 = get_merge_parent(branch1);
> >  	if (!parent1)
> > @@ -446,7 +447,8 @@ static int real_merge(struct merge_tree_options *o,
> >  	if (o->show_messages == -1)
> >  		o->show_messages = !result.clean;
> >
> > -	printf("%s%c", oid_to_hex(&result.tree->object.oid), line_termination);
> > +	tree_oid = result.tree ? &result.tree->object.oid : null_oid();
> > +	printf("%s%c", oid_to_hex(tree_oid), line_termination);
>
> As we are not using tree_oid anywhere else, it might be cleaner to
> switch on result.tree more explicitly, especially given that the
> return statement also uses result.tree to decide what exit status to
> use, e.g.
>
> 	if (result.tree)
> 		fputs(oid_to_hex(&result.tree->object.oid), stdout);
> 	else
> 		fputs(oid_to_hex(null_oid()), stdout);
> 	fputc(line_termination, stdout);
>
> but that is merely "might be cleaner".

I would call it "more readable", even, not just "cleaner" :-)

> > -	return !result.clean; /* result.clean < 0 handled above */
> > +	return !result.tree || !result.clean; /* result.clean < 0 handled above */
>
> The original returned 1 when we failed to cleanly merge and 0 when
> we cleanly merged.  Now, if we failed to come up with a merged tree,
> result.tree would be NULL and we return 1 from the function, because
> we failed to cleanly merge.
>
> OK.  These negations make my head spin X-<.

And here I thought that you couldn't possibly not like if I didn't refrain
from not avoiding double negations...

Ciao,
Dscho