Re: [PATCH v2 2/9] help.c: use puts() instead of printf{,_ln}() for consistency

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 23, 2022, 21:57 UTC
Message-ID: <220223.86h78pb5nd.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqfso9s14j.fsf@gitster.g>

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

Show 10 quoted lines

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

Show 19 quoted lines

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