From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 28 Oct 2025 06:39:04 GMT
Subject: Re: [PATCH] whitespace: correct bit assignment comments
Message-ID: <aQBlCCDWMhLX_aBa@pks.im>
In-Reply-To: <xmqqfrb4hyjl.fsf@gitster.g>

On Mon, Oct 27, 2025 at 01:36:46PM -0700, Junio C Hamano wrote:
> diff --git a/diff.h b/diff.h
> index 2fa256c3ef..60749154e7 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -331,9 +331,9 @@ struct diff_options {
>  
>  	int ita_invisible_in_index;
>  /* white-space error highlighting */
> -#define WSEH_NEW (1<<12)
> -#define WSEH_CONTEXT (1<<13)
> -#define WSEH_OLD (1<<14)
> +#define WSEH_NEW	(1<<12)
> +#define WSEH_CONTEXT	(1<<13)
> +#define WSEH_OLD	(1<<14)
>  	unsigned ws_error_highlight;
>  	const char *prefix;
>  	int prefix_length;

Here you're using tabs for indentation, whereas below you use spaces. We
should probably be consistent.

> diff --git a/ws.h b/ws.h
> index 5ba676c559..611c6b6d50 100644
> --- a/ws.h
> +++ b/ws.h
> @@ -7,19 +7,19 @@ struct strbuf;
>  /*
>   * whitespace rules.
>   * used by both diff and apply
> - * last two digits are tab width
> + * last two octal-digits are tab width (we support only up to 63).
>   */
> -#define WS_BLANK_AT_EOL         0100
> -#define WS_SPACE_BEFORE_TAB     0200
> -#define WS_INDENT_WITH_NON_TAB  0400
> -#define WS_CR_AT_EOL           01000
> -#define WS_BLANK_AT_EOF        02000
> -#define WS_TAB_IN_INDENT       04000
> -#define WS_TRAILING_SPACE      (WS_BLANK_AT_EOL|WS_BLANK_AT_EOF)
> +#define WS_BLANK_AT_EOL         (1<<6)
> +#define WS_SPACE_BEFORE_TAB     (1<<7)
> +#define WS_INDENT_WITH_NON_TAB  (1<<8)
> +#define WS_CR_AT_EOL            (1<<9)
> +#define WS_BLANK_AT_EOF         (1<<10)
> +#define WS_TAB_IN_INDENT        (1<<11)
> +#define WS_TRAILING_SPACE       (WS_BLANK_AT_EOL|WS_BLANK_AT_EOF)
>  #define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB|8)

The "8" here is a bit curious, but this matches what the comment says:
the last two digits are the tab width, and there of course is no macro
for that.

> -#define WS_TAB_WIDTH_MASK        077
> +#define WS_TAB_WIDTH_MASK       ((1<<6)-1)
>  /* All WS_* -- when extended, adapt diff.c emit_symbol */
> -#define WS_RULE_MASK           07777
> +#define WS_RULE_MASK            ((1<<12)-1)
>  extern unsigned whitespace_rule_cfg;
>  unsigned whitespace_rule(struct index_state *, const char *);
>  unsigned parse_whitespace_rule(const char *);

All of these conversion look correct to me, and I agree that this is
easier to read.

Thanks!

Patrick