Michael J Gruber <git@xxxxxxxxxxxxxxxxxxxx> writes:
>> The "test" command is used as it does not generate any output on stdout.
>
> "test" is a bit of a red herring here since it will receive commands.
> But your example works even with "true" which ignores its commands and
> produces no output.
Either sounds strange, as they exit without reading their input at
all, so the other side of the pipe may get an write error it has to
handle ;-)
> The description doesn't make it clear whether exit-code refers to
> the actual diff (foo vs. bar) or to the diff after textconv (empty
> vs. empty). In any case, "-b" should not make a difference for
> your example.
>
> diff_flush() in diff.c has this piece of code:
>
It is understandable that "-b" makes difference. The actual
short-cut happens a bit before the code you quoted.
if (output_format & DIFF_FORMAT_NO_OUTPUT &&
DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&
DIFF_OPT_TST(options, 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.
*/
if (options->close_file)
fclose(options->file);
options->file = fopen("/dev/null", "w");
if (!options->file)
die_errno("Could not open /dev/null");
options->close_file = 1;
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;
}
}
When running without producing output but we need the exit status,
unless DIFF_FROM_CONTENTS is set (e.g. "-b" in use), we do not run
the code that would inspect the blob contents that would have
produced the actual patch. When DIFF_FROM_CONTENTS is set, we need
to dig into the blob contents and the body of the above if() gets
run only to trigger o->found_changes (e.g. in builtin_diff() that
sets ecbdata.found_changesp to point at o->found_changes before
calling into the xdiff machinery), but the output is discarded to
/dev/null.
The textconv filter is an unfortunate beast, in that while some
paths use one while others don't, so it won't be as simple as "-b"
that flips DIFF_FROM_CONTENTS to say "We need to inspect blobs for
ALL FILEPAIRS to see if there is any difference" if we want to keep
the optimization as much as possible.
Without DIFF_FROM_CONTENTS set, any filepair that point at two
different blobs that might end up to be the same after applying
textconv will flip the o->found_changes bit on, and with
EXIT_WITH_STATUS bit on, we have another optimization to skip
checking the remainder of the filepairs. So if you want to support
textconv with QUICK, you would also need to disable that
optimization by teaching diff_change() to refrain from setting
HAS_CHANGES bit, i.e.
diff --git a/diff.h b/diff.h
index 125447b..f6c0c07 100644
--- a/diff.h
+++ b/diff.h
@@ -69,7 +69,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
#define DIFF_OPT_FIND_COPIES_HARDER (1 << 6)
#define DIFF_OPT_FOLLOW_RENAMES (1 << 7)
#define DIFF_OPT_RENAME_EMPTY (1 << 8)
-/* (1 << 9) unused */
+#define DIFF_OPT_WITH_TEXTCONV (1 << 9)
#define DIFF_OPT_HAS_CHANGES (1 << 10)
#define DIFF_OPT_QUICK (1 << 11)
#define DIFF_OPT_NO_INDEX (1 << 12)
diff --git a/diff.c b/diff.c
index 4dfe660..0016ad2 100644
--- a/diff.c
+++ b/diff.c
@@ -5018,6 +5018,11 @@ void diff_change(struct diff_options *options,
two->dirty_submodule = new_dirty_submodule;
p = diff_queue(&diff_queued_diff, one, two);
+ if (either path one or path two has textconv defined) {
+ DIFF_OPT_SET(options, DIFF_WITH_TEXTCONV);
+ return;
+ }
+
if (DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))
return;
And then in order to keep the optimization, add this to the above
codepath:
diff --git a/diff.c b/diff.c
index 4dfe660..7b318cc 100644
--- a/diff.c
+++ b/diff.c
@@ -4598,6 +4598,7 @@ void diff_flush(struct diff_options *options)
int i, output_format = options->output_format;
int separator = 0;
int dirstat_by_line = 0;
+ int textconv_hack;
/*
* Order: raw, stat, summary, patch
@@ -4652,9 +4653,17 @@ void diff_flush(struct diff_options *options)
separator++;
}
+ /*
+ * If there is any path that needs textconv and we haven't
+ * found any change on paths that don't, we need to pass
+ * them through textconv and see the textual difference.
+ */
+ textconv_hack = (DIFF_OPT_TST(options, DIFF_WITH_TEXTCONV) &&
+ !DIFF_OPT_TST(options, HAS_CHANGES));
+
if (output_format & DIFF_FORMAT_NO_OUTPUT &&
DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&
- DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {
+ (textconv_hack || DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))) {
/*
* run diff_flush_patch for the exit status. setting
* options->file to /dev/null should be safe, because we
so that if we find some other change (e.g. diff_addremove() noticed
a path added or removed, or diff_change() noticed a changed blob
that does not have textconv defined), we do not have to inspect all
the paths to see if there is any change at all by going into the
body of this if() block.
And then finally, clean things up in a way similar to how
DIFF_FROM_CONTENTS codepath handles the HAS_CHANGES bit.
diff --git a/diff.c b/diff.c
index 4dfe660..7b318cc 100644
--- a/diff.c
+++ b/diff.c
@@ -4709,7 +4718,7 @@ free_queue:
* diff_addremove/diff_change does not set the bit when
* DIFF_FROM_CONTENTS is in effect (e.g. with -w).
*/
- if (DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {
+ if (textconv_hack || DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {
if (options->found_changes)
DIFF_OPT_SET(options, HAS_CHANGES);
else
By the way, I have a suspicion that the existing code immediately
after the context of this hunk is wrong to clear HAS_CHANGES bit
when options->found_changes is clear. HAS_CHANGES bit should be
clear upon entry to this function, when DIFF_FROM_CONTENTS is in
use.
I also suspect that addremove() that refrains from setting
HAS_CHANGES when DIFF_FROM_CONTENTS is in effect is wrong. No
matter what combination of -w or -b is used, an addition of a new
file, or a removal of an existing file, is a change.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html