Re: [PATCH v3] diff: ensure consistent diff behavior with ignore options
From: Junio C Hamano <hidden>
Date: 2025-08-06 20:56:31
Lidong Yan [off-list ref] writes:
+static int quick_consume(void *priv, char *line, unsigned long len)
+{
+ struct emit_callback *ecbdata = priv;
+ struct diff_options *o = ecbdata->opt;
+
+ o->found_changes = 1;
+ return 1;
+}"make DEVELOPER=YesPlease" would not be very happy, without -static int quick_consume(void *priv, char *line, unsigned long len) +static int quick_consume(void *priv, char *line UNUSED, unsigned long len UNUSED)
+/* return 1 if any change is found; otherwise, return 0 */
+static int diff_flush_patch_quiet(struct diff_filepair *p, struct diff_options *o)
+{
+ int diff_opt = o->diff_optimize;
+ int found_changes = o->found_changes;
+ int ret;
+
+ o->diff_optimize = DIFF_OPT_DRY_RUN;
+ o->found_changes = 0;
+ diff_flush_patch(p, o);
+ ret = o->found_changes;
+ o->diff_optimize = diff_opt;
+ o->found_changes |= found_changes;
+ return ret;
+}quoted hunk
static void diff_flush_stat(struct diff_filepair *p, struct diff_options *o, struct diffstat_t *diffstat) {@@ -6778,7 +6810,19 @@ 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)) + int need_flush = 1; + + if (!check_pair_status(p)) + continue; + + if (options->flags.diff_from_contents) { + if (diff_flush_patch_quiet(p, options)) + need_flush = 1; + else + need_flush = 0; + } + + if (need_flush) flush_one_pair(p, options); }
I am having a hard time understanding the logic in this loop. Is it
equivalent to the following loop, and ...
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
if (!check_pair_status(p))
continue;
if (options->flags.diff_from_contents &&
!diff_flush_patch_quietly(p, options))
continue; /* no changes */
flush_one_pair(p, options);
}
... if so, isn't the above rewrite easier to follow?
The idea is that we cannot do anything with DIFF_STATUS_UNKNOWN
pairs (hence we continue), and when we are filtering output by the
"has the pair changed in a way that the user cares about?" criteria,
we check with flush_patch_quietly, and if there is no change worth
talking about, we do not do anything with such a pair, either.
Only when these conditions are not met, we call flush_one_pair() to
show the name or name status or whatever.
quoted hunk
separator++;@@ -6831,19 +6875,10 @@ void diff_flush(struct diff_options *options) 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); + diff_flush_patch_quiet(p, options); if (options->found_changes) break; }
It is a natural consequence of having a helper that does the "quiet" thing that we can now lose the /dev/null hack, which is very nice. Overall, the changes are easy to follow and look good. Thanks.