Re: [PATCH 1/8] clang-format: indent preprocessor directives after hash

[Junio C Hamano <gitster@pobox.com>]

Karthik Nayak <karthik.188@gmail.com> writes:

> We do not have a rule around the indentation of preprocessor directives.
> This was also discussed on the list [1], noting how there is often
> inconsistency in the styling. While there was discussion, there was no
> conclusion around what is the preferred style here. One style being
> indenting after the hash:
>
>     #if FOO
>     #  if BAR
>     #    include <foo>
>     #  endif
>     #endif
>
> The other being before the hash:
>
>     #if FOO
>       #if BAR
>         #include <foo>
>       #endif
>     #endif
>
> Let's pick the former and add 'IndentPPDirectives: AfterHash' value to
> our '.clang-format'. There is no clear reason to pick one over the
> other, but it would definitely be nicer to be consistent.

When I experimented with reindenting the CPP directives in
<git-compat-util.h> [*], I think I saw an existing violation in an
early part of the file.  Outside the borrowed code in compat/, we
have these:

    $ git grep -e '^[ 	]\{1,\}#' master -- ':!compat/' \*.[ch]
    blame.c:	#define FINGERPRINT_FILE_THRESHOLD	10
    block-sha1/sha1.c:  #define setW(x, val) (*(volatile unsigned int *)&W(x) = (val))
    block-sha1/sha1.c:  #define setW(x, val) do { W(x) = (val); __asm__("":::"memory"); } while (0)
    block-sha1/sha1.c:  #define setW(x, val) (W(x) = (val))
    diff.h:	#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
    diff.h:	#define COLOR_MOVED_MIN_ALNUM_COUNT 20
    diff.h:	#define COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE (1<<5)
    diff.h:	#define COLOR_MOVED_WS_ERROR (1<<0)
    git-compat-util.h: #define GIT_GNUC_PREREQ(maj, min) 0
    pkt-line.c:	#define hex(a) (hexchar[(a) & 15])
    pkt-line.c:	#undef hex
    sha1dc/sha1.c:	#define sha1_load(m, t, temp)  { temp = m[t]; }
    sha1dc/sha1.c:	#define sha1_load(m, t, temp)  { temp = m[t]; sha1_bswap32(temp); }

Should we clean them up before we start adding these rules to the
file, especially if we plan to run the rules for stylistic check?
Otherwise wouldn't we see noises coming from the existing lines that
may dwarf the new ones, whose addition we want prevent?

If we were to run the check in CI to help contributors, I would
assume that you are arranging it to only complain about the lines
touched by the commits they are contributing, not about the existing
style violations.  This comment is not limited to the CPP directive
indentation but any other style rules .clang-format defines.

If we are not checking only for lines affected by commits being
contributed, then perhaps we should exclude compat/ from these
rules?

Thanks for working on this.

[Reference]

 * https://lore.kernel.org/git/xmqqjzim197j.fsf@gitster.g/