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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 27, 2024, 15:08 UTC
Message-ID: <fa8ebbfd-e01f-fbde-c851-54c162b610ff@gmx.de>
In-Reply-To: <gh34tenefb.fsf@gouders.net>

Hi Dirk,

On Tue, 27 Feb 2024, Dirk Gouders wrote:

Show 13 quoted lines

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> 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

Show 12 quoted lines

> > +			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
>
>