| Message ID | patch-v2-2.9-124643c4b35-20220221T193708Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cd87ce7d0def6b6efeb1ae688bde217a0a47c9eb |
| Headers | show |
| Series | help: tests, parse_options() sanity, fix "help -g" regression | expand |
Æ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. > 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.
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.
diff --git a/help.c b/help.c index 71444906ddf..77af953826e 100644 --- a/help.c +++ b/help.c @@ -124,7 +124,8 @@ static void print_cmd_by_category(const struct category_description *catdesc, uint32_t mask = catdesc[i].category; const char *desc = catdesc[i].desc; - printf("\n%s\n", _(desc)); + putchar('\n'); + puts(_(desc)); 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")); putchar('\n'); pretty_print_cmdnames(other_cmds, colopts); putchar('\n'); @@ -439,7 +440,7 @@ void list_all_cmds_help(void) struct cmdname_help *aliases; int i, longest; - printf_ln(_("See 'git help <command>' to read about a specific subcommand")); + puts(_("See 'git help <command>' to read about a specific subcommand")); print_cmd_by_category(main_categories, &longest); list_all_other_cmds(&others);
Change code in "help.c" that used printf_ln() without format specifiers to use puts() instead, as other existing code in the file does. Let's also change related code to use puts() instead of the equivalent of calling "printf" with a "%s\n" format. This formatting-only change will make a subsequent functional change easier to read, as it'll be changing code that's consistently using the same functions to do the same things. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- help.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)