Re: [PATCH 1/5] "diff --check" should affect exit status

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

Wincent Colaiuta <win@wincent.com> writes:

> @@ -2965,8 +2976,8 @@ void diff_flush(struct diff_options *options)
>  			     DIFF_FORMAT_CHECKDIFF)) {
>  		for (i = 0; i < q->nr; i++) {
>  			struct diff_filepair *p = q->queue[i];
> -			if (check_pair_status(p))
> -				flush_one_pair(p, options);
> +			if (check_pair_status(p) && flush_one_pair(p, options))
> +				DIFF_OPT_SET(options, CHECK_FAILED);
>  		}
>  		separator++;
>  	}

Isn't this wrong when check is not in effect?

I think highjacking the "did we encounter problems" return value of the
entire callchain for the purpose of checkdiff is very ugly and wrong to
begin with, so please do not argue "but if checkdiff is not in effect,
the caller does not check CHECK_FAILED".

Wouldn't it be much cleaner to make diff_flush_checkdiff(), or its
underlying function run_checkdiff(), set that CHECK_FAILED flag to
options structure, and return success?  The toplevel caller can decide
to exit with non-zero when --check is in effect and CHECK_FAILED flag is
set.