git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: Matheus Tavares <matheus.bernardino@usp.br>,
	git@vger.kernel.org, avarab@gmail.com
Subject: Re: [PATCH v3 2/3] t0021: implementation the rot13-filter.pl script in C
Date: Tue, 9 Aug 2022 12:00:02 +0200 (CEST)	[thread overview]
Message-ID: <439p713r-32o4-5187-n8nn-r81n3007s4pp@tzk.qr> (raw)
In-Reply-To: <xmqqr11zoe6i.fsf@gitster.g>

Hi Junio,

On Mon, 1 Aug 2022, Junio C Hamano wrote:

> Matheus Tavares <matheus.bernardino@usp.br> writes:
>
> > +		/* Read until flush */
> > +		while ((buf = packet_read_line(0, &size))) {
> > +			if (!strcmp(buf, "can-delay=1")) {
> > +				entry = strmap_get(&delay, pathname);
> > +				if (entry && !entry->requested) {
> > +					entry->requested = 1;
> > +				} else if (!entry && always_delay) {
> > +					add_delay_entry(pathname, 1, 1);
> > +				}
>
> These are unnecessary {} around single statement blocks, but let's
> let it pass in a test helper.

I would like to encourage you to think of ways how this project could
avoid the cost (mental space, reviewer time, back and forth between
contributor and reviewer) of such trivial code formatting issues.

My favored solution would be to adjust the code formatting rules in Git to
such an extent that it can be completely automated, whether via a
`clang-format-diff` rule [*1*] or via an adapted `checkpatch` [*2*] or via
something that is modeled after cURL's `checksrc` script [*3*].

It costs us too much time, and is too annoying all around, having to spend
so many brain cycles on code style (which people like me find much less
interesting than the actual, functional changes).

I'd much rather focus on the implementation of the rot13 filter and
potentially how this patch could give rise to even broader enhancements to
Git's source code that eventually have a user-visible, positive impact.

Ciao,
Dscho

Footnote *1*: https://lore.kernel.org/git/YstJl+5BPyR5RWnR@tapette.crustytoothpaste.net/
Footnote *2*: https://lore.kernel.org/git/xmqqbktvl0s4.fsf@gitster.g/
Footnote *3*: https://github.com/curl/curl/blob/master/scripts/checksrc.pl

  parent reply	other threads:[~2022-08-09 10:00 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-22 19:42 [PATCH 0/2] t0021: convert perl script to C test-tool helper Matheus Tavares
2022-07-22 19:42 ` [PATCH 1/2] t/t0021: convert the rot13-filter.pl script to C Matheus Tavares
2022-07-23  4:52   ` Ævar Arnfjörð Bjarmason
2022-07-23  4:59   ` Ævar Arnfjörð Bjarmason
2022-07-23 13:36     ` Matheus Tavares
2022-07-22 19:42 ` [PATCH 2/2] t/t0021: replace old rot13-filter.pl uses with new test-tool cmd Matheus Tavares
2022-07-24 15:09 ` [PATCH v2] t/t0021: convert the rot13-filter.pl script to C Matheus Tavares
2022-07-28 16:58   ` Johannes Schindelin
2022-07-28 17:54     ` Junio C Hamano
2022-07-28 19:50     ` Ævar Arnfjörð Bjarmason
2022-07-31  2:52     ` Matheus Tavares
2022-08-09  9:36       ` Johannes Schindelin
2022-07-31 18:19   ` [PATCH v3 0/3] t0021: convert perl script to C test-tool helper Matheus Tavares
2022-07-31 18:19     ` [PATCH v3 1/3] t0021: avoid grepping for a Perl-specific string at filter output Matheus Tavares
2022-08-01 20:41       ` Junio C Hamano
2022-07-31 18:19     ` [PATCH v3 2/3] t0021: implementation the rot13-filter.pl script in C Matheus Tavares
2022-08-01 11:33       ` Ævar Arnfjörð Bjarmason
2022-08-02  0:16         ` Matheus Tavares
2022-08-09  9:45           ` Johannes Schindelin
2022-08-01 11:39       ` Ævar Arnfjörð Bjarmason
2022-08-01 21:18       ` Junio C Hamano
2022-08-02  0:13         ` Matheus Tavares
2022-08-09 10:00         ` Johannes Schindelin [this message]
2022-08-10 18:37           ` Junio C Hamano
2022-08-10 19:58             ` Junio C Hamano
2022-08-09 10:37         ` Johannes Schindelin
2022-08-09 10:47       ` Johannes Schindelin
2022-07-31 18:19     ` [PATCH v3 3/3] tests: use the new C rot13-filter helper to avoid PERL prereq Matheus Tavares
2022-08-15  1:06     ` [PATCH v4 0/3] t0021: convert perl script to C test-tool helper Matheus Tavares
2022-08-15  1:06       ` [PATCH v4 1/3] t0021: avoid grepping for a Perl-specific string at filter output Matheus Tavares
2022-08-15  1:06       ` [PATCH v4 2/3] t0021: implementation the rot13-filter.pl script in C Matheus Tavares
2022-08-15  1:06       ` [PATCH v4 3/3] tests: use the new C rot13-filter helper to avoid PERL prereq Matheus Tavares
2022-08-15 13:01       ` [PATCH v4 0/3] t0021: convert perl script to C test-tool helper Johannes Schindelin
2022-08-19 22:17         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=439p713r-32o4-5187-n8nn-r81n3007s4pp@tzk.qr \
    --to=johannes.schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=matheus.bernardino@usp.br \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).