Re: [PATCH v2 2/5] test-ref-store: tweaks to for-each-reflog-ent format

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 7, 2021, 15:53 UTC
Message-ID: <87fsugl6en.fsf@evledraar.gmail.com>
In-Reply-To: <CAFQ2z_Oen0Q5hfpbZdzWgJ8D=EetBNGx7sQ3ZxnJSXC-tSKGYw@mail.gmail.com>

On Tue, Sep 07 2021, Han-Wen Nienhuys wrote:

Show 19 quoted lines

> On Tue, Sep 7, 2021 at 12:35 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> > -     printf("%s %s %s %"PRItime" %d %s\n",
>> > -            oid_to_hex(old_oid), oid_to_hex(new_oid),
>> > -            committer, timestamp, tz, msg);
>> > +     printf("%s %s %s %" PRItime " %+05d\t%s\n", oid_to_hex(old_oid),
>> > +            oid_to_hex(new_oid), committer, timestamp, tz, msg);
>>
>> Nit: Would be a more readable diff if this wasn't a
>> line-wrap-while-at-it change in addition to changing the format string.
>>
>> I.e. the last 4x parameters aren't changed, so leaving them on their own
>> line & just changing the string & the two oid_to_hex()...
>
> This is clang-format's output, and the new code takes up less lines vertically.
>
> If you think this is really, really important, I can change it, but I
> think it's a better use of everyone's time to leave mechanical tasks
> (like formatting) to the machines.

It's not "really, really important" or even "really important", just notes while reading the series.

Patches are read N times, including during review. Since humans read formatting changes, it's in general are not something we can leave to machines. We're after all looking at the patch on-list, not diffing two versions of the generated machine code, or of clang-format's output.

I don't think the intent of the clang-format target is to run it before patch submission, but to serve as input on suggested formatting changes.

If on master you run:

    git ls-files '*.[ch]' -z | xargs -n 1 -0 clang-format -i

You'll get:

    631 files changed, 48246 insertions(+), 45061 deletions(-)

Which I think speaks for itself in the likelyhood that "git-clang-format" is going to inject unnecessary churn into submitted patches.

I think this case is quite small and not worth a re-roll.