diff --git a/Makefile.am b/Makefile.am index 6e7cb83e..f0905b53 100644 --- a/Makefile.am +++ b/Makefile.am @@ -307,7 +307,6 @@ TESTS = tests/newline1/run-test \ XFAIL_TESTS = \ tests/delhunk5/run-test \ tests/delhunk6/run-test \ - tests/interdiff-color-context/run-test \ tests/rediff-empty-hunk/run-test test-perms: src/combinediff$(EXEEXT) src/flipdiff$(EXEEXT) \ diff --git a/NEWS b/NEWS index 3678644e..ad05c3f3 100644 --- a/NEWS +++ b/NEWS @@ -14,6 +14,10 @@ Patchutils news compatibility (will change to 'strip' in version 0.5.0). Addresses GitHub issues #22, #27, #59, and #68. + Fixed interdiff --color option handling and improved color output. + Replaced external diff --color dependency with internal ANSI color + codes. Addresses GitHub issue #119. + Code improvements and build system enhancements. Added redirectfd() utility function to redirect stdout without reassignment. Enhanced CI testing with musl support for better compatibility testing. diff --git a/src/interdiff.c b/src/interdiff.c index 54d6c0f4..f3f474b0 100644 --- a/src/interdiff.c +++ b/src/interdiff.c @@ -33,6 +33,7 @@ #endif /* HAVE_ERROR_H */ #include #include +#include #include #include #include @@ -58,6 +59,26 @@ #define PATCH "patch" #endif +/* Line type for coloring */ +enum line_type { + LINE_FILE, + LINE_HEADER, + LINE_HUNK, + LINE_ADDED, + LINE_REMOVED, + LINE_TYPE_MAX +}; + +/* ANSI color codes for diff output */ +static const char *const color_codes[LINE_TYPE_MAX] = { + + [LINE_FILE] = "\033[1m", /* Bold for filenames */ + [LINE_HEADER] = "\033[1m", /* Bold for headers */ + [LINE_HUNK] = "\033[36m", /* Cyan for hunk headers */ + [LINE_ADDED] = "\033[32m", /* Green for added lines */ + [LINE_REMOVED] = "\033[31m" /* Red for removed lines */ +}; + /* This can be invoked as interdiff, combinediff, or flipdiff. */ static enum { mode_inter, @@ -98,6 +119,8 @@ static int ignore_components = 0; static int ignore_components_specified = 0; static int unzip = 0; static int no_revert_omitted = 0; +static int use_colors = 0; +static int color_option_specified = 0; static int debug = 0; static struct patlist *pat_drop_context = NULL; @@ -106,15 +129,42 @@ static struct file_list *files_done = NULL; static struct file_list *files_in_patch2 = NULL; static struct file_list *files_in_patch1 = NULL; -/* checks whether file needs processing and sets context */ +/* + * Print colored output using a variadic format string. + */ + static void + print_color (FILE *output_file, enum line_type type, const char *format, ...) + { + const char *color_start = NULL; + va_list args; + + /* Only colorize if colors are enabled AND we're outputting to stdout */ + if (use_colors && output_file == stdout) + color_start = color_codes[type]; + + /* Print color start code */ + if (color_start) + fputs (color_start, output_file); + + /* Print the formatted content */ + va_start (args, format); + vfprintf (output_file, format, args); + va_end (args); + + /* Print color end code */ + if (color_start) + fputs ("\033[0m", output_file); + } + + /* checks whether file needs processing and sets context */ static int check_filename (const char *fn) { - if (patlist_match(pat_drop_context, fn)) { + if (patlist_match(pat_drop_context, fn)) max_context = 0; - } else { + else max_context = max_context_real; - } + return 1; } @@ -181,9 +231,8 @@ determine_ignore_components (struct file_list *list1, struct file_list *list2) const char *stripped1 = stripped(l1->file, p); for (struct file_list *l2 = list2; l2; l2 = l2->next) { const char *stripped2 = stripped(l2->file, p); - if (!strcmp(stripped1, stripped2)) { + if (!strcmp(stripped1, stripped2)) return p; - } } } } @@ -1050,15 +1099,24 @@ trim_context (FILE *f /* positioned at start of @@ line */, printf ("Trim: %lu,%lu\n", strip_pre, strip_post); fsetpos (f, &pos); - fprintf (out, "@@ -%lu", orig_offset); - if (new_orig_count != 1) - fprintf (out, ",%lu", new_orig_count); - fprintf (out, " +%lu", new_offset); - if (new_new_count != 1) - fprintf (out, ",%lu", new_new_count); - fprintf (out, " @@\n"); + { + if (new_orig_count != 1 && new_new_count != 1) { + print_color (out, LINE_HUNK, "@@ -%lu,%lu +%lu,%lu @@\n", + orig_offset, new_orig_count, new_offset, new_new_count); + } else if (new_orig_count != 1) { + print_color (out, LINE_HUNK, "@@ -%lu,%lu +%lu @@\n", + orig_offset, new_orig_count, new_offset); + } else if (new_new_count != 1) { + print_color (out, LINE_HUNK, "@@ -%lu +%lu,%lu @@\n", + orig_offset, new_offset, new_new_count); + } else { + print_color (out, LINE_HUNK, "@@ -%lu +%lu @@\n", + orig_offset, new_offset); + } + } while (total_count--) { + enum line_type type; ssize_t got = getline (&line, &linelen, f); assert (got > 0); @@ -1070,7 +1128,18 @@ trim_context (FILE *f /* positioned at start of @@ line */, if (total_count < strip_post) continue; - fwrite (line, (size_t) got, 1, out); + switch (line[0]) { + case '+': + type = LINE_ADDED; + break; + case '-': + type = LINE_REMOVED; + break; + default: + fwrite (line, (size_t) got, 1, out); + continue; + } + print_color (out, type, "%s", line); } } @@ -1264,13 +1333,13 @@ output_delta (FILE *p1, FILE *p2, FILE *out) d = *q; *q = '\0'; } - fprintf (out, DIFF " %s %s %s\n", options, oldname + 4, - newname + 4); + print_color (out, LINE_HEADER, DIFF " %s %s %s\n", options, oldname + 4, + newname + 4); if (p) *p = c; if (q) *q = d; } - fprintf (out, "--- %s\n", oldname + 4); - fprintf (out, "+++ %s\n", newname + 4); + print_color (out, LINE_FILE, "--- %s\n", oldname + 4); + print_color (out, LINE_FILE, "+++ %s\n", newname + 4); rewind (tmpdiff); trim_context (tmpdiff, file.unline, out); fclose (tmpdiff); @@ -2096,12 +2165,10 @@ interdiff (FILE *p1, FILE *p2, const char *patch1, const char *patch2) if (flipdiff_inplace) { /* Use atomic in-place writing for safety */ - if (write_file_inplace(patch2, flip1) != 0) { + if (write_file_inplace(patch2, flip1) != 0) error (EXIT_FAILURE, errno, "failed to write %s", patch2); - } - if (write_file_inplace(patch1, flip2) != 0) { + if (write_file_inplace(patch1, flip2) != 0) error (EXIT_FAILURE, errno, "failed to write %s", patch1); - } } else { copy (flip1, stdout); puts ("\n=== 8< === cut here === 8< ===\n"); @@ -2280,12 +2347,12 @@ main (int argc, char *argv[]) const char *color_mode = optarg ? optarg : "auto"; /* Handle auto mode: check if stdout is a terminal */ - if (strcmp(color_mode, "auto") == 0) { + if (strcmp(color_mode, "auto") == 0) color_mode = isatty(STDOUT_FILENO) ? "always" : "never"; - } - if (asprintf (diff_opts + num_diff_opts++, "--color=%s", color_mode) < 0) - error (EXIT_FAILURE, errno, "Memory allocation failed"); + /* Set our internal color flag instead of passing to diff */ + use_colors = (strcmp(color_mode, "always") == 0); + color_option_specified = 1; break; } case 1000 + 'I': @@ -2317,18 +2384,9 @@ main (int argc, char *argv[]) error (EXIT_FAILURE, 0, "-z and --in-place are mutually exclusive."); - /* Add default color=always if no color option was specified and we're in a terminal */ - int has_color_option = 0; - for (int i = 0; i < num_diff_opts; i++) { - if (strncmp(diff_opts[i], "--color", 7) == 0) { - has_color_option = 1; - break; - } - } - if (!has_color_option && isatty(STDOUT_FILENO)) { - if (asprintf (diff_opts + num_diff_opts++, "--color=always") < 0) - error (EXIT_FAILURE, errno, "Memory allocation failed"); - } + /* Set default color behavior if no color option was specified */ + if (!color_option_specified && isatty(STDOUT_FILENO)) + use_colors = 1; if (optind + 2 != argc) syntax (1); diff --git a/tests/interdiff-color-context/run-test b/tests/interdiff-color-context/run-test index b03d2782..dbd3da1a 100755 --- a/tests/interdiff-color-context/run-test +++ b/tests/interdiff-color-context/run-test @@ -2,48 +2,113 @@ # This is an interdiff(1) testcase. # Test: --color option should not break context trimming +# +# The bug: When --color is passed to internal diff, ANSI escape codes +# break trim_context()'s ability to detect and strip "unline" content. +# This test verifies the bug is fixed by ensuring --color doesn't break interdiff. . ${top_srcdir-.}/tests/common.sh -# Create test files -cat << EOF > file +# Create test files that will generate interdiff output +cat << EOF > original.txt line1 line2 line3 +line4 +line5 +line6 +line7 +line8 +line9 line10 -line11 -line12 EOF -# First patch -cat << EOF > patch1 ---- file -+++ file -@@ -1,3 +1,3 @@ +# Create patches that modify overlapping regions to force interdiff generation +cat << EOF > patch1.patch +--- original.txt ++++ version1.txt +@@ -1,6 +1,6 @@ line1 --line2 -+MODIFIED2 - line3 + line2 +-line3 ++MODIFIED3 + line4 + line5 + line6 +@@ -7,4 +7,4 @@ + line7 + line8 +-line9 ++MODIFIED9 + line10 EOF -# Second patch -cat << EOF > patch2 ---- file -+++ file -@@ -4,3 +4,3 @@ - line10 --line11 -+MODIFIED11 - line12 +cat << EOF > patch2.patch +--- original.txt ++++ version2.txt +@@ -2,6 +2,6 @@ + line2 + line3 + line4 +-line5 ++CHANGED5 + line6 + line7 +@@ -8,3 +8,3 @@ + line8 + line9 +-line10 ++CHANGED10 EOF -# Generate interdiff with --color=always -${INTERDIFF} --color=always patch1 patch2 2>errors >result || { cat errors; exit 1; } +# Test the key requirement: --color should not break interdiff functionality +# The original bug would cause interdiff to fail or produce incorrect output + +# First, test without color to establish baseline +${INTERDIFF} patch1.patch patch2.patch >baseline.out 2>baseline.err +baseline_exit=$? + +# Then test with --color=always +${INTERDIFF} --color=always patch1.patch patch2.patch >color.out 2>color.err +color_exit=$? + +# Key test 1: Both should have the same exit status +if [ $baseline_exit -ne $color_exit ]; then + echo "ERROR: --color option changes interdiff exit status" + echo "Baseline exit: $baseline_exit, Color exit: $color_exit" + exit 1 +fi + +# Key test 2: If baseline succeeded, color version should also produce reasonable output +if [ $baseline_exit -eq 0 ]; then + # Strip ANSI codes from color output for comparison + sed 's/\x1b\[[0-9;]*m//g' color.out > color_stripped.out + + # The semantic content should be equivalent + if ! diff baseline.out color_stripped.out >/dev/null 2>&1; then + echo "ERROR: --color option changes interdiff semantic output" + echo "This indicates the color implementation interferes with diff processing" + echo + echo "=== Baseline output ===" + cat baseline.out + echo + echo "=== Color output (stripped) ===" + cat color_stripped.out + echo + echo "=== Difference ===" + diff baseline.out color_stripped.out || true + exit 1 + fi +fi -# Apply patch1 to create the base state for testing the interdiff -cp file file-with-patch1 -${PATCH} file-with-patch1 < patch1 || exit 1 +# Key test 3: The color output should not contain malformed content +# Look for signs that trim_context() failed due to color codes +if grep -E '^\x1b\[[0-9;]*m[!]+' color.out >/dev/null 2>&1; then + echo "ERROR: Found colored unline artifacts in output" + echo "This suggests trim_context() failed to strip artificial content" + echo "Raw output:" + cat -v color.out + exit 1 +fi -# Test that the interdiff result can be applied cleanly -cp file-with-patch1 file-test -${PATCH} file-test < result || exit 1 +echo "SUCCESS: --color option does not break interdiff context trimming"