Subject: Re: [PATCH v3 05/11] commit-reach: start reporting errors in `paint_down_to_common()`

From: Johannes Schindelin <[email-masked]>
Date: Tue, 27 Feb 2024 16:08:16 +0100 (CET)

Hi Dirk,

On Tue, 27 Feb 2024, Dirk Gouders wrote:

> "Johannes Schindelin via GitGitGadget" <[email-masked]> writes:
>
> > Let's start at the bottom of the stack by teaching the
> > `paint_down_to_common()` function to return an `int`: if negative, it
> > indicates fatal error, if 0 success.
>
> Kind of pedantic but the above doesn't describe the real change, i.e. a
> value != 0 indicates a fatal error:
>
> > -		common = paint_down_to_common(r, array[i], filled,
> > -					      work, min_generation, 0);
> > +		if (paint_down_to_common(r, array[i], filled,
> > +					 work, min_generation, 0, &common)) {

The fact that we do not bother to verify that the return value is
negative, but only check for a non-zero one instead, does not change the
fact that in the form this patch leaves the code, `paint_down_to_common()`
returns -1 for fatal errors and 0 for success, as advertised, though.

Is it lazy to omit the `< 0` here? Not actually, the reason why I omitted
it here was to stay under 80 columns per line.

Good eyes, though. If `paint_down_to_common()` _did_ return values other
than -1 and 0, in particular positive ones that would not indicate a fatal
error, the hunk under discussion would have introduced a problematic bug.

Ciao,
Johannes

> > +			clear_commit_marks(array[i], all_flags);
> > +			clear_commit_marks_many(filled, work, all_flags);
> > +			free_commit_list(common);
> > +			free(work);
> > +			free(redundant);
> > +			free(filled_index);
> > +			return -1;
> > +		}
>
> Dirk
>
>