git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Philippe Blain" <levraiphilippeblain@gmail.com>
Subject: Re: [PATCH v2 2/9] help.c: use puts() instead of printf{,_ln}() for consistency
Date: Wed, 23 Feb 2022 22:57:06 +0100	[thread overview]
Message-ID: <220223.86h78pb5nd.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <xmqqfso9s14j.fsf@gitster.g>


On Wed, Feb 23 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> -		printf("\n%s\n", _(desc));
>> +		putchar('\n');
>> +		puts(_(desc));
>
> This is sort of "Meh".  Even the justification that says "we'll do
> the same thing in future patches" is not really a justification, as
> it is entirely fine to add more of the "line-break plus %\n" printf()
> in the later steps in the same series.

Yes, I agree that justification wouldn't make sense, "let's change this
for consistency with code that doesn't exist yet and I'm about to
introduce" is a non-starter as an argument.

But that's not what I mentioned in the commit message or why I changed
this, as noted it's for doing the same "as other existing code in the
file does".

I.e. you'll see that adjacent and related code if you run this on
master:

    git grep -W '\\n' -- help.c

Now, I fully agree that's not a *strong* reason to change this, it could
just be left in place.

I just thought post-series that skimming through those related functions
made for marginally easier reading if they all used the same pattern to
accomplish the same thing.

In any case, I don't at all feel strongly about including this change,
so I can drop it if you'd like. I just wanted to clarify why I changed
it.

>>  		print_command_list(cmds, mask, longest);
>>  	}
>>  	free(cmds);
>> @@ -317,7 +318,7 @@ void list_commands(struct cmdnames *main_cmds, struct cmdnames *other_cmds)
>>  	}
>>  
>>  	if (other_cmds->cnt) {
>> -		printf_ln(_("git commands available from elsewhere on your $PATH"));
>> +		puts(_("git commands available from elsewhere on your $PATH"));
>
> This *IS* an improvement, as the first parameter to printf_ln() is
> supposed to be a format string, and should have been
>
> 	printf_ln("%s", _("git commands ..."));
>
>> -	printf_ln(_("See 'git help <command>' to read about a specific subcommand"));
>> +	puts(_("See 'git help <command>' to read about a specific subcommand"));
>
> Ditto.


  reply	other threads:[~2022-02-23 22:05 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-28 15:35 [PATCH 0/7] help: test and fix small "help -g" regression Ævar Arnfjörð Bjarmason
2021-12-28 15:35 ` [PATCH 1/7] help doc: add missing "]" to "[-a|--all]" Ævar Arnfjörð Bjarmason
2021-12-28 15:35 ` [PATCH 2/7] help.c: use puts() instead of printf{,_ln}() for consistency Ævar Arnfjörð Bjarmason
2021-12-28 15:35 ` [PATCH 3/7] help tests: test "git" and "git help [-a|-g] spacing Ævar Arnfjörð Bjarmason
2021-12-28 15:35 ` [PATCH 4/7] help.c: split up list_all_cmds_help() function Ævar Arnfjörð Bjarmason
2021-12-28 15:35 ` [PATCH 5/7] help: error if [-a|-g|-c] and [-i|-m|-w] are combined Ævar Arnfjörð Bjarmason
2021-12-28 16:18   ` Eric Sunshine
2021-12-29  0:04     ` Ævar Arnfjörð Bjarmason
2021-12-28 15:35 ` [PATCH 6/7] help: add --no-[external-commands|aliases] for use with --all Ævar Arnfjörð Bjarmason
2021-12-28 16:28   ` Eric Sunshine
2021-12-28 15:35 ` [PATCH 7/7] help: don't print "\n" before single-section output Ævar Arnfjörð Bjarmason
2022-02-21 19:38 ` [PATCH v2 0/9] help: tests, parse_options() sanity, fix "help -g" regression Ævar Arnfjörð Bjarmason
2022-02-21 19:38   ` [PATCH v2 1/9] help doc: add missing "]" to "[-a|--all]" Ævar Arnfjörð Bjarmason
2022-02-21 19:38   ` [PATCH v2 2/9] help.c: use puts() instead of printf{,_ln}() for consistency Ævar Arnfjörð Bjarmason
2022-02-23 21:51     ` Junio C Hamano
2022-02-23 21:57       ` Ævar Arnfjörð Bjarmason [this message]
2022-02-21 19:38   ` [PATCH v2 3/9] help tests: test "git" and "git help [-a|-g] spacing Ævar Arnfjörð Bjarmason
2022-02-21 19:38   ` [PATCH v2 4/9] help.c: split up list_all_cmds_help() function Ævar Arnfjörð Bjarmason
2022-02-23 22:03     ` Junio C Hamano
2022-02-21 19:38   ` [PATCH v2 5/9] help: note the option name on option incompatibility Ævar Arnfjörð Bjarmason
2022-02-23 22:04     ` Junio C Hamano
2022-02-21 19:38   ` [PATCH v2 6/9] help: correct usage & behavior of "git help --all" Ævar Arnfjörð Bjarmason
2022-02-23 22:12     ` Junio C Hamano
2022-02-21 19:38   ` [PATCH v2 7/9] help: error if [-a|-g|-c] and [-i|-m|-w] are combined Ævar Arnfjörð Bjarmason
2022-02-23 22:16     ` Junio C Hamano
2022-02-21 19:38   ` [PATCH v2 8/9] help: add --no-[external-commands|aliases] for use with --all Ævar Arnfjörð Bjarmason
2022-02-23 22:19     ` Junio C Hamano
2022-02-21 19:38   ` [PATCH v2 9/9] help: don't print "\n" before single-section output Ævar Arnfjörð Bjarmason
2022-02-23 22:31     ` 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=220223.86h78pb5nd.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=levraiphilippeblain@gmail.com \
    --cc=pclouds@gmail.com \
    --cc=sunshine@sunshineco.com \
    --cc=szeder.dev@gmail.com \
    /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).