FIX: use utf8_strnwidth for line_prefix in diff.c

This patch adresses diff.c:2721 and proposes the fix using a new function.

Md Isfarul Haque (2):
FIX: use utf8_strnwidth for line_prefix in diff.c FIX memory leak in one branch

From: Md Isfarul Haque <[email protected]>

This patch adresses diff.c:2721 and proposes the fix using a new function.

Signed-off-by Md Isfarul Haque <[email protected]>

---
 diff.c | 18 ++++++++++++++++--
 diff.h |  1 +
 2 files changed, 17 insertions(+), 2 deletions(-)

diff.c +0 -0

index a89a6a6128a..e3223b8ce5b 100644

From: Md Isfarul Haque <[email protected]>

This patch adresses diff.c:2721 and proposes the fix using a new function.

diff.c +1 -1

@@ -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)

From: Md Isfarul Haque <[email protected]>

Signed-off-by Md Isfarul Haque <[email protected]>

---
 diff.c | 1 +
 1 file changed, 1 insertion(+)

Quoted 4 lines · Md Isfarul Haque via GitGitGadget

From: Md Isfarul Haque <[email protected]>

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.

Quoted 14 lines

Signed-off-by Md Isfarul Haque <[email protected]>

---
 diff.c | 18 ++++++++++++++++--
 diff.h |  1 +
 2 files changed, 17 insertions(+), 2 deletions(-)

diff.c +0 -0

index a89a6a6128a..e3223b8ce5b 100644
@@ -2300,6 +2300,20 @@ const char *diff_line_prefix(struct diff_options *opt)
        return msgbuf->buf;
 }
 

diff.c const char *diff_line_prefix(struct diff_options *opt) · line 2303

Quoted 1 line

diff.c +1 -0

+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.

Quoted 2 lines · Md Isfarul Haque via GitGitGadget

From: Md Isfarul Haque <[email protected]>

Quoted 1 line

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).

diff.c const char *diff_line_prefix(struct diff_options *opt) · line 2304

Quoted 2 lines

diff.c +2 -0

+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.

Quoted 10 lines

diff.c +1 -1

@@ -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)

diff.c static void show_stats(struct diffstat_t *data, struct diff_options *options) · line 2735

Quoted 5 lines

diff.c +1 -1

@@ -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.