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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 13, 2007, 23:51 UTC
Message-ID: <7vir32ywyz.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1197552751-53480-2-git-send-email-win@wincent.com>

Wincent Colaiuta <win@wincent.com> writes:

Show 11 quoted lines

> @@ -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.