On 1/25/24 01:38, Junio C Hamano wrote:
> "Md Isfarul Haque via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> From: Md Isfarul Haque <isfarul.876@xxxxxxxxx>
>>
>> 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?
--
Thanks and regards,
Md Isfarul Haque
