Re: [PATCH v2 2/4] line-log: route -L output through the standard diff pipeline

[Junio C Hamano <gitster@pobox.com>]

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Michael Montalbo <mmontalbo@gmail.com>
>
> `git log -L` has always bypassed the standard diff pipeline.
> `dump_diff_hacky()` in line-log.c hand-rolls its own diff headers and
> hunk output, which means most diff formatting options are silently
> ignored.  A NEEDSWORK comment has acknowledged this since the feature
> was introduced:
>
>     /*
>      * NEEDSWORK: manually building a diff here is not the Right
>      * Thing(tm).  log -L should be built into the diff pipeline.
>      */
>
> Remove `dump_diff_hacky()` and its helpers and route -L output through
> `builtin_diff()` / `fn_out_consume()`, the same path used by `git diff`
> and `git log -p`.  The mechanism is a pair of callback wrappers that sit
> between `xdi_diff_outf()` and `fn_out_consume()`, filtering xdiff's
> output to only the tracked line ranges.  To ensure xdiff emits all lines
> within each range as context, the context length is inflated to span the
> largest range.
>
> Wire up the `-L` implies `--patch` default in revision setup rather
> than forcing it at output time, so `line_log_print()` is just
> `diffcore_std()` + `diff_flush()` with no format save/restore.
> Rename detection is a no-op since pairs are already resolved during
> the history walk in `queue_diffs()`, but running `diffcore_std()`
> means `-S`/`-G` (pickaxe), `--orderfile`, and `--diff-filter` now
> work with `-L`, and `diff_resolve_rename_copy()` sets pair statuses
> correctly without manual assignment.
>
> Switch `diff_filepair_dup()` from `xmalloc` to `xcalloc` so that new
> fields (including `line_ranges`) are zero-initialized by default.
>
> As a result, diff formatting options that were previously silently
> ignored (e.g. --word-diff, --no-prefix, -w, --color-moved) now work
> with -L, and output gains `index` lines, `new file mode` headers, and
> funcname context in `@@` headers.  This is a user-visible output change:
> tools that parse -L output may need to handle the additional header
> lines.
>
> The context-length inflation means xdiff may process more output than
> needed for very wide line ranges, but benchmarks on files up to 7800
> lines show no measurable regression.
>
> Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
> ---
>  diff.c                                        | 279 +++++++++++++++++-
>  diffcore.h                                    |  16 +
>  line-log.c                                    | 174 ++---------
>  line-log.h                                    |  14 +-
>  revision.c                                    |   2 +
>  t/t4211-line-log.sh                           |  12 +-
>  t/t4211/sha1/expect.beginning-of-file         |   4 +
>  t/t4211/sha1/expect.end-of-file               |  11 +-
>  t/t4211/sha1/expect.move-support-f            |   5 +
>  t/t4211/sha1/expect.multiple                  |  10 +-
>  t/t4211/sha1/expect.multiple-overlapping      |   7 +
>  t/t4211/sha1/expect.multiple-superset         |   7 +
>  t/t4211/sha1/expect.no-assertion-error        |  12 +-
>  t/t4211/sha1/expect.parallel-change-f-to-main |   7 +
>  t/t4211/sha1/expect.simple-f                  |   4 +
>  t/t4211/sha1/expect.simple-f-to-main          |   5 +
>  t/t4211/sha1/expect.simple-main               |  11 +-
>  t/t4211/sha1/expect.simple-main-to-end        |  11 +-
>  t/t4211/sha1/expect.two-ranges                |  10 +-
>  t/t4211/sha1/expect.vanishes-early            |  10 +-
>  t/t4211/sha256/expect.beginning-of-file       |   4 +
>  t/t4211/sha256/expect.end-of-file             |  11 +-
>  t/t4211/sha256/expect.move-support-f          |   5 +
>  t/t4211/sha256/expect.multiple                |  10 +-
>  t/t4211/sha256/expect.multiple-overlapping    |   7 +
>  t/t4211/sha256/expect.multiple-superset       |   7 +
>  t/t4211/sha256/expect.no-assertion-error      |  12 +-
>  .../sha256/expect.parallel-change-f-to-main   |   7 +
>  t/t4211/sha256/expect.simple-f                |   4 +
>  t/t4211/sha256/expect.simple-f-to-main        |   5 +
>  t/t4211/sha256/expect.simple-main             |  11 +-
>  t/t4211/sha256/expect.simple-main-to-end      |  11 +-
>  t/t4211/sha256/expect.two-ranges              |  10 +-
>  t/t4211/sha256/expect.vanishes-early          |  10 +-
>  34 files changed, 512 insertions(+), 213 deletions(-)

Huge diff to the test material mostly comes from the addition of the
diff headers like the index line, etc., which makes this patch scary
but is very welcome addition.

> @@ -106,6 +117,11 @@ int diff_filespec_is_binary(struct repository *, struct diff_filespec *);
>  struct diff_filepair {
>  	struct diff_filespec *one;
>  	struct diff_filespec *two;
> +	/*
> +	 * Tracked line ranges for -L filtering; borrowed from
> +	 * line_log_data and must not be freed.
> +	 */
> +	const struct range_set *line_ranges;

OK.

> diff --git a/line-log.c b/line-log.c
> index 9d12ece181..858a899cd2 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -885,160 +885,6 @@ static void queue_diffs(struct line_log_data *range,
>  	move_diff_queue(queue, &diff_queued_diff);
>  }
>  int line_log_print(struct rev_info *rev, struct commit *commit)
>  {
> -
>  	show_log(rev);
>  	if (!(rev->diffopt.output_format & DIFF_FORMAT_NO_OUTPUT)) {
>  		struct line_log_data *range = lookup_line_range(rev, commit);
> -		dump_diff_hacky(rev, range);
> +		struct line_log_data *r;
> +		const char *prefix = diff_line_prefix(&rev->diffopt);
> +
> +		fprintf(rev->diffopt.file, "%s\n", prefix);
> +
> +		for (r = range; r; r = r->next) {
> +			if (r->pair) {
> +				struct diff_filepair *p =
> +					diff_filepair_dup(r->pair);
> +				p->line_ranges = &r->ranges;
> +				diff_q(&diff_queued_diff, p);
> +			}
> +		}
> +
> +		diffcore_std(&rev->diffopt);
> +		diff_flush(&rev->diffopt);

Very welcome change.