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

From: David Symonds <dsymonds@gmail.com>
Date: Nov 3, 2007, 02:35 UTC
Message-ID: <ee77f5c20711021935u16fe0eb2n202dfca3fc220e2d@mail.gmail.com>
In-Reply-To: <7v3avo2z6x.fsf@gitster.siamese.dyndns.org>

On 11/3/07, Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

> David Symonds <dsymonds@gmail.com> writes:
>
> > 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).

Sure, I thought there might be. I'm still finding my way around the git code.

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

I can whip that up in a separate patch if you'd like. However, it still doesn't allow for different handling of the different whitespace errors, does it?

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

That seems a little counterintuitive to be splitting like that. For overriding, a simple environment variable like GIT_EXTRA_CONFIG (or whatever) could pass in arbitrary one-shot configuration parameters, which seems like a better (more general) solution.

Show 6 quoted lines

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

Yes, I agree that there's no place for "error" or "fix" in git-diff; that was the reason for me resending the series, because I adjusted the diff warnings to happen whenever the relevant setting was anything but "okay", with the idea being that any incorrect whitespace should be flagged in git-diff, and there's a natural split between "okay" and "warn"/"error"/"fix".

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

Yes, you're correct. That's easy to fix.

Dave.