From b838418338f1973bfa9ea51f4da962a159f9f00a Mon Sep 17 00:00:00 2001
From: Turiiya <34311583+ttytm@users.noreply.github.com>
Date: Sat, 13 Apr 2024 08:00:12 +0200
Subject: [PATCH] v.util: fix diff coloring, add test (#21260)

---
 vlib/v/util/diff/diff.v      | 20 ++++++++--------
 vlib/v/util/diff/diff_test.v | 44 +++++++++++++++++++++++++++---------
 2 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/vlib/v/util/diff/diff.v b/vlib/v/util/diff/diff.v
index 990deaa34ab8d9..5c999df24fb606 100644
--- a/vlib/v/util/diff/diff.v
+++ b/vlib/v/util/diff/diff.v
@@ -31,14 +31,6 @@ pub fn find_working_diff_command() !string {
 			continue
 		}
 		if p.exit_code == 0 { // success
-			// TODO: proper implemenation of --color flag
-			$if !macos {
-				if diffcmd in ['gdiff', 'diff'] {
-					if p.output.contains('GNU diffutils') && env_diffopts == '' {
-						return '${diffcmd} --color=always'
-					}
-				}
-			}
 			if diffcmd in ['code', 'code.cmd'] {
 				// Make sure the diff flag `-d` is included in any case.
 				return '${diffcmd} ${env_diffopts} -d'
@@ -51,9 +43,8 @@ pub fn find_working_diff_command() !string {
 }
 
 pub fn color_compare_files(diff_cmd string, path1 string, path2 string) string {
-	os.find_abs_path_of_executable(diff_cmd.all_before(' ')) or {
-		return 'comparison command: `${diff_cmd}` not found'
-	}
+	cmd := diff_cmd.all_before(' ')
+	os.find_abs_path_of_executable(cmd) or { return 'comparison command: `${cmd}` not found' }
 	flags := $if openbsd {
 		['-d', '-a', '-U', '2']
 	} $else $if freebsd {
@@ -61,6 +52,13 @@ pub fn color_compare_files(diff_cmd string, path1 string, path2 string) string {
 	} $else {
 		['--minimal', '--text', '--unified=2', '--show-function-line="fn "']
 	}
+	if cmd == 'diff' {
+		color_diff_cmd := '${diff_cmd} --color=always ${flags.join(' ')} ${os.quoted_path(path1)} ${os.quoted_path(path2)}'
+		color_result := os.execute(color_diff_cmd)
+		if !color_result.output.starts_with('diff: unrecognized option') {
+			return color_result.output.trim_right('\r\n')
+		}
+	}
 	full_cmd := '${diff_cmd} ${flags.join(' ')} ${os.quoted_path(path1)} ${os.quoted_path(path2)}'
 	return os.execute(full_cmd).output.trim_right('\r\n')
 }
diff --git a/vlib/v/util/diff/diff_test.v b/vlib/v/util/diff/diff_test.v
index 4f70020913b80d..e0a10781506d50 100644
--- a/vlib/v/util/diff/diff_test.v
+++ b/vlib/v/util/diff/diff_test.v
@@ -1,12 +1,21 @@
 import v.util.diff
 import os
 
-fn test_compare_files() {
+const tdir = os.join_path(os.vtmp_dir(), 'diff_test')
+
+fn testsuite_begin() {
 	os.find_abs_path_of_executable('diff') or {
-		eprintln('> skipping test, since this test requires `diff` to be installed.')
-		return
+		eprintln('> skipping test, since this test requires `diff` to be installed')
+		exit(0)
 	}
+	os.mkdir_all(tdir)!
+}
+
+fn testsuite_end() {
+	os.rmdir_all(tdir) or {}
+}
 
+fn test_compare_files() {
 	f1 := "Module{
 	name: 'Foo'
 	description: 'Awesome V module.'
@@ -22,13 +31,8 @@ fn test_compare_files() {
 	dependencies: []
 }
 "
-	tdir := os.join_path(os.vtmp_dir(), 'diff_test')
-	os.mkdir_all(tdir)!
-	defer {
-		os.rmdir_all(tdir) or {}
-	}
-	p1 := os.join_path(tdir, 'f1.txt')
-	p2 := os.join_path(tdir, 'f2.txt')
+	p1 := os.join_path(tdir, '${@FN}_f1.txt')
+	p2 := os.join_path(tdir, '${@FN}_f2.txt')
 	os.write_file(p1, f1)!
 	os.write_file(p2, f2)!
 
@@ -47,16 +51,34 @@ fn test_compare_files() {
 	assert res.contains("+\tlicense: 'MIT'"), res
 
 	// Test again using `find_working_diff_command()`.
+	os.setenv('VDIFF_TOOL', 'diff', true)
 	res = diff.color_compare_files(diff.find_working_diff_command()!, p1, p2)
 	assert res.contains("-\tversion: '0.0.0'"), res
 	assert res.contains("+\tversion: '0.1.0'"), res
 	assert res.contains("+\tlicense: 'MIT'"), res
 
 	// Test adding a flag via env flag.
-	os.setenv('VDIFF_OPTIONS', '--ignore-case', true)
+	os.setenv('VDIFF_OPTIONS', '--ignore-case', true) // ignored, when VDIFF_TOOL is not explicitly set
 	res = diff.color_compare_files(diff.find_working_diff_command()!, p1, p2)
 	assert !res.contains("+\tname: 'foo'"), res
 	assert res.contains("-\tversion: '0.0.0'"), res
 	assert res.contains("+\tversion: '0.1.0'"), res
 	assert res.contains("+\tlicense: 'MIT'"), res
 }
+
+fn test_coloring() {
+	if os.execute('diff --color=always').output.starts_with('diff: unrecognized option') {
+		eprintln('> skipping test, since `diff` does not support --color=always')
+		return
+	}
+	f1 := 'abc\n'
+	f2 := 'abcd\n'
+	p1 := os.join_path(tdir, '${@FN}_f1.txt')
+	p2 := os.join_path(tdir, '${@FN}_f2.txt')
+	os.write_file(p1, f1)!
+	os.write_file(p2, f2)!
+	res := diff.color_compare_files('diff', p1, p2)
+	esc := rune(27)
+	assert res.contains('${esc}[31m-abc${esc}['), res
+	assert res.contains('${esc}[32m+abcd${esc}['), res
+}