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

["David Symonds" <dsymonds@gmail.com>]

On 11/3/07, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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