Re: [PATCH v2] help.c: help.autocorrect=prompt waits for user action

"Azeem Bande-Ali via GitGitGadget" [off-list ref] writes:

Some people seem to make an asterisk stick to types like this, but
in our codebase written in C, an asterisk sticks to the identifier
that it makes into a pointer, i.e.

			char *answer;

This is because doing so differently would confuse novices with
constructs like this:

			int* a, b;

where only 'a' is a pointer, and 'b' is not.

+			struct strbuf msg = STRBUF_INIT;
+			strbuf_addf(&msg, _("Run '%s' instead? (y/N)"),
+				    assumed);

I think these should be kept on a single line for readability, i.e.

			strbuf_addf(&msg, _("Run '%s' instead? (y/N)"), assumed);

as I see a fairly long line after this block already.

	fprintf_ln(stderr, _("git: '%s' is not a git command. See 'git --help'."), cmd);


Other than these cosmetic bits, this round looks good to me,
including the documentation update.

Thanks.

On Sat, Aug 14, 2021 at 2:20 PM Junio C Hamano [off-list ref] wrote:

"Azeem Bande-Ali via GitGitGadget" [off-list ref] writes:

quoted

+                     char* answer;

Some people seem to make an asterisk stick to types like this, but
in our codebase written in C, an asterisk sticks to the identifier
that it makes into a pointer, i.e.

                        char *answer;

Thanks! Will fix. I appreciate you providing the motivation for the
policy as well!

quoted

+                     struct strbuf msg = STRBUF_INIT;
+                     strbuf_addf(&msg, _("Run '%s' instead? (y/N)"),
+                                 assumed);

I think these should be kept on a single line for readability, i.e.

                        strbuf_addf(&msg, _("Run '%s' instead? (y/N)"), assumed);

as I see a fairly long line after this block already.

        fprintf_ln(stderr, _("git: '%s' is not a git command. See 'git --help'."), cmd);

The line split was done by clang-format via `make style`. Is it okay
to ignore the clang-format recommendation?
(Incidentally `make style` would have also caught the first issue, I
seem to have not run it after
making a change in that line.)

Other than these cosmetic bits, this round looks good to me,
including the documentation update.

Great thanks! This is my first patch so I just wanted to confirm:
should I add a "Reviewed-by:"
line to the commit with your name as well?

Hi,

On Sat, 14 Aug 2021, Junio C Hamano wrote:

"Azeem Bande-Ali via GitGitGadget" [off-list ref] writes:

quoted

@@ -618,7 +628,17 @@ const char *help_unknown_cmd(const char *cmd)
 				   _("Continuing under the assumption that "
 				     "you meant '%s'."),
 				   assumed);
-		else {
+		else if (autocorrect == AUTOCORRECT_PROMPT) {
+			char* answer;

Some people seem to make an asterisk stick to types like this, but
in our codebase written in C, an asterisk sticks to the identifier
that it makes into a pointer, i.e.

			char *answer;

This is because doing so differently would confuse novices with
constructs like this:

			int* a, b;

where only 'a' is a pointer, and 'b' is not.

quoted

+			struct strbuf msg = STRBUF_INIT;
+			strbuf_addf(&msg, _("Run '%s' instead? (y/N)"),
+				    assumed);

I think these should be kept on a single line for readability, i.e.

			strbuf_addf(&msg, _("Run '%s' instead? (y/N)"), assumed);

It might even make sense to use the `xstrfmt()` function instead:

			char *msg = xstrfmt(_("Run '%s' instead? (y/N)"), assumed);
			[...]
			free(msg);

Ciao,
Dscho