| Message ID | ac9338533c9096c090d1463c1b29505bde019731.1706105064.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | FIX: use utf8_strnwidth for line_prefix in diff.c | expand |
On Wed, Jan 24, 2024 at 3:04 PM Md Isfarul Haque via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Md Isfarul Haque <isfarul.876@gmail.com> > > This patch adresses diff.c:2721 and proposes the fix using a new function. Please give more details here about what is currently at diff.c:2721 and what the patch is fixing, as lines at diff.c:2721 could move to a different location if some changes are made to diff.c before your patches get merged or after they get merged. Also if the patch is addressing an issue in a code comment I would expect the corresponding code comment to be removed by the patch. About the subject, maybe "diff: use utf8_strnwidth() for line_prefix" is already better. > Signed-off-by: Md Isfarul Haque <isfarul.876@gmail.com> > --- > diff.c | 18 ++++++++++++++++-- > diff.h | 1 + > 2 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/diff.c b/diff.c > index a89a6a6128a..e3223b8ce5b 100644 > --- a/diff.c > +++ b/diff.c > @@ -2300,6 +2300,20 @@ const char *diff_line_prefix(struct diff_options *opt) > return msgbuf->buf; > } > > +const struct strbuf *diff_line_prefix_buf(struct diff_options *opt) This function seems to be used only in diff.c, so it could be static.
"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. > @@ -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.
On 1/25/24 01:31, Christian Couder wrote: > On Wed, Jan 24, 2024 at 3:04 PM Md Isfarul Haque via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> >> From: Md Isfarul Haque <isfarul.876@gmail.com> >> >> This patch adresses diff.c:2721 and proposes the fix using a new function. > > Please give more details here about what is currently at diff.c:2721 > and what the patch is fixing, as lines at diff.c:2721 could move to a > different location if some changes are made to diff.c before your > patches get merged or after they get merged. > > Also if the patch is addressing an issue in a code comment I would > expect the corresponding code comment to be removed by the patch. I understand and apologize for the mess-up. I will keep it in mind next time. > About the subject, maybe "diff: use utf8_strnwidth() for line_prefix" > is already better. > >> Signed-off-by: Md Isfarul Haque <isfarul.876@gmail.com> >> --- >> diff.c | 18 ++++++++++++++++-- >> diff.h | 1 + >> 2 files changed, 17 insertions(+), 2 deletions(-) >> >> diff --git a/diff.c b/diff.c >> index a89a6a6128a..e3223b8ce5b 100644 >> --- a/diff.c >> +++ b/diff.c >> @@ -2300,6 +2300,20 @@ const char *diff_line_prefix(struct diff_options *opt) >> return msgbuf->buf; >> } >> >> +const struct strbuf *diff_line_prefix_buf(struct diff_options *opt) > > This function seems to be used only in diff.c, so it could be static. As Junio pointed out, I will probably use the existing function with slight modifications and use it. Besides, having unnecessary allocations and frees will probably only add overhead.
On 1/25/24 01:38, Junio C Hamano wrote: > "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). > I understand. Sorry for the mess-up. I will keep it in mind the next time. >> +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. > Ah, this is indeed very neat. Didn't strike me. I'm not extremely familiar with the codebase and was unaware of this practice. I will follow this pattern in the future. >> @@ -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? It was more of a force of habit on my end, since I usually do not use functions that do not have a limit on the length they are reading. However, considering that the string is generated by another function and is most likely safe as it was used earlier, I will implement this suggestion. > > 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. > Alright. Where should I add the test? A new/existing test in t/t4013 or t4124-log-graph-octopus.sh?
diff --git a/diff.c b/diff.c index a89a6a6128a..e3223b8ce5b 100644 --- a/diff.c +++ b/diff.c @@ -2300,6 +2300,20 @@ const char *diff_line_prefix(struct diff_options *opt) return msgbuf->buf; } +const struct strbuf *diff_line_prefix_buf(struct diff_options *opt) +{ + struct strbuf *msgbuf = (struct strbuf *)malloc(sizeof(*msgbuf)); + if (!opt->output_prefix){ + msgbuf->buf = ""; + msgbuf->len = 0; + msgbuf->alloc = 1; + } + else { + msgbuf = opt->output_prefix(opt, opt->output_prefix_data); + } + return msgbuf; +} + static unsigned long sane_truncate_line(char *line, unsigned long len) { const char *cp; @@ -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); else width = options->stat_width ? options->stat_width : 80; number_width = decimal_width(max_change) > number_width ? diff --git a/diff.h b/diff.h index 66bd8aeb293..6eb8dc9a97e 100644 --- a/diff.h +++ b/diff.h @@ -460,6 +460,7 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix); const char *diff_line_prefix(struct diff_options *); +const struct strbuf *diff_line_prefix_buf(struct diff_options *); extern const char mime_boundary_leader[];