{"thread":{"id":"60788","subject":"[PATCH 0/2] [GSoC][RFC/Patch] FIX: use utf8_strnwidth for line_prefix in diff.c","startedAt":"2024-01-24T14:04:27Z","lastAt":"2024-01-25T05:52:50Z","messageCount":9,"participants":["Md Isfarul Haque via GitGitGadget","Junio C Hamano","Christian Couder","Md Isfarul Haque"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"487328","messageId":"pull.1653.git.git.1706105064.gitgitgadget@gmail.com","threadId":"60788","inReplyTo":null,"subject":"[PATCH 0/2] [GSoC][RFC/Patch] FIX: use utf8_strnwidth for line_prefix in diff.c","fromName":"Md Isfarul Haque via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-24T14:04:22Z","receivedAt":"2024-01-24T14:04:27Z","isPatch":true,"sender":{"key":"isfarul.876@gmail.com","avatar":null},"body":"This patch adresses diff.c:2721 and proposes the fix using a new function.\n\nMd Isfarul Haque (2):\n  FIX: use utf8_strnwidth for line_prefix in diff.c\n  FIX memory leak in one branch\n\n diff.c | 19 +++++++++++++++++--\n diff.h |  1 +\n 2 files changed, 18 insertions(+), 2 deletions(-)\n\n\nbase-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1653%2FInnocentZero%2Fdiff_needswork-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1653/InnocentZero/diff_needswork-v1\nPull-Request: https://github.com/git/git/pull/1653\n-- \ngitgitgadget\n"},{"id":"487329","messageId":"ac9338533c9096c090d1463c1b29505bde019731.1706105064.git.gitgitgadget@gmail.com","threadId":"60788","inReplyTo":"pull.1653.git.git.1706105064.gitgitgadget@gmail.com","subject":"[PATCH 1/2] FIX: use utf8_strnwidth for line_prefix in diff.c","fromName":"Md Isfarul Haque via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-24T14:04:23Z","receivedAt":"2024-01-24T14:04:27Z","isPatch":true,"sender":{"key":"isfarul.876@gmail.com","avatar":null},"body":"From: Md Isfarul Haque <isfarul.876@gmail.com>\n\nThis patch adresses diff.c:2721 and proposes the fix using a new function.\n\nSigned-off-by: Md Isfarul Haque <isfarul.876@gmail.com>\n---\n diff.c | 18 ++++++++++++++++--\n diff.h |  1 +\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex a89a6a6128a..e3223b8ce5b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2300,6 +2300,20 @@ const char *diff_line_prefix(struct diff_options *opt)\n \treturn msgbuf->buf;\n }\n \n+const struct strbuf *diff_line_prefix_buf(struct diff_options *opt)\n+{\n+\tstruct strbuf *msgbuf = (struct strbuf *)malloc(sizeof(*msgbuf));\n+\tif (!opt->output_prefix){\n+\t\tmsgbuf->buf = \"\";\n+\t\tmsgbuf->len = 0;\n+\t\tmsgbuf->alloc = 1;\n+\t}\n+\telse {\n+\t\tmsgbuf = opt->output_prefix(opt, opt->output_prefix_data);\n+\t}\n+\treturn msgbuf;\n+}\n+\n static unsigned long sane_truncate_line(char *line, unsigned long len)\n {\n \tconst char *cp;\n@@ -2635,7 +2649,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \tint width, name_width, graph_width, number_width = 0, bin_width = 0;\n \tconst char *reset, *add_c, *del_c;\n \tint extra_shown = 0;\n-\tconst char *line_prefix = diff_line_prefix(options);\n+\tconst struct strbuf *line_prefix = diff_line_prefix_buf(options);\n \tstruct strbuf out = STRBUF_INIT;\n \n \tif (data->nr == 0)\n@@ -2718,7 +2732,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t * used to correctly count the display width instead of strlen().\n \t */\n \tif (options->stat_width == -1)\n-\t\twidth = term_columns() - strlen(line_prefix);\n+\t\twidth = term_columns() - utf8_strnwidth(line_prefix->buf, line_prefix->len, 1);\n \telse\n \t\twidth = options->stat_width ? options->stat_width : 80;\n \tnumber_width = decimal_width(max_change) > number_width ?\ndiff --git a/diff.h b/diff.h\nindex 66bd8aeb293..6eb8dc9a97e 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -460,6 +460,7 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix);\n \n const char *diff_line_prefix(struct diff_options *);\n \n+const struct strbuf *diff_line_prefix_buf(struct diff_options *);\n \n extern const char mime_boundary_leader[];\n \n-- \ngitgitgadget\n\n"},{"id":"487330","messageId":"a7c81f7e114fc8854436e2ca1fccb4c968653317.1706105064.git.gitgitgadget@gmail.com","threadId":"60788","inReplyTo":"pull.1653.git.git.1706105064.gitgitgadget@gmail.com","subject":"[PATCH 2/2] FIX memory leak in one branch","fromName":"Md Isfarul Haque via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-24T14:04:24Z","receivedAt":"2024-01-24T14:04:28Z","isPatch":true,"sender":{"key":"isfarul.876@gmail.com","avatar":null},"body":"From: Md Isfarul Haque <isfarul.876@gmail.com>\n\nSigned-off-by: Md Isfarul Haque <isfarul.876@gmail.com>\n---\n diff.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/diff.c b/diff.c\nindex e3223b8ce5b..9fa00103a6b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2309,6 +2309,7 @@ const struct strbuf *diff_line_prefix_buf(struct diff_options *opt)\n \t\tmsgbuf->alloc = 1;\n \t}\n \telse {\n+\t\tfree(msgbuf);\n \t\tmsgbuf = opt->output_prefix(opt, opt->output_prefix_data);\n \t}\n \treturn msgbuf;\n-- \ngitgitgadget\n"},{"id":"487341","messageId":"xmqqjznyfx0l.fsf@gitster.g","threadId":"60788","inReplyTo":"pull.1653.git.git.1706105064.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] [GSoC][RFC/Patch] FIX: use utf8_strnwidth for line_prefix in diff.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-24T17:42:50Z","receivedAt":"2024-01-24T17:42:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Md Isfarul Haque via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> This patch adresses diff.c:2721 and proposes the fix using a new function.\n\nYay.  My favorite long-time pet-peeve topic.\n\n>\n> Md Isfarul Haque (2):\n>   FIX: use utf8_strnwidth for line_prefix in diff.c\n>   FIX memory leak in one branch\n\nOur convention does not use \"FIX\" and other prefix on the subject.\nPlease check Documentation/SubmittingPatches.\n\nThanks.\n"},{"id":"487347","messageId":"CAP8UFD2oxpefhGtH61wZvcHEvd24jtu_ZBVsnNZMjD2aM_NDcg@mail.gmail.com","threadId":"60788","inReplyTo":"ac9338533c9096c090d1463c1b29505bde019731.1706105064.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] FIX: use utf8_strnwidth for line_prefix in diff.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-01-24T20:01:20Z","receivedAt":"2024-01-24T20:01:35Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jan 24, 2024 at 3:04 PM Md Isfarul Haque via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Md Isfarul Haque <isfarul.876@gmail.com>\n>\n> This patch adresses diff.c:2721 and proposes the fix using a new function.\n\nPlease give more details here about what is currently at diff.c:2721\nand what the patch is fixing, as lines at diff.c:2721 could move to a\ndifferent location if some changes are made to diff.c before your\npatches get merged or after they get merged.\n\nAlso if the patch is addressing an issue in a code comment I would\nexpect the corresponding code comment to be removed by the patch.\n\nAbout the subject, maybe \"diff: use utf8_strnwidth() for line_prefix\"\nis already better.\n\n> Signed-off-by: Md Isfarul Haque <isfarul.876@gmail.com>\n> ---\n>  diff.c | 18 ++++++++++++++++--\n>  diff.h |  1 +\n>  2 files changed, 17 insertions(+), 2 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index a89a6a6128a..e3223b8ce5b 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2300,6 +2300,20 @@ const char *diff_line_prefix(struct diff_options *opt)\n>         return msgbuf->buf;\n>  }\n>\n> +const struct strbuf *diff_line_prefix_buf(struct diff_options *opt)\n\nThis function seems to be used only in diff.c, so it could be static.\n"},{"id":"487348","messageId":"xmqqplxqcx5p.fsf@gitster.g","threadId":"60788","inReplyTo":"ac9338533c9096c090d1463c1b29505bde019731.1706105064.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] FIX: use utf8_strnwidth for line_prefix in diff.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-24T20:08:02Z","receivedAt":"2024-01-24T20:08:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Md Isfarul Haque via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Md Isfarul Haque <isfarul.876@gmail.com>\n>\n> This patch adresses diff.c:2721 and proposes the fix using a new function.\n\nOnce the issue has fully been addressed, it is expected that the\nNEEDSWORK comment there would be removed, making this proposed log\nmessage useless.  Make it a habit to write a log message that is\nself-contained enough to help readers (including yourself in the\nfuture when you have forgotten the details of what you did in this\ncommit).\n\n> +const struct strbuf *diff_line_prefix_buf(struct diff_options *opt)\n> +{\n\nGiven that there is only one caller of this function in the same\nfile, I do not see a reason why this needs to be extern and exported\nin diff.h (actually I do not see a need for this helper at all).\n\nWhen dealing with a string buffer, it is much more common in this\ncodebase for the caller to prepare a strbuf (often on its stack) and\npass a pointer to it to helper functions.  I.e.\n\n\tstatic void prepare_diff_line_prefix_buf(struct strbuf *buf,\n\t\t\t\t\t\tstruct diff_options *opt)\n\t{\n\t\t... stuff whatever you need into the string buffer ...\n                strbuf_add(buf, ...);\n\t}\n\n\t/* in show_stats() */\n\tstruct strbuf line_prefix = STRBUF_INIT;\n\t...\n\tprepare_diff_line_prefix_buf(&line_prefix, options);\n\t... use line_prefix and ...\n\t... release the resource before returning ...\n\tstrbuf_release(&line_prefix);\n\t\nis more common and less prone to resource leak over time.\n\n> @@ -2635,7 +2649,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>  \tint width, name_width, graph_width, number_width = 0, bin_width = 0;\n>  \tconst char *reset, *add_c, *del_c;\n>  \tint extra_shown = 0;\n> -\tconst char *line_prefix = diff_line_prefix(options);\n> +\tconst struct strbuf *line_prefix = diff_line_prefix_buf(options);\n>  \tstruct strbuf out = STRBUF_INIT;\n>  \n>  \tif (data->nr == 0)\n> @@ -2718,7 +2732,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>  \t * used to correctly count the display width instead of strlen().\n>  \t */\n>  \tif (options->stat_width == -1)\n> -\t\twidth = term_columns() - strlen(line_prefix);\n> +\t\twidth = term_columns() - utf8_strnwidth(line_prefix->buf, line_prefix->len, 1);\n\nI do not see the need for any of the diff_line_prefix_buf() related\nchanges, only to do this change.  You have a const char *line_prefix\nat this point, and utf8_strnwidth() wants to know its length, so\nwhat you need is to massage the parameter to match what it wants.\nPerhaps even something simple and dumb like\n\n\tutf8_strnwidth(line_prefix, strlen(line_prefix), 1);\n\nmight be sufficient to replace strlen(line_prefix) in the original?\n\nThis patch hopefully will change the behaviour of the command.  A\npatch needs to also protect the change from future breakages by\nadding a test or two to demonstrate the desired behaviour.  Such a\ntest should pass with the code change in the patch, and should fail\nwhen the code change in the patch gets reverted.\n\nThanks.\n"},{"id":"487350","messageId":"xmqqil3icx0g.fsf@gitster.g","threadId":"60788","inReplyTo":"a7c81f7e114fc8854436e2ca1fccb4c968653317.1706105064.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] FIX memory leak in one branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-24T20:11:11Z","receivedAt":"2024-01-24T20:11:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Md Isfarul Haque via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Md Isfarul Haque <isfarul.876@gmail.com>\n>\n> Signed-off-by: Md Isfarul Haque <isfarul.876@gmail.com>\n> ---\n>  diff.c | 1 +\n>  1 file changed, 1 insertion(+)\n\nWe do not need to see that you are just as human as other developers\nand are prone to make mistakes that you need to fix it in a\nfollow-up.  In other words, please do not introduce a bug in [1/2]\nonly to be fixed in [2/2].  By squashing these two patches into one,\nyou can pretend as if you are a perfect developer and never leaked\nmemory ;-).\n\n> diff --git a/diff.c b/diff.c\n> index e3223b8ce5b..9fa00103a6b 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2309,6 +2309,7 @@ const struct strbuf *diff_line_prefix_buf(struct diff_options *opt)\n>  \t\tmsgbuf->alloc = 1;\n>  \t}\n>  \telse {\n> +\t\tfree(msgbuf);\n>  \t\tmsgbuf = opt->output_prefix(opt, opt->output_prefix_data);\n>  \t}\n>  \treturn msgbuf;\n\nBut as I said, I do not see a need for this helper function in the\nfirst place, so...\n"},{"id":"487358","messageId":"d135a291-7b51-4cd6-86ec-676c1c5b04e9@gmail.com","threadId":"60788","inReplyTo":"CAP8UFD2oxpefhGtH61wZvcHEvd24jtu_ZBVsnNZMjD2aM_NDcg@mail.gmail.com","subject":"Re: [PATCH 1/2] FIX: use utf8_strnwidth for line_prefix in diff.c","fromName":"Md Isfarul Haque","fromEmail":"isfarul.876@gmail.com","sentAt":"2024-01-25T05:42:02Z","receivedAt":"2024-01-25T05:42:08Z","isPatch":true,"sender":{"key":"isfarul.876@gmail.com","avatar":null},"body":"On 1/25/24 01:31, Christian Couder wrote:\n> On Wed, Jan 24, 2024 at 3:04 PM Md Isfarul Haque via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>>\n>> From: Md Isfarul Haque <isfarul.876@gmail.com>\n>>\n>> This patch adresses diff.c:2721 and proposes the fix using a new function.\n> \n> Please give more details here about what is currently at diff.c:2721\n> and what the patch is fixing, as lines at diff.c:2721 could move to a\n> different location if some changes are made to diff.c before your\n> patches get merged or after they get merged.\n> \n> Also if the patch is addressing an issue in a code comment I would\n> expect the corresponding code comment to be removed by the patch.\n\nI understand and apologize for the mess-up. I will keep it in mind next time. \n\n> About the subject, maybe \"diff: use utf8_strnwidth() for line_prefix\"\n> is already better.\n> \n>> Signed-off-by: Md Isfarul Haque <isfarul.876@gmail.com>\n>> ---\n>>  diff.c | 18 ++++++++++++++++--\n>>  diff.h |  1 +\n>>  2 files changed, 17 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/diff.c b/diff.c\n>> index a89a6a6128a..e3223b8ce5b 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -2300,6 +2300,20 @@ const char *diff_line_prefix(struct diff_options *opt)\n>>         return msgbuf->buf;\n>>  }\n>>\n>> +const struct strbuf *diff_line_prefix_buf(struct diff_options *opt)\n> \n> This function seems to be used only in diff.c, so it could be static.\n\nAs Junio pointed out, I will probably use the existing function with slight\nmodifications and use it. Besides, having unnecessary allocations and frees\nwill probably only add overhead.\n\n-- \n\nThanks and regards\nMd. Isfarul Haque\n\n\n"},{"id":"487359","messageId":"d1569248-ce56-4e19-9244-c60c7617ca28@gmail.com","threadId":"60788","inReplyTo":"xmqqplxqcx5p.fsf@gitster.g","subject":"Re: [PATCH 1/2] FIX: use utf8_strnwidth for line_prefix in diff.c","fromName":"Md Isfarul Haque","fromEmail":"isfarul.876@gmail.com","sentAt":"2024-01-25T05:52:46Z","receivedAt":"2024-01-25T05:52:50Z","isPatch":true,"sender":{"key":"isfarul.876@gmail.com","avatar":null},"body":"On 1/25/24 01:38, Junio C Hamano wrote:\n> \"Md Isfarul Haque via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Md Isfarul Haque <isfarul.876@gmail.com>\n>>\n>> This patch adresses diff.c:2721 and proposes the fix using a new function.\n> \n> Once the issue has fully been addressed, it is expected that the\n> NEEDSWORK comment there would be removed, making this proposed log\n> message useless.  Make it a habit to write a log message that is\n> self-contained enough to help readers (including yourself in the\n> future when you have forgotten the details of what you did in this\n> commit).\n> \n\nI understand. Sorry for the mess-up. I will keep it in mind the next time.\n\n>> +const struct strbuf *diff_line_prefix_buf(struct diff_options *opt)\n>> +{\n> \n> Given that there is only one caller of this function in the same\n> file, I do not see a reason why this needs to be extern and exported\n> in diff.h (actually I do not see a need for this helper at all).\n> \n> When dealing with a string buffer, it is much more common in this\n> codebase for the caller to prepare a strbuf (often on its stack) and\n> pass a pointer to it to helper functions.  I.e.\n> \n> \tstatic void prepare_diff_line_prefix_buf(struct strbuf *buf,\n> \t\t\t\t\t\tstruct diff_options *opt)\n> \t{\n> \t\t... stuff whatever you need into the string buffer ...\n>                 strbuf_add(buf, ...);\n> \t}\n> \n> \t/* in show_stats() */\n> \tstruct strbuf line_prefix = STRBUF_INIT;\n> \t...\n> \tprepare_diff_line_prefix_buf(&line_prefix, options);\n> \t... use line_prefix and ...\n> \t... release the resource before returning ...\n> \tstrbuf_release(&line_prefix);\n> \t\n> is more common and less prone to resource leak over time.\n> \n\nAh, this is indeed very neat. Didn't strike me. I'm not extremely familiar\nwith the codebase and was unaware of this practice. I will follow this \npattern in the future.\n\n>> @@ -2635,7 +2649,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>>  \tint width, name_width, graph_width, number_width = 0, bin_width = 0;\n>>  \tconst char *reset, *add_c, *del_c;\n>>  \tint extra_shown = 0;\n>> -\tconst char *line_prefix = diff_line_prefix(options);\n>> +\tconst struct strbuf *line_prefix = diff_line_prefix_buf(options);\n>>  \tstruct strbuf out = STRBUF_INIT;\n>>  \n>>  \tif (data->nr == 0)\n>> @@ -2718,7 +2732,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>>  \t * used to correctly count the display width instead of strlen().\n>>  \t */\n>>  \tif (options->stat_width == -1)\n>> -\t\twidth = term_columns() - strlen(line_prefix);\n>> +\t\twidth = term_columns() - utf8_strnwidth(line_prefix->buf, line_prefix->len, 1);\n> \n> I do not see the need for any of the diff_line_prefix_buf() related\n> changes, only to do this change.  You have a const char *line_prefix\n> at this point, and utf8_strnwidth() wants to know its length, so\n> what you need is to massage the parameter to match what it wants.\n> Perhaps even something simple and dumb like\n> \n> \tutf8_strnwidth(line_prefix, strlen(line_prefix), 1);\n> \n> might be sufficient to replace strlen(line_prefix) in the original?\n\nIt was more of a force of habit on my end, since I usually do not use\nfunctions that do not have a limit on the length they are reading.\nHowever, considering that the string is generated by another function\nand is most likely safe as it was used earlier, I will implement \nthis suggestion.\n\n> \n> This patch hopefully will change the behaviour of the command.  A\n> patch needs to also protect the change from future breakages by\n> adding a test or two to demonstrate the desired behaviour.  Such a\n> test should pass with the code change in the patch, and should fail\n> when the code change in the patch gets reverted.\n> \n\nAlright. Where should I add the test? A new/existing test in t/t4013 \nor t4124-log-graph-octopus.sh?\n\n-- \n\nThanks and regards,\nMd Isfarul Haque\n\n"}]}