Re: [PATCH v2 10/11] bisect: check get_terms return at all call sites

[Junio C Hamano <gitster@pobox.com>]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> There is one slight complication here: One caller _needs_ the return
> value to indicate an error when the `BISECT_TERMS` file is absent, all
> the other call sites are totally okay with a "missing" `BISECT_TERMS`
> file. To address that, extend the function signature of `get_terms()` to
> indicate which behavior the caller wants.

> -static int get_terms(struct bisect_terms *terms)
> +static int get_terms(struct bisect_terms *terms, int file_missing_is_ok)
>  {
>  	struct strbuf str = STRBUF_INIT;
>  	FILE *fp = NULL;
> @@ -493,7 +493,7 @@ static int get_terms(struct bisect_terms *terms)
>  
>  	fp = fopen(git_path_bisect_terms(), "r");
>  	if (!fp) {
> -		res = -1;
> +		res = file_missing_is_ok ? 0 : -1;
>  		goto finish;
>  	}

Hmph.  So, depending on the caller, a missing file error may have to
be treated as OK or as an error, while all other kinds of anomalies
are treated by all callers as errors.

As all the existing callsites of this function need to be adjusted
for this change anyway, I would have thought a more typical way to
handle a situation like this would be to define different error
codes for this function and have the callers deal with them.  But it
seems that almost all callers, except for one, pass "missing is OK."

So, instead of adjusting the majority of callers with something like:

        -       if (get_terms(...))
        +       if (get_terms(...) == BISECT_TERMS_ERROR)
                        oops we got an error

and keeping only the single oddball caller to barf on any non-zero
return, 

        -       if (get_terms(...))
        +       switch (get_terms(...)) {
	+	case BISECT_TERMS_ERROR:
                        oops we got an error
	+		break;
	+	case BISECT_TERMS_MISSING_FILE:
	+		deal with the missing file error
	+		break;
	+	default:
	+		break; /* ok */
	+	}

it may be simpler to change:

        -       if (get_terms(...))
        +       if (get_terms(..., 1))
                        oops we got an error

for the majority of them.  The one oddball caller then becomes:

        -       if (get_terms(...))
        +       if (get_terms(..., 0))
                        oops we got an error

to treat a missing file as an error as well.

I guess I can buy that.

If get_terms() were a public function that had many more callers,
my preference would probably be very different.  But this is local
to a single file, so the meaning of the mysterious 0/1 parameter
will quickly become evident to those who have to work with this
part of the system anyway.

Thanks.