Re: [PATCH 1/2] Implement parsing for new core.whitespace.* options.

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 2, 2007, 20:02 UTC
Message-ID: <7v3avo2z6x.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <11940160932021-git-send-email-dsymonds@gmail.com>

David Symonds <dsymonds@gmail.com> writes:

Show 6 quoted lines

> Each of the new core.whitespace.* options (enumerated below) can be set to one
> of:
> 	* okay (default): Whitespace of this type is okay
> 	* warn: Whitespace of this type should be warned about
> 	* error: Whitespace of this type should raise an error
> 	* autofix: Whitespace of this type should be automatically fixed

Many problems at the conceptual level (I haven't look at the patch yet).

We call these options (nowarn,warn,error,strip) in apply.whitespace. "strip" is a bit of misnomer, as we only handled the trailing whitespace initially. We should add "fix" as a synonym to "strip".

The intention is to define what is an anomaly with core.whitespace and then define what to do with it with apply.whitespace.

And that is a good distinction. You may usually use "fix", but occasionally you would want to override it one-shot (when you do want to have byte-to-byte identical application of the patch), and the command line option "--whitespace=" lets you do so. At least you need to extend --whitespace command line option handling to allow these overridden.

Adding the "error" and "fix" to "diff" is a mistake --- there is no error condition nor fixing there. That shows how the approach of your patch is inappropriate by trying to mix what core.whitespace (give the definition of what is an error) and apply.whitespace (specify what to do with an error) are designed to do.

Defaulting to "nowarn" is wrong. Trailing whitespace errors and space before tab errors should be turned on by default as before.