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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Han-Wen Nienhuys <hanwen@google.com>
Cc: Han-Wen Nienhuys via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Taylor Blau <me@ttaylorr.com>,
	Han-Wen Nienhuys <hanwenn@gmail.com>
Subject: Re: [PATCH v2 2/5] test-ref-store: tweaks to for-each-reflog-ent format
Date: Tue, 07 Sep 2021 17:53:07 +0200	[thread overview]
Message-ID: <87fsugl6en.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <CAFQ2z_Oen0Q5hfpbZdzWgJ8D=EetBNGx7sQ3ZxnJSXC-tSKGYw@mail.gmail.com>

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

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