Thread (1 message) 1 message, 1 author, 2025-08-03

Re: [PATCH] diff: ensure consistent diff behavior with -I<regex> across output formats

From: Junio C Hamano <hidden>
Date: 2025-08-03 15:43:57

Jeff King [off-list ref] writes:
quoted hunk
We already have the diff_from_contents flag which is used for
--exit-code. We should be able to see where that logic is applied and do
something similar. It looks like it happens in diff_flush(), which makes
sense:

          if (output_format & DIFF_FORMAT_NO_OUTPUT &&
              options->flags.exit_with_status &&
              options->flags.diff_from_contents) {
                  /*
                   * run diff_flush_patch for the exit status. setting
                   * options->file to /dev/null should be safe, because we
                   * aren't supposed to produce any output anyway.
                   */
                  diff_free_file(options);
                  options->file = xfopen("/dev/null", "w");
                  options->close_file = 1;
                  options->color_moved = 0;
                  for (i = 0; i < q->nr; i++) {
                          struct diff_filepair *p = q->queue[i];
                          if (check_pair_status(p))
                                  diff_flush_patch(p, options);
                          if (options->found_changes)
                                  break;
                  }
          }

So here's a naive application of the same technique:
diff --git a/diff.c b/diff.c
index 76291e238c..0fe6eb7443 100644
--- a/diff.c
+++ b/diff.c
@@ -6845,8 +6845,28 @@ void diff_flush(struct diff_options *options)
 			     DIFF_FORMAT_CHECKDIFF)) {
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
-			if (check_pair_status(p))
-				flush_one_pair(p, options);
+
+			if (!check_pair_status(p))
+				continue;
+
+			if (options->flags.diff_from_contents) {
+				FILE *orig_out = options->file;
+				int orig_changes = options->found_changes;
+				int skip;
+
+				options->file = xfopen("/dev/null", "w");
+				diff_flush_patch(p, options);
+				skip = !options->found_changes;
+
+				fclose(options->file);
+				options->file = orig_out;
+				options->found_changes = orig_changes;
+
+				if (skip)
+					continue;
+			}
+
+			flush_one_pair(p, options);
 		}
 		separator++;
 	}
which works on a trivial example. It affects all of raw, name-only,
name-status, and checkdiff. I know Junio said that --raw should not be
affected, but I'm not sure I agree. Anyway, it should be possible to
split the logic by output type.
This is much more palatable to avoid code duplication and
repetition, compared to what was posted before.  A new code that
loops over q->queue[] one more time (which is yucky but cannot be
helped due to output order as you noted earlier) and runs
diff_flush_patch(), like the one we see above, is probably the best
we can do.  The beauty of the above approach is that the new code
only needs to know about a single .diff_from_contents bit, and does
not have to care what features cause the .diff_from_contents bit to
be flipped on.  Doing anything more would be maintenance nightmare.

Thanks.


Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help