threads / patch / 46280

patchsubmodule--helper: do not call utf8_fprintf() unnecessarily

Subject: [PATCH] submodule--helper: do not call utf8_fprintf() unnecessarily

## tl;dr

4 messages between Jun 28, 2017 and Jun 28, 2017. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Junio C Hamano· Jun 28, 2017, 20:38 UTC · lore

The helper function utf8_fprintf(fp, ...) has exactly the same effect to the output stream fp as fprintf(fp, ...) does, and the only difference is that its return value counts in display columns consumed (assuming that the payload is encoded in UTF-8), as opposed to number of bytes.

There is no reason to call it unless the caller cares about its return value.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * The helper was introduced at c0821965 ("Add utf8_fprintf helper
   that returns correct number of columns", 2013-02-09), which also
   taught the help text output from the parse_options API to use it
   to align columns.  These original callers naturally do use the
   returned value and left alone by this fix, which corrects all the
   later callers that misuses it.
 builtin/submodule--helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin/submodule--helper.c +2 −2
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 8517032b3e..50c6af1de7 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -326,7 +326,7 @@ static int module_list(int argc, const char **argv, const char *prefix)
 			printf("%06o %s %d\t", ce->ce_mode,
 			       oid_to_hex(&ce->oid), ce_stage(ce));
 
-		utf8_fprintf(stdout, "%s\n", ce->name);
+		fprintf(stdout, "%s\n", ce->name);
 	}
 	return 0;
 }
@@ -1038,7 +1038,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 		return 1;
 
 	for_each_string_list_item(item, &suc.projectlines)
-		utf8_fprintf(stdout, "%s", item->string);
+		fprintf(stdout, "%s", item->string);
 
 	return 0;
 }
Stefan Beller· Jun 28, 2017, 20:44 UTC · re: Junio C Hamano · lore

Re: [PATCH] submodule--helper: do not call utf8_fprintf() unnecessarily

On Wed, Jun 28, 2017 at 1:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> The helper function utf8_fprintf(fp, ...) has exactly the same
> effect to the output stream fp as fprintf(fp, ...) does, and the
> only difference is that its return value counts in display columns
> consumed (assuming that the payload is encoded in UTF-8), as opposed
> to number of bytes.
>
> There is no reason to call it unless the caller cares about its
> return value.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * The helper was introduced at c0821965 ("Add utf8_fprintf helper
>    that returns correct number of columns", 2013-02-09), which also
>    taught the help text output from the parse_options API to use it
>    to align columns.  These original callers naturally do use the
>    returned value and left alone by this fix, which corrects all the
>    later callers that misuses it.
>
The patch looks correct.

Thanks, Stefan

Show 25 quoted lines
>  builtin/submodule--helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 8517032b3e..50c6af1de7 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -326,7 +326,7 @@ static int module_list(int argc, const char **argv, const char *prefix)
>                         printf("%06o %s %d\t", ce->ce_mode,
>                                oid_to_hex(&ce->oid), ce_stage(ce));
>
> -               utf8_fprintf(stdout, "%s\n", ce->name);
> +               fprintf(stdout, "%s\n", ce->name);
>         }
>         return 0;
>  }
> @@ -1038,7 +1038,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)
>                 return 1;
>
>         for_each_string_list_item(item, &suc.projectlines)
> -               utf8_fprintf(stdout, "%s", item->string);
> +               fprintf(stdout, "%s", item->string);
>
>         return 0;
>  }
Junio C Hamano· Jun 28, 2017, 20:58 UTC · re: Stefan Beller · lore

Re: [PATCH] submodule--helper: do not call utf8_fprintf() unnecessarily

Stefan Beller <sbeller@google.com> writes:
Show 22 quoted lines
> On Wed, Jun 28, 2017 at 1:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> The helper function utf8_fprintf(fp, ...) has exactly the same
>> effect to the output stream fp as fprintf(fp, ...) does, and the
>> only difference is that its return value counts in display columns
>> consumed (assuming that the payload is encoded in UTF-8), as opposed
>> to number of bytes.
>>
>> There is no reason to call it unless the caller cares about its
>> return value.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>
>>  * The helper was introduced at c0821965 ("Add utf8_fprintf helper
>>    that returns correct number of columns", 2013-02-09), which also
>>    taught the help text output from the parse_options API to use it
>>    to align columns.  These original callers naturally do use the
>>    returned value and left alone by this fix, which corrects all the
>>    later callers that misuses it.
>>
>
> The patch looks correct.

Thanks. I had a small voice back in my head telling me that I may have misread the code and this patch breaks things, which you cleared up for me ;-)

Stefan Beller· Jun 28, 2017, 21:18 UTC · re: Junio C Hamano · lore

Re: [PATCH] submodule--helper: do not call utf8_fprintf() unnecessarily

On Wed, Jun 28, 2017 at 1:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 24 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> On Wed, Jun 28, 2017 at 1:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> The helper function utf8_fprintf(fp, ...) has exactly the same
>>> effect to the output stream fp as fprintf(fp, ...) does, and the
>>> only difference is that its return value counts in display columns
>>> consumed (assuming that the payload is encoded in UTF-8), as opposed
>>> to number of bytes.
>>>
>>> There is no reason to call it unless the caller cares about its
>>> return value.
>>>
>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>> ---
>>>
>>>  * The helper was introduced at c0821965 ("Add utf8_fprintf helper
>>>    that returns correct number of columns", 2013-02-09), which also
>>>    taught the help text output from the parse_options API to use it
>>>    to align columns.  These original callers naturally do use the
>>>    returned value and left alone by this fix, which corrects all the
>>>    later callers that misuses it.
>>>
>>
>> The patch looks correct.

I said this because I had a similar implementation a couple weeks back when Peff tried to poke (security) holes into submodule usage.

I tried finding the reason why it was originally introduced, but to no avail. It seems to be randomly introduced.

> Thanks.  I had a small voice back in my head telling me that I may
> have misread the code and this patch breaks things, which you
> cleared up for me ;-)

That said, #include "utf8.h" could also go from the file with this or after this patch, I believe.

← back to recent threads