Re: [PATCH 1/2] FIX: use utf8_strnwidth for line_prefix in diff.c
"Md Isfarul Haque via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Md Isfarul Haque <isfarul.876@gmail.com>
>
> This patch adresses diff.c:2721 and proposes the fix using a new function.
Once the issue has fully been addressed, it is expected that the NEEDSWORK comment there would be removed, making this proposed log message useless. Make it a habit to write a log message that is self-contained enough to help readers (including yourself in the future when you have forgotten the details of what you did in this commit).
> +const struct strbuf *diff_line_prefix_buf(struct diff_options *opt)
> +{Given that there is only one caller of this function in the same file, I do not see a reason why this needs to be extern and exported in diff.h (actually I do not see a need for this helper at all).
When dealing with a string buffer, it is much more common in this codebase for the caller to prepare a strbuf (often on its stack) and pass a pointer to it to helper functions. I.e.
static void prepare_diff_line_prefix_buf(struct strbuf *buf,
struct diff_options *opt)
{
... stuff whatever you need into the string buffer ...
strbuf_add(buf, ...);
} /* in show_stats() */
struct strbuf line_prefix = STRBUF_INIT;
...
prepare_diff_line_prefix_buf(&line_prefix, options);
... use line_prefix and ...
... release the resource before returning ...
strbuf_release(&line_prefix);
is more common and less prone to resource leak over time.
Show 15 quoted lines
> @@ -2635,7 +2649,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> int width, name_width, graph_width, number_width = 0, bin_width = 0;
> const char *reset, *add_c, *del_c;
> int extra_shown = 0;
> - const char *line_prefix = diff_line_prefix(options);
> + const struct strbuf *line_prefix = diff_line_prefix_buf(options);
> struct strbuf out = STRBUF_INIT;
>
> if (data->nr == 0)
> @@ -2718,7 +2732,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> * used to correctly count the display width instead of strlen().
> */
> if (options->stat_width == -1)
> - width = term_columns() - strlen(line_prefix);
> + width = term_columns() - utf8_strnwidth(line_prefix->buf, line_prefix->len, 1);
I do not see the need for any of the diff_line_prefix_buf() related changes, only to do this change. You have a const char *line_prefix at this point, and utf8_strnwidth() wants to know its length, so what you need is to massage the parameter to match what it wants. Perhaps even something simple and dumb like
utf8_strnwidth(line_prefix, strlen(line_prefix), 1);
might be sufficient to replace strlen(line_prefix) in the original?
This patch hopefully will change the behaviour of the command. A patch needs to also protect the change from future breakages by adding a test or two to demonstrate the desired behaviour. Such a test should pass with the code change in the patch, and should fail when the code change in the patch gets reverted.
Thanks.
