Re: [RFC 1/1] Add new indent-with-tab whitespace check

From: Chris Webb <chris@arachsys.com>
Date: Apr 2, 2010, 10:55 UTC
Message-ID: <20100402105504.GO19047@arachsys.com>
In-Reply-To: <7vsk7fovjx.fsf_-_@alter.siamese.dyndns.org>

Hi. Thanks for the detailed feedback.

Junio C Hamano <gitster@pobox.com> writes:

> This sounds more like "tab-in-indent".  Naming the option that way would
> give it an added benefit of allowing it to be abbreviated to "tab" by the
> parser.

Yes that's nice: much harder to confuse with indent-with-non-tab that way.

> I think WS_TAB_IN_INDENT are incompatible with either WS_SPACE_BEFORE_TAB
> or WS_INDENT_WITH_TAB.  Don't we want to notice such a combination as an
> error?

Oh yes, that has to be the right behaviour for the finished feature here. (I guess strictly they could coexist with the meaning 'no indented lines allowed at all' but I don't think that's useful or sensible.)

Show 9 quoted lines

> I'll queue this with s/indent-with-tab/tab-in-indent/ tweak to 'pu' but
> the topic would need:
> 
>  - detecting incompatible settings;
>  - tests;
>  - docs;
>  - matching change to apply --whitespace=fix;
> 
> before moving to 'next', I think.

Are you happy with my suggestion in [0/1] to replumb ws_fix_copy() to take a strbuf as dst rather than a char *, to deal with the fact that whitespace can grow with --whitespace=fix and WS_TAB_IN_INDENT?

If so, I'll polish up the existing patch as you suggest in your first three points above, and then make this change to the way ws_fix_copy() works so I can do the right thing in whitespace=fix.

Show 5 quoted lines

> There is another issue with this change.  Because "whitespace" without any
> string in .gitattributes are defined to cause all the whitespace breakages
> known to git to be caught, and tab-in-indent is inherently incompatible
> with indent-with-non-tab, this cannot be supported without changing the
> definition of "default set of whitespace breakage classes".

Ah, this was a behaviour I didn't know about. I should probably put your patch (without the WS_TAB_IN_INDENT line) as the first one in the series, before I introduce WS_TAB_IN_INDENT, so we don't break this wildcard whitespace behaviour at any point in the series.

Cheers,

Chris.