| Message ID | 20230818235932.3253552-2-gitster@pobox.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5f107caed755dae950382fdafc76a33b31528caa |
| Headers | show |
| Series | fix interactions with "-w" and "--exit-code" | expand |
On Fri, Aug 18, 2023 at 04:59:28PM -0700, Junio C Hamano wrote: > diff --git a/diff.c b/diff.c > index d52db685f7..0ce678fc06 100644 > --- a/diff.c > +++ b/diff.c > @@ -6551,6 +6551,21 @@ void diff_flush(struct diff_options *options) > separator++; > } > > + if (output_format & DIFF_FORMAT_PATCH) { > + if (separator) { This step makes sense, but I did a double-take when looking at the patch, because it is moving code _up_ rather than _down_. But the problem is that the block you moved was larger than the intervening bit, so the diff chose to flip-flop the context and changed bits. Obviously orthogonal to your series, but I wonder if there's a way to convince Git to show what actually happened. I don't think this is really a heuristic or algorithm problem. Seeing the pre- and post-images, it can't know whether it was "move A up" or "move B down", and the "real" diff is simply much larger in this case. -Peff
diff --git a/diff.c b/diff.c index d52db685f7..0ce678fc06 100644 --- a/diff.c +++ b/diff.c @@ -6551,6 +6551,21 @@ void diff_flush(struct diff_options *options) separator++; } + if (output_format & DIFF_FORMAT_PATCH) { + if (separator) { + emit_diff_symbol(options, DIFF_SYMBOL_SEPARATOR, NULL, 0, 0); + if (options->stat_sep) + /* attach patch instead of inline */ + emit_diff_symbol(options, DIFF_SYMBOL_STAT_SEP, + NULL, 0, 0); + } + + diff_flush_patch_all_file_pairs(options); + } + + if (output_format & DIFF_FORMAT_CALLBACK) + options->format_callback(q, options, options->format_callback_data); + if (output_format & DIFF_FORMAT_NO_OUTPUT && options->flags.exit_with_status && options->flags.diff_from_contents) { @@ -6572,21 +6587,6 @@ void diff_flush(struct diff_options *options) } } - if (output_format & DIFF_FORMAT_PATCH) { - if (separator) { - emit_diff_symbol(options, DIFF_SYMBOL_SEPARATOR, NULL, 0, 0); - if (options->stat_sep) - /* attach patch instead of inline */ - emit_diff_symbol(options, DIFF_SYMBOL_STAT_SEP, - NULL, 0, 0); - } - - diff_flush_patch_all_file_pairs(options); - } - - if (output_format & DIFF_FORMAT_CALLBACK) - options->format_callback(q, options, options->format_callback_data); - for (i = 0; i < q->nr; i++) diff_free_filepair(q->queue[i]); free_queue:
When "--exit-code" is asked and the code cannot just answer by comparing the object names on both sides but needs to inspect and compare the contents, there are two ways that the result is found out. Some output modes, like "--stat" and "--patch", inherently have to inspect the contents in order to show the differences in the way they do. The codepaths for these modes set the .found_changes bit as they compute what to show. However, other output modes do not need to inspect the contents to show the differences in the way they do. The most notable example is "--quiet", which does not need to compute any output to show. When they are asked to report "--exit-code", they run the codepaths for the "--patch" output with their output redirected to "/dev/null", only to set the .found_changes bit. Currently, this fallback invocation of "--patch" output is done after the "--stat" output format and its friends and before the "--patch" and internal callback logic. Move it to the end of the sequence to clarify the fallback status of this code block. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diff.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)