Lidong Yan <yldhome2d2@xxxxxxxxx> 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;
> +}
> 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.
> 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.
