Skip to contentSkip to conversation

line-log: integrate -L with the standard log output pipeline

PATCH Superseded Michael Montalbo via GitGitGadget 4 participants 20 messages Apr 28 – Jun 1 1 message not in the archive Documentation

lore
Share & tools
lore
#0 lore raw

cover line-log: integrate -L with the standard log output pipeline

Since its introduction, git log -L has short-circuited from log_tree_commit() into its own output function, bypassing log_tree_diff() and log_tree_diff_flush(). This skips no_free save/restore, always_show_header, diff_free() cleanup, and means that pickaxe (-S, -G, --find-object) and --diff-filter cannot suppress commits whose pairs are all filtered out, because show_log() runs before diffcore_std().

This series restructures the flow so that -L goes through the same log_tree_diff() -> log_tree_diff_flush() path as normal single-parent and merge diffs, then uses that to enable several non-patch diff formats.

Patch 1: revision: move -L setup before output_format-to-diff derivation

Preparatory reorder in setup_revisions(). The -L block sets a default DIFF_FORMAT_PATCH when no format is requested; move it before the derivation of revs->diff from output_format so the default is visible to that check. No behavior change on its own.

Patch 2: line-log: integrate -L output with the standard log-tree pipeline

Rename line_log_print() to line_log_queue_pairs(), stripping it down to only queue pre-computed filepairs. log_tree_diff_flush() handles show_log(), diffcore_std(), and diff_flush(). This fixes pickaxe and --diff-filter suppression, and aligns the commit/diff separator with the rest of log output. Also rejects --full-diff, which is meaningless when filepairs are pre-computed.

Patch 3: line-log: allow non-patch diff formats with -L

Expand the allowlist to accept --raw, --name-only, --name-status, and --summary. These only read filepair metadata already set by the line-log machinery. Diff stat formats (--stat, --numstat, --shortstat, --dirstat) remain blocked because they call compute_diffstat() on full blob content and would show whole-file statistics rather than range-scoped ones.

Michael Montalbo (3):
revision: move -L setup before output_format-to-diff derivation
line-log: integrate -L output with the standard log-tree pipeline
line-log: allow non-patch diff formats with -L

 Documentation/line-range-options.adoc         | 10 +-
 line-log.c                                    | 30 ++----
 line-log.h                                    |  2 +-
 log-tree.c                                    |  9 +-
 revision.c                                    | 25 +++--
 t/t4211-line-log.sh                           | 99 ++++++++++++++++---
 t/t4211/sha1/expect.parallel-change-f-to-main |  1 -
 .../sha256/expect.parallel-change-f-to-main   |  1 -
 8 files changed, 120 insertions(+), 57 deletions(-)

base-commit: 9f223ef1c026d91c7ac68cc0211bde255dda6199
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2094%2Fmmontalbo%2Fmm%2Fline-log-use-log-tree-diff-flush-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2094/mmontalbo/mm/line-log-use-log-tree-diff-flush-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2094

Signature · 1 line
gitgitgadget
#1 lore raw

PATCH 1/3 revision: move -L setup before output_format-to-diff derivation

From: Michael Montalbo <[email protected]>

The line_level_traverse block sets a default DIFF_FORMAT_PATCH when no output format has been explicitly requested. This default must be visible to the "Did the user ask for any diff output?" check that derives revs->diff from revs->diffopt.output_format.

Currently the -L block runs after that derivation, so revs->diff stays 0 when no explicit format is given. This does not matter yet because log_tree_commit() short-circuits into line_log_print() before consulting revs->diff, but the next commit will route -L through the normal log_tree_diff() path, which checks revs->diff.

Move the block above the derivation so the default DIFF_FORMAT_PATCH is in place when revs->diff is computed. No behavior change on its own.

---
 revision.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
revision.c +8 -8
index 599b3a66c3..4a8e24bc38 100644
@@ -3112,6 +3112,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		object_context_release(&oc);
 	}
 
+	if (revs->line_level_traverse) {
+		if (want_ancestry(revs))
+			revs->limited = 1;
+		revs->topo_order = 1;
+		if (!revs->diffopt.output_format)
+			revs->diffopt.output_format = DIFF_FORMAT_PATCH;
+	}
+
 	/* Did the user ask for any diff output? Run the diff! */
 	if (revs->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT)
 		revs->diff = 1;
@@ -3125,14 +3133,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (revs->diffopt.objfind)
 		revs->simplify_history = 0;
 
-	if (revs->line_level_traverse) {
-		if (want_ancestry(revs))
-			revs->limited = 1;
-		revs->topo_order = 1;
-		if (!revs->diffopt.output_format)
-			revs->diffopt.output_format = DIFF_FORMAT_PATCH;
-	}
-
 	if (revs->topo_order && !generation_numbers_enabled(the_repository))
 		revs->limited = 1;
 
Signature · 1 line
gitgitgadget
#2 lore raw

PATCH 2/3 line-log: integrate -L output with the standard log-tree pipeline

In reply to Michael Montalbo via GitGitGadget

From: Michael Montalbo <[email protected]>

git log -L has bypassed log_tree_diff() and log_tree_diff_flush() since the feature was introduced, short-circuiting from log_tree_commit() directly into line_log_print(). This skips the no_free save/restore (noted in a NEEDSWORK comment added by f8781bfda3), the always_show_header fallback, show_diff_of_diff(), and diff_free() cleanup.

Restructure so that -L flows through log_tree_diff() -> log_tree_diff_flush(), the same path used by the normal single-parent and merge diff codepaths:

- Rename line_log_print() to line_log_queue_pairs() and strip it down to just queuing pre-computed filepairs. The show_log(), separator, diffcore_std(), and diff_flush() calls are removed since log_tree_diff_flush() handles all of those.

- In log_tree_diff(), call line_log_queue_pairs() then log_tree_diff_flush(), mirroring the diff_tree_oid() + flush pattern used by the single-parent and merge codepaths.

- Remove the early return in log_tree_commit() that bypassed no_free save/restore, always_show_header, and diff_free().

Because show_log() is now deferred until after diffcore_std() inside log_tree_diff_flush(), pickaxe (-S, -G, --find-object) and --diff-filter now properly suppress commits when all pairs are filtered out.

The blank-line separator between commit header and diff changes
slightly: the old code printed one unconditionally, while log_tree_diff_flush() only emits one for verbose headers. This matches the rest of log output.

Also reject --full-diff, which is meaningless with -L: the filepairs are pre-computed during the history walk and scoped to tracked paths, so there is no tree diff to widen.

Update tests accordingly.

---
 line-log.c                                    | 30 ++++-------
 line-log.h                                    |  2 +-
 log-tree.c                                    |  9 ++--
 revision.c                                    |  2 +
 t/t4211-line-log.sh                           | 52 ++++++++++++++-----
 t/t4211/sha1/expect.parallel-change-f-to-main |  1 -
 .../sha256/expect.parallel-change-f-to-main   |  1 -
 7 files changed, 56 insertions(+), 41 deletions(-)
line-log.c +9 -21
index 858a899cd2..7ee55b05cc 100644
@@ -13,7 +13,6 @@
 #include "revision.h"
 #include "xdiff-interface.h"
 #include "strbuf.h"
-#include "log-tree.h"
 #include "line-log.h"
 #include "setup.h"
 #include "strvec.h"
@@ -1004,29 +1003,18 @@ static int process_all_files(struct line_log_data **range_out,
 	return changed;
 }
 
-int line_log_print(struct rev_info *rev, struct commit *commit)
+void line_log_queue_pairs(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);
-		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);
-			}
-		}
+	struct line_log_data *range = lookup_line_range(rev, commit);
+	struct line_log_data *r;
 
-		diffcore_std(&rev->diffopt);
-		diff_flush(&rev->diffopt);
+	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);
+		}
 	}
-	return 1;
 }
 
 static int bloom_filter_check(struct rev_info *rev,
line-log.h +1 -1
index 04a6ea64d3..99e1755ce3 100644
@@ -46,7 +46,7 @@ int line_log_filter(struct rev_info *rev);
 int line_log_process_ranges_arbitrary_commit(struct rev_info *rev,
 						    struct commit *commit);
 
-int line_log_print(struct rev_info *rev, struct commit *commit);
+void line_log_queue_pairs(struct rev_info *rev, struct commit *commit);
 
 void line_log_free(struct rev_info *rev);
 
log-tree.c +5 -4
index 7e048701d0..1ead481891 100644
@@ -1105,6 +1105,11 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
 	if (!all_need_diff && !opt->merges_need_diff)
 		return 0;
 
+	if (opt->line_level_traverse) {
+		line_log_queue_pairs(opt, commit);
+		return log_tree_diff_flush(opt);
+	}
+
 	parse_commit_or_die(commit);
 	oid = get_commit_tree_oid(commit);
 
@@ -1179,10 +1184,6 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 	opt->loginfo = &log;
 	opt->diffopt.no_free = 1;
 
-	/* NEEDSWORK: no restoring of no_free?  Why? */
-	if (opt->line_level_traverse)
-		return line_log_print(opt, commit);
-
 	if (opt->track_linear && !opt->linear && !opt->reverse_output_stage)
 		fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
 	shown = log_tree_diff(opt, commit, &log);
revision.c +2 -0
index 4a8e24bc38..a1c795de96 100644
@@ -3181,6 +3181,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (revs->line_level_traverse &&
 	    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
 		die(_("-L does not yet support diff formats besides -p and -s"));
+	if (revs->line_level_traverse && revs->full_diff)
+		die(_("-L is not compatible with --full-diff"));
 
 	if (revs->expand_tabs_in_log < 0)
 		revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
t/t4211-line-log.sh +39 -13

94 lines not on this page · raw message

t/t4211/sha1/expect.parallel-change-f-to-main +0 -1
index 65a8cc673a..6d7a201036 100644
@@ -5,7 +5,6 @@ Date:   Fri Apr 12 16:16:24 2013 +0200
 
     Merge across the rename
 
-
 commit 6ce3c4ff690136099bb17e1a8766b75764726ea7
 Author: Thomas Rast <[email protected]>
 Date:   Thu Feb 28 10:49:50 2013 +0100
t/t4211/sha256/expect.parallel-change-f-to-main +0 -1
index 3178989253..c93e03bef4 100644
@@ -5,7 +5,6 @@ Date:   Fri Apr 12 16:16:24 2013 +0200
 
     Merge across the rename
 
-
 commit 4f7a58195a92c400e28a2354328587f1ff14fb77f5cf894536f17ccbc72931b9
 Author: Thomas Rast <[email protected]>
 Date:   Thu Feb 28 10:49:50 2013 +0100
Signature · 1 line
gitgitgadget
#3 lore raw

PATCH 3/3 line-log: allow non-patch diff formats with -L

In reply to Michael Montalbo via GitGitGadget

From: Michael Montalbo <[email protected]>

Now that -L flows through log_tree_diff_flush() and diff_flush(), metadata-only diff formats work because they only read filepair fields (status, mode, path, oid) already set on the pre-computed pairs.

Expand the allowlist in setup_revisions() to also accept --raw, --name-only, --name-status, and --summary. Diff stat formats (--stat, --numstat, --shortstat, --dirstat) remain blocked because they call compute_diffstat() on full blob content and would show whole-file statistics rather than range-scoped ones.

---
 Documentation/line-range-options.adoc | 10 +++---
 revision.c                            |  7 ++--
 t/t4211-line-log.sh                   | 47 +++++++++++++++++++++++++--
 3 files changed, 56 insertions(+), 8 deletions(-)
Documentation/line-range-options.adoc +6 -4
index ecb2c79fb9..72f639b5e7 100644
@@ -8,12 +8,14 @@
 	give zero or one positive revision arguments, and
 	_<start>_ and _<end>_ (or _<funcname>_) must exist in the starting revision.
 	You can specify this option more than once. Implies `--patch`.
-	Patch output can be suppressed using `--no-patch`, but other diff formats
-	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
-	`--name-only`, `--name-status`, `--check`) are not currently implemented.
+	Patch output can be suppressed using `--no-patch`.
+	Non-patch diff formats `--raw`, `--name-only`, `--name-status`,
+	and `--summary` are supported.  Diff stat formats
+	(`--stat`, `--numstat`, `--shortstat`, `--dirstat`) are not
+	currently implemented.
 +
 Patch formatting options such as `--word-diff`, `--color-moved`,
 `--no-prefix`, and whitespace options (`-w`, `-b`) are supported,
-as are pickaxe options (`-S`, `-G`).
+as are pickaxe options (`-S`, `-G`) and `--diff-filter`.
 +
 include::line-range-format.adoc[]
revision.c +5 -2
index a1c795de96..b41ec4016b 100644
@@ -3179,8 +3179,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		die(_("the option '%s' requires '%s'"), "--grep-reflog", "--walk-reflogs");
 
 	if (revs->line_level_traverse &&
-	    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
-		die(_("-L does not yet support diff formats besides -p and -s"));
+	    (revs->diffopt.output_format &
+	     ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT |
+	       DIFF_FORMAT_RAW | DIFF_FORMAT_NAME |
+	       DIFF_FORMAT_NAME_STATUS | DIFF_FORMAT_SUMMARY)))
+		die(_("-L does not yet support the requested diff format"));
 	if (revs->line_level_traverse && revs->full_diff)
 		die(_("-L is not compatible with --full-diff"));
 
t/t4211-line-log.sh +45 -2

57 lines not on this page · raw message

Signature · 1 line
gitgitgadget
#4 lore raw
In reply to Michael Montalbo via GitGitGadget
Quoted 10 lines · Michael Montalbo via GitGitGadget

Since its introduction, git log -L has short-circuited from log_tree_commit() into its own output function, bypassing log_tree_diff() and log_tree_diff_flush(). This skips no_free save/restore, always_show_header, diff_free() cleanup, and means that pickaxe (-S, -G, --find-object) and --diff-filter cannot suppress commits whose pairs are all filtered out, because show_log() runs before diffcore_std().

This series restructures the flow so that -L goes through the same log_tree_diff() -> log_tree_diff_flush() path as normal single-parent and merge diffs, then uses that to enable several non-patch diff formats.

This unfortunately saw no reviews and

https://lore.kernel.org/git/[email protected]/

does not show the previous rouns so I am assuming nobody is interested in the topic?

Or are people more busily writing their own patches than reviewing others' patches? Unfortunately that is not sustainable.

Quoted 44 lines

Patch 1: revision: move -L setup before output_format-to-diff derivation

Preparatory reorder in setup_revisions(). The -L block sets a default DIFF_FORMAT_PATCH when no format is requested; move it before the derivation of revs->diff from output_format so the default is visible to that check. No behavior change on its own.

Patch 2: line-log: integrate -L output with the standard log-tree pipeline

Rename line_log_print() to line_log_queue_pairs(), stripping it down to only queue pre-computed filepairs. log_tree_diff_flush() handles show_log(), diffcore_std(), and diff_flush(). This fixes pickaxe and --diff-filter suppression, and aligns the commit/diff separator with the rest of log output. Also rejects --full-diff, which is meaningless when filepairs are pre-computed.

Patch 3: line-log: allow non-patch diff formats with -L

Expand the allowlist to accept --raw, --name-only, --name-status, and --summary. These only read filepair metadata already set by the line-log machinery. Diff stat formats (--stat, --numstat, --shortstat, --dirstat) remain blocked because they call compute_diffstat() on full blob content and would show whole-file statistics rather than range-scoped ones.

Michael Montalbo (3):
revision: move -L setup before output_format-to-diff derivation
line-log: integrate -L output with the standard log-tree pipeline
line-log: allow non-patch diff formats with -L

 Documentation/line-range-options.adoc         | 10 +-
 line-log.c                                    | 30 ++----
 line-log.h                                    |  2 +-
 log-tree.c                                    |  9 +-
 revision.c                                    | 25 +++--
 t/t4211-line-log.sh                           | 99 ++++++++++++++++---
 t/t4211/sha1/expect.parallel-change-f-to-main |  1 -
 .../sha256/expect.parallel-change-f-to-main   |  1 -
 8 files changed, 120 insertions(+), 57 deletions(-)

base-commit: 9f223ef1c026d91c7ac68cc0211bde255dda6199
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2094%2Fmmontalbo%2Fmm%2Fline-log-use-log-tree-diff-flush-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2094/mmontalbo/mm/line-log-use-log-tree-diff-flush-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2094

#5 lore raw
In reply to Michael Montalbo via GitGitGadget

Hi Michael,

Quoted 11 lines · Michael Montalbo via GitGitGadget

Since its introduction, git log -L has short-circuited from log_tree_commit() into its own output function, bypassing log_tree_diff() and log_tree_diff_flush(). This skips no_free save/restore, always_show_header, diff_free() cleanup, and means that pickaxe (-S, -G, --find-object) and --diff-filter cannot suppress commits whose pairs are all filtered out, because show_log() runs before diffcore_std().

This series restructures the flow so that -L goes through the same log_tree_diff() -> log_tree_diff_flush() path as normal single-parent and merge diffs, then uses that to enable several non-patch diff formats.

Cleanup by itself to shrink the number of concepts in the code is already a good thing IMO, so getting additional features out of it is even nicer.

Quoted 6 lines

Patch 1: revision: move -L setup before output_format-to-diff derivation

Preparatory reorder in setup_revisions(). The -L block sets a default DIFF_FORMAT_PATCH when no format is requested; move it before the derivation of revs->diff from output_format so the default is visible to that check. No behavior change on its own.

Straightforward, nice.

Quoted 9 lines

Patch 2: line-log: integrate -L output with the standard log-tree pipeline

Rename line_log_print() to line_log_queue_pairs(), stripping it down to only queue pre-computed filepairs. log_tree_diff_flush() handles show_log(), diffcore_std(), and diff_flush(). This fixes pickaxe and --diff-filter suppression, and aligns the commit/diff separator with the rest of log output. Also rejects --full-diff, which is meaningless when filepairs are pre-computed.

At first I questioned the removal of the DIFF_FORMAT_NO_OUTPUT conditional in line_log_queue_pairs, but now that it only queues pairs it shouldn't be checking output formats. Good.

I also noted that log_tree_diff() returns the result of log_tree_diff_flush() in the -L case, which is a bit different from the other patterns. I think the difference is that the other cases have some conditional logic around the log_tree_diff_flush cases (?) but I'm not sure. Perhaps that branch should also be looking at opt->loginfo ?

Finally, I wonder if in describing the removal of the early return:

Quoted 2 lines

- Remove the early return in log_tree_commit() that bypassed no_free save/restore, always_show_header, and diff_free().

we might want to be more explicit that this is _because_ line-level diff is now handled in the regular pipeline?

[I suppose we could, in theory, split the rejection of --full-diff to a separate prep commit, idk.j]

Quoted 7 lines

Patch 3: line-log: allow non-patch diff formats with -L

Expand the allowlist to accept --raw, --name-only, --name-status, and --summary. These only read filepair metadata already set by the line-log machinery. Diff stat formats (--stat, --numstat, --shortstat, --dirstat) remain blocked because they call compute_diffstat() on full blob content and would show whole-file statistics rather than range-scoped ones.

Short and sweet.

The stat formats are kind of like --full-diff, and I think they should probably all be rejected or all allowed: since the stats are based on the full-diff, it makes sense to enable them if we can also make -L + --full-diff semantically sensible.

Otherwise, we'd need to find a way to make the stat formats scoped for -L.

Quoted 16 lines

Michael Montalbo (3):
revision: move -L setup before output_format-to-diff derivation
line-log: integrate -L output with the standard log-tree pipeline
line-log: allow non-patch diff formats with -L

 Documentation/line-range-options.adoc         | 10 +-
 line-log.c                                    | 30 ++----
 line-log.h                                    |  2 +-
 log-tree.c                                    |  9 +-
 revision.c                                    | 25 +++--
 t/t4211-line-log.sh                           | 99 ++++++++++++++++---
 t/t4211/sha1/expect.parallel-change-f-to-main |  1 -
 .../sha256/expect.parallel-change-f-to-main   |  1 -
 8 files changed, 120 insertions(+), 57 deletions(-)
Quoted 6 lines

base-commit: 9f223ef1c026d91c7ac68cc0211bde255dda6199
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2094%2Fmmontalbo%2Fmm%2Fline-log-use-log-tree-diff-flush-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2094/mmontalbo/mm/line-log-use-log-tree-diff-flush-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2094
--
gitgitgadget

A few other comments:

- Tests should use test_grep; some do, but some don't.
- There is one occurrence of "sed | grep" that I wonder if we want to rewrite to avoid issues with exit status one side of the pipe?

Thanks for working on this!

[Apologies for the unusual review format; this was easier for me at the moment than digging up the individual patches, and I don't think _most_ of the review would benefit from spreading out across multiple mails.]

Signature · 1 line
D. Ben Knoble

A message that is not in the archive (sent off-list or not yet received): CALnO6CBh7nDCwT=u1xSN2c6_x88t_gNfAaT_B4PzYKr=5i_bNA@mail.gmail.com

#7 lore raw
Quoted 3 lines · D. Ben Knoble

Hi Michael,

Hi Ben,

Thanks for the thorough review!

Quoted 37 lines · Michael Montalbo via GitGitGadget
Quoted 11 lines

Since its introduction, git log -L has short-circuited from log_tree_commit() into its own output function, bypassing log_tree_diff() and log_tree_diff_flush(). This skips no_free save/restore, always_show_header, diff_free() cleanup, and means that pickaxe (-S, -G, --find-object) and --diff-filter cannot suppress commits whose pairs are all filtered out, because show_log() runs before diffcore_std().

This series restructures the flow so that -L goes through the same log_tree_diff() -> log_tree_diff_flush() path as normal single-parent and merge diffs, then uses that to enable several non-patch diff formats.

Cleanup by itself to shrink the number of concepts in the code is already a good thing IMO, so getting additional features out of it is even nicer.

Quoted 6 lines

Patch 1: revision: move -L setup before output_format-to-diff derivation

Preparatory reorder in setup_revisions(). The -L block sets a default DIFF_FORMAT_PATCH when no format is requested; move it before the derivation of revs->diff from output_format so the default is visible to that check. No behavior change on its own.

Straightforward, nice.

Quoted 9 lines

Patch 2: line-log: integrate -L output with the standard log-tree pipeline

Rename line_log_print() to line_log_queue_pairs(), stripping it down to only queue pre-computed filepairs. log_tree_diff_flush() handles show_log(), diffcore_std(), and diff_flush(). This fixes pickaxe and --diff-filter suppression, and aligns the commit/diff separator with the rest of log output. Also rejects --full-diff, which is meaningless when filepairs are pre-computed.

Quoted 4 lines

At first I questioned the removal of the DIFF_FORMAT_NO_OUTPUT conditional in line_log_queue_pairs, but now that it only queues pairs it shouldn't be checking output formats. Good.

I also noted that log_tree_diff() returns the result of log_tree_diff_flush() in the -L case, which is a bit different from the other patterns. I think the difference is that the other cases have some conditional logic around the log_tree_diff_flush cases (?) but I'm not sure. Perhaps that branch should also be looking at opt->loginfo ?

Good catch. In practice I think they agree, since log_tree_diff_flush() returns 1 exactly when it calls show_log() which consumes loginfo, but matching the existing convention is cleaner. Will update.

Quoted 7 lines

Finally, I wonder if in describing the removal of the early return:

Quoted 2 lines

- Remove the early return in log_tree_commit() that bypassed no_free save/restore, always_show_header, and diff_free().

we might want to be more explicit that this is _because_ line-level diff is now handled in the regular pipeline?

Agreed, will reword to: "Remove the early return in log_tree_commit() that is no longer needed now that -L output flows through log_tree_diff() and log_tree_diff_flush(); this restores no_free save/restore, always_show_header, and diff_free() cleanup."

Quoted 2 lines

[I suppose we could, in theory, split the rejection of --full-diff to a separate prep commit, idk.j]

It felt natural to me to put alongside the integration since --full-diff is not yet implementable with pre-computed filepairs. Happy to split it out if you feel strongly though.

Quoted 9 lines
Quoted 7 lines

Patch 3: line-log: allow non-patch diff formats with -L

Expand the allowlist to accept --raw, --name-only, --name-status, and --summary. These only read filepair metadata already set by the line-log machinery. Diff stat formats (--stat, --numstat, --shortstat, --dirstat) remain blocked because they call compute_diffstat() on full blob content and would show whole-file statistics rather than range-scoped ones.

Quoted 6 lines

Short and sweet.

The stat formats are kind of like --full-diff, and I think they should probably all be rejected or all allowed: since the stats are based on the full-diff, it makes sense to enable them if we can also make -L + --full-diff semantically sensible.

Otherwise, we'd need to find a way to make the stat formats scoped for -L.

I am working on a follow up series that takes the second path you suggest: it adds a line-range filter in diffcore_std() that clips insertions and deletions to the tracked ranges before compute_diffstat() runs, so --stat, --numstat, etc. report range-scoped numbers. That series builds on top of these three patches, which is why stats remain blocked here.

For --full-diff, thinking about it more, the semantics would actually be well-defined: "filter commits by line range, but show the full diff for those commits." Right now, there might be a higher implementation barrier, though. The line-log machinery fuses commit filtering with diff generation, so there is no separate "full diff" to fall back to for display. I will soften the rejection to "not yet supported" rather than "incompatible," since it could be wired up if someone separates the two concerns.

Quoted 26 lines
Quoted 18 lines

Michael Montalbo (3):
revision: move -L setup before output_format-to-diff derivation
line-log: integrate -L output with the standard log-tree pipeline
line-log: allow non-patch diff formats with -L

 Documentation/line-range-options.adoc         | 10 +-
 line-log.c                                    | 30 ++----
 line-log.h                                    |  2 +-
 log-tree.c                                    |  9 +-
 revision.c                                    | 25 +++--
 t/t4211-line-log.sh                           | 99 ++++++++++++++++---
 t/t4211/sha1/expect.parallel-change-f-to-main |  1 -
 .../sha256/expect.parallel-change-f-to-main   |  1 -
 8 files changed, 120 insertions(+), 57 deletions(-)
Quoted 6 lines

base-commit: 9f223ef1c026d91c7ac68cc0211bde255dda6199
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2094%2Fmmontalbo%2Fmm%2Fline-log-use-log-tree-diff-flush-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2094/mmontalbo/mm/line-log-use-log-tree-diff-flush-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2094
--
gitgitgadget

Quoted 5 lines

A few other comments:

- Tests should use test_grep; some do, but some don't.
- There is one occurrence of "sed | grep" that I wonder if we want to rewrite to avoid issues with exit status one side of the pipe?

Will fix both these issues.

Quoted 4 lines

Thanks for working on this!

[Apologies for the unusual review format; this was easier for me at the moment than digging up the individual patches, and I don't think _most_ of the review would benefit from spreading out across multiple mails.]

Thank you too! This review format worked fine for me :)

Quoted 2 lines

--
D. Ben Knoble

#8 lore raw

v2 cover line-log: integrate -L with the standard log output pipeline

In reply to Michael Montalbo via GitGitGadget

Since its introduction, git log -L has short-circuited from log_tree_commit() into its own output function, bypassing log_tree_diff() and log_tree_diff_flush(). This skips no_free save/restore, always_show_header, diff_free() cleanup, and means that pickaxe (-S, -G, --find-object) and --diff-filter cannot suppress commits whose pairs are all filtered out, because show_log() runs before diffcore_std().

This series restructures the flow so that -L goes through the same log_tree_diff() -> log_tree_diff_flush() path as normal single-parent and merge diffs, then uses that to enable several non-patch diff formats.

Patch 1: revision: move -L setup before output_format-to-diff derivation

Preparatory reorder in setup_revisions(). The -L block sets a default DIFF_FORMAT_PATCH when no format is requested; move it before the derivation of revs->diff from output_format so the default is visible to that check. No behavior change on its own.

Patch 2: line-log: integrate -L output with the standard log-tree pipeline

Rename line_log_print() to line_log_queue_pairs(), stripping it down to only queue pre-computed filepairs. log_tree_diff_flush() handles show_log(), diffcore_std(), and diff_flush(). This fixes pickaxe and --diff-filter suppression, and aligns the commit/diff separator with the rest of log output. Rejects --full-diff, which is not yet supported when filepairs are pre-computed.

Patch 3: line-log: allow non-patch diff formats with -L

Expand the allowlist to accept --raw, --name-only, --name-status, and --summary. These only read filepair metadata already set by the line-log machinery. Diff stat formats (--stat, --numstat, --shortstat, --dirstat) remain blocked because they call compute_diffstat() on full blob content and would show whole-file statistics rather than range-scoped ones.

Changes since v1:

* Patch 2: use !opt->loginfo return convention in log_tree_diff() to match the existing single-parent and merge codepaths, instead of returning log_tree_diff_flush() directly.
* Patch 2: reword the early-return removal to explicitly tie it to the pipeline change.
* Patch 2: soften --full-diff rejection to "not yet supported".
* Patches 2-3: use test_grep consistently in new tests.
* Patch 2: replace sed | grep pipe with sed > file && test_grep for proper exit status handling.

Michael Montalbo (3):
revision: move -L setup before output_format-to-diff derivation
line-log: integrate -L output with the standard log-tree pipeline
line-log: allow non-patch diff formats with -L

 Documentation/line-range-options.adoc         |  10 +-
 line-log.c                                    |  30 ++----
 line-log.h                                    |   2 +-
 log-tree.c                                    |  10 +-
 revision.c                                    |  24 +++--
 t/t4211-line-log.sh                           | 100 +++++++++++++++---
 t/t4211/sha1/expect.parallel-change-f-to-main |   1 -
 .../sha256/expect.parallel-change-f-to-main   |   1 -
 8 files changed, 121 insertions(+), 57 deletions(-)

base-commit: 9f223ef1c026d91c7ac68cc0211bde255dda6199
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2094%2Fmmontalbo%2Fmm%2Fline-log-use-log-tree-diff-flush-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2094/mmontalbo/mm/line-log-use-log-tree-diff-flush-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2094

Range-diff vs v1:

 1:  9633eb62c6 = 1:  9633eb62c6 revision: move -L setup before output_format-to-diff derivation
 2:  2d9e0ca015 ! 2:  7acfc5376e line-log: integrate -L output with the standard log-tree pipeline
     @@ Commit message
             log_tree_diff_flush(), mirroring the diff_tree_oid() + flush
             pattern used by the single-parent and merge codepaths.
      
     -     - Remove the early return in log_tree_commit() that bypassed
     -       no_free save/restore, always_show_header, and diff_free().
     +     - Remove the early return in log_tree_commit() that is no longer
     +       needed now that -L output flows through log_tree_diff() and
     +       log_tree_diff_flush(); this restores no_free save/restore,
     +       always_show_header, and diff_free() cleanup.
      
          Because show_log() is now deferred until after diffcore_std() inside
          log_tree_diff_flush(), pickaxe (-S, -G, --find-object) and
     @@ Commit message
          log_tree_diff_flush() only emits one for verbose headers.  This
          matches the rest of log output.
      
     -    Also reject --full-diff, which is meaningless with -L: the filepairs
     -    are pre-computed during the history walk and scoped to tracked paths,
     -    so there is no tree diff to widen.
     +    Also reject --full-diff, which is not yet supported with -L: the
     +    filepairs are pre-computed during the history walk and scoped to
     +    tracked line ranges, so there is currently no full-tree diff to
     +    fall back to for display.
      
          Update tests accordingly.
      
     @@ log-tree.c: static int log_tree_diff(struct rev_info *opt, struct commit *commit
       
      +	if (opt->line_level_traverse) {
      +		line_log_queue_pairs(opt, commit);
     -+		return log_tree_diff_flush(opt);
     ++		log_tree_diff_flush(opt);
     ++		return !opt->loginfo;
      +	}
      +
       	parse_commit_or_die(commit);
     @@ log-tree.c: int log_tree_commit(struct rev_info *opt, struct commit *commit)
      
       ## revision.c ##
      @@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
     + 		die(_("the option '%s' requires '%s'"), "--grep-reflog", "--walk-reflogs");
     + 
       	if (revs->line_level_traverse &&
     - 	    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
     - 		die(_("-L does not yet support diff formats besides -p and -s"));
     -+	if (revs->line_level_traverse && revs->full_diff)
     -+		die(_("-L is not compatible with --full-diff"));
     +-	    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
     +-		die(_("-L does not yet support diff formats besides -p and -s"));
     ++	    (revs->full_diff ||
     ++	     (revs->diffopt.output_format &
     ++	      ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT))))
     ++		die(_("-L does not yet support the requested diff format"));
       
       	if (revs->expand_tabs_in_log < 0)
       		revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
     @@ t/t4211-line-log.sh: test_expect_success '-L with -G filters to diff-text matche
      +	test_cmp expect actual
      +'
      +
     -+test_expect_success '--full-diff is not supported with -L' '
     ++test_expect_success '--full-diff is not yet supported with -L' '
      +	test_must_fail git log -L1,24:b.c --full-diff 2>err &&
     -+	test_grep "not compatible with --full-diff" err
     ++	test_grep "does not yet support" err
      +'
      +
      +test_expect_success '-L --oneline has no extra blank line before diff' '
      +	git checkout parent-oids &&
      +	git log --oneline -L:func2:file.c -1 >actual &&
      +	# Oneline header on line 1, diff starts immediately on line 2
     -+	sed -n 2p actual | grep "^diff --git"
     ++	sed -n 2p actual >line2 &&
     ++	test_grep "^diff --git" line2
      +'
      +
       test_done
 3:  06c24b416f ! 3:  10a3d8dde2 line-log: allow non-patch diff formats with -L
     @@ Documentation/line-range-options.adoc
      
       ## revision.c ##
      @@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
     - 		die(_("the option '%s' requires '%s'"), "--grep-reflog", "--walk-reflogs");
     - 
       	if (revs->line_level_traverse &&
     --	    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
     --		die(_("-L does not yet support diff formats besides -p and -s"));
     -+	    (revs->diffopt.output_format &
     -+	     ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT |
     -+	       DIFF_FORMAT_RAW | DIFF_FORMAT_NAME |
     -+	       DIFF_FORMAT_NAME_STATUS | DIFF_FORMAT_SUMMARY)))
     -+		die(_("-L does not yet support the requested diff format"));
     - 	if (revs->line_level_traverse && revs->full_diff)
     - 		die(_("-L is not compatible with --full-diff"));
     + 	    (revs->full_diff ||
     + 	     (revs->diffopt.output_format &
     +-	      ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT))))
     ++	      ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT |
     ++		DIFF_FORMAT_RAW | DIFF_FORMAT_NAME |
     ++		DIFF_FORMAT_NAME_STATUS | DIFF_FORMAT_SUMMARY))))
     + 		die(_("-L does not yet support the requested diff format"));
       
     + 	if (revs->expand_tabs_in_log < 0)
      
       ## t/t4211-line-log.sh ##
      @@ t/t4211-line-log.sh: test_expect_success '-p shows the default patch output' '
     @@ t/t4211-line-log.sh: test_expect_success '-p shows the default patch output' '
      -	test_must_fail git log -L1,24:b.c --raw
      +test_expect_success '--raw shows mode, oid, status and path' '
      +	git log -L1,24:b.c --raw --format= >actual &&
     -+	grep "^:100644 100644 [0-9a-f]\{7\} [0-9a-f]\{7\} M	b.c$" actual &&
     -+	! grep "^diff --git" actual &&
     -+	! grep "^@@" actual
     ++	test_grep "^:100644 100644 [0-9a-f]\{7\} [0-9a-f]\{7\} M	b.c$" actual &&
     ++	! test_grep "^diff --git" actual &&
     ++	! test_grep "^@@" actual
      +'
      +
      +test_expect_success '--name-only shows path' '
      +	git log -L1,24:b.c --name-only --format= >actual &&
     -+	grep "^b.c$" actual &&
     -+	! grep "^diff --git" actual &&
     -+	! grep "^@@" actual
     ++	test_grep "^b.c$" actual &&
     ++	! test_grep "^diff --git" actual &&
     ++	! test_grep "^@@" actual
      +'
      +
      +test_expect_success '--name-status shows status and path' '
      +	git log -L1,24:b.c --name-status --format= >actual &&
     -+	grep "^M	b.c$" actual &&
     -+	! grep "^diff --git" actual &&
     -+	! grep "^@@" actual
     ++	test_grep "^M	b.c$" actual &&
     ++	! test_grep "^diff --git" actual &&
     ++	! test_grep "^@@" actual
      +'
      +
      +test_expect_success '--stat is not yet supported with -L' '
     @@ t/t4211-line-log.sh: test_expect_success '-p shows the default patch output' '
       
       test_expect_success 'setup for checking fancy rename following' '
      @@ t/t4211-line-log.sh: test_expect_success '-L --oneline has no extra blank line before diff' '
     - 	sed -n 2p actual | grep "^diff --git"
     + 	test_grep "^diff --git" line2
       '
       
      +test_expect_success '--summary shows new file on root commit' '
      +	git checkout parent-oids &&
      +	git log -L:func2:file.c --summary --format= >actual &&
     -+	grep "create mode 100644 file.c" actual
     ++	test_grep "create mode 100644 file.c" actual
      +'
      +
       test_done
Signature · 1 line
gitgitgadget
#9 lore raw

PATCH v2 1/3 revision: move -L setup before output_format-to-diff derivation

From: Michael Montalbo <[email protected]>

The line_level_traverse block sets a default DIFF_FORMAT_PATCH when no output format has been explicitly requested. This default must be visible to the "Did the user ask for any diff output?" check that derives revs->diff from revs->diffopt.output_format.

Currently the -L block runs after that derivation, so revs->diff stays 0 when no explicit format is given. This does not matter yet because log_tree_commit() short-circuits into line_log_print() before consulting revs->diff, but the next commit will route -L through the normal log_tree_diff() path, which checks revs->diff.

Move the block above the derivation so the default DIFF_FORMAT_PATCH is in place when revs->diff is computed. No behavior change on its own.

---
 revision.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
revision.c +8 -8
index 599b3a66c3..4a8e24bc38 100644
@@ -3112,6 +3112,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		object_context_release(&oc);
 	}
 
+	if (revs->line_level_traverse) {
+		if (want_ancestry(revs))
+			revs->limited = 1;
+		revs->topo_order = 1;
+		if (!revs->diffopt.output_format)
+			revs->diffopt.output_format = DIFF_FORMAT_PATCH;
+	}
+
 	/* Did the user ask for any diff output? Run the diff! */
 	if (revs->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT)
 		revs->diff = 1;
@@ -3125,14 +3133,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (revs->diffopt.objfind)
 		revs->simplify_history = 0;
 
-	if (revs->line_level_traverse) {
-		if (want_ancestry(revs))
-			revs->limited = 1;
-		revs->topo_order = 1;
-		if (!revs->diffopt.output_format)
-			revs->diffopt.output_format = DIFF_FORMAT_PATCH;
-	}
-
 	if (revs->topo_order && !generation_numbers_enabled(the_repository))
 		revs->limited = 1;
 
Signature · 1 line
gitgitgadget
#10 lore raw

PATCH v2 2/3 line-log: integrate -L output with the standard log-tree pipeline

In reply to Michael Montalbo via GitGitGadget

From: Michael Montalbo <[email protected]>

git log -L has bypassed log_tree_diff() and log_tree_diff_flush() since the feature was introduced, short-circuiting from log_tree_commit() directly into line_log_print(). This skips the no_free save/restore (noted in a NEEDSWORK comment added by f8781bfda3), the always_show_header fallback, show_diff_of_diff(), and diff_free() cleanup.

Restructure so that -L flows through log_tree_diff() -> log_tree_diff_flush(), the same path used by the normal single-parent and merge diff codepaths:

- Rename line_log_print() to line_log_queue_pairs() and strip it down to just queuing pre-computed filepairs. The show_log(), separator, diffcore_std(), and diff_flush() calls are removed since log_tree_diff_flush() handles all of those.

- In log_tree_diff(), call line_log_queue_pairs() then log_tree_diff_flush(), mirroring the diff_tree_oid() + flush pattern used by the single-parent and merge codepaths.

- Remove the early return in log_tree_commit() that is no longer needed now that -L output flows through log_tree_diff() and log_tree_diff_flush(); this restores no_free save/restore, always_show_header, and diff_free() cleanup.

Because show_log() is now deferred until after diffcore_std() inside log_tree_diff_flush(), pickaxe (-S, -G, --find-object) and --diff-filter now properly suppress commits when all pairs are filtered out.

The blank-line separator between commit header and diff changes
slightly: the old code printed one unconditionally, while log_tree_diff_flush() only emits one for verbose headers. This matches the rest of log output.

Also reject --full-diff, which is not yet supported with -L: the filepairs are pre-computed during the history walk and scoped to tracked line ranges, so there is currently no full-tree diff to fall back to for display.

Update tests accordingly.

---
 line-log.c                                    | 30 ++++-------
 line-log.h                                    |  2 +-
 log-tree.c                                    | 10 ++--
 revision.c                                    |  6 ++-
 t/t4211-line-log.sh                           | 53 ++++++++++++++-----
 t/t4211/sha1/expect.parallel-change-f-to-main |  1 -
 .../sha256/expect.parallel-change-f-to-main   |  1 -
 7 files changed, 60 insertions(+), 43 deletions(-)
line-log.c +9 -21
index 858a899cd2..7ee55b05cc 100644
@@ -13,7 +13,6 @@
 #include "revision.h"
 #include "xdiff-interface.h"
 #include "strbuf.h"
-#include "log-tree.h"
 #include "line-log.h"
 #include "setup.h"
 #include "strvec.h"
@@ -1004,29 +1003,18 @@ static int process_all_files(struct line_log_data **range_out,
 	return changed;
 }
 
-int line_log_print(struct rev_info *rev, struct commit *commit)
+void line_log_queue_pairs(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);
-		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);
-			}
-		}
+	struct line_log_data *range = lookup_line_range(rev, commit);
+	struct line_log_data *r;
 
-		diffcore_std(&rev->diffopt);
-		diff_flush(&rev->diffopt);
+	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);
+		}
 	}
-	return 1;
 }
 
 static int bloom_filter_check(struct rev_info *rev,
line-log.h +1 -1
index 04a6ea64d3..99e1755ce3 100644
@@ -46,7 +46,7 @@ int line_log_filter(struct rev_info *rev);
 int line_log_process_ranges_arbitrary_commit(struct rev_info *rev,
 						    struct commit *commit);
 
-int line_log_print(struct rev_info *rev, struct commit *commit);
+void line_log_queue_pairs(struct rev_info *rev, struct commit *commit);
 
 void line_log_free(struct rev_info *rev);
 
log-tree.c +6 -4
index 7e048701d0..88b3019293 100644
@@ -1105,6 +1105,12 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
 	if (!all_need_diff && !opt->merges_need_diff)
 		return 0;
 
+	if (opt->line_level_traverse) {
+		line_log_queue_pairs(opt, commit);
+		log_tree_diff_flush(opt);
+		return !opt->loginfo;
+	}
+
 	parse_commit_or_die(commit);
 	oid = get_commit_tree_oid(commit);
 
@@ -1179,10 +1185,6 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 	opt->loginfo = &log;
 	opt->diffopt.no_free = 1;
 
-	/* NEEDSWORK: no restoring of no_free?  Why? */
-	if (opt->line_level_traverse)
-		return line_log_print(opt, commit);
-
 	if (opt->track_linear && !opt->linear && !opt->reverse_output_stage)
 		fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
 	shown = log_tree_diff(opt, commit, &log);
revision.c +4 -2
index 4a8e24bc38..c903f7a1b4 100644
@@ -3179,8 +3179,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		die(_("the option '%s' requires '%s'"), "--grep-reflog", "--walk-reflogs");
 
 	if (revs->line_level_traverse &&
-	    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
-		die(_("-L does not yet support diff formats besides -p and -s"));
+	    (revs->full_diff ||
+	     (revs->diffopt.output_format &
+	      ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT))))
+		die(_("-L does not yet support the requested diff format"));
 
 	if (revs->expand_tabs_in_log < 0)
 		revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
t/t4211-line-log.sh +40 -13

95 lines not on this page · raw message

t/t4211/sha1/expect.parallel-change-f-to-main +0 -1
index 65a8cc673a..6d7a201036 100644
@@ -5,7 +5,6 @@ Date:   Fri Apr 12 16:16:24 2013 +0200
 
     Merge across the rename
 
-
 commit 6ce3c4ff690136099bb17e1a8766b75764726ea7
 Author: Thomas Rast <[email protected]>
 Date:   Thu Feb 28 10:49:50 2013 +0100
t/t4211/sha256/expect.parallel-change-f-to-main +0 -1
index 3178989253..c93e03bef4 100644
@@ -5,7 +5,6 @@ Date:   Fri Apr 12 16:16:24 2013 +0200
 
     Merge across the rename
 
-
 commit 4f7a58195a92c400e28a2354328587f1ff14fb77f5cf894536f17ccbc72931b9
 Author: Thomas Rast <[email protected]>
 Date:   Thu Feb 28 10:49:50 2013 +0100
Signature · 1 line
gitgitgadget
#11 lore raw

PATCH v2 3/3 line-log: allow non-patch diff formats with -L

In reply to Michael Montalbo via GitGitGadget

From: Michael Montalbo <[email protected]>

Now that -L flows through log_tree_diff_flush() and diff_flush(), metadata-only diff formats work because they only read filepair fields (status, mode, path, oid) already set on the pre-computed pairs.

Expand the allowlist in setup_revisions() to also accept --raw, --name-only, --name-status, and --summary. Diff stat formats (--stat, --numstat, --shortstat, --dirstat) remain blocked because they call compute_diffstat() on full blob content and would show whole-file statistics rather than range-scoped ones.

---
 Documentation/line-range-options.adoc | 10 +++---
 revision.c                            |  4 ++-
 t/t4211-line-log.sh                   | 47 +++++++++++++++++++++++++--
 3 files changed, 54 insertions(+), 7 deletions(-)
Documentation/line-range-options.adoc +6 -4
index ecb2c79fb9..72f639b5e7 100644
@@ -8,12 +8,14 @@
 	give zero or one positive revision arguments, and
 	_<start>_ and _<end>_ (or _<funcname>_) must exist in the starting revision.
 	You can specify this option more than once. Implies `--patch`.
-	Patch output can be suppressed using `--no-patch`, but other diff formats
-	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
-	`--name-only`, `--name-status`, `--check`) are not currently implemented.
+	Patch output can be suppressed using `--no-patch`.
+	Non-patch diff formats `--raw`, `--name-only`, `--name-status`,
+	and `--summary` are supported.  Diff stat formats
+	(`--stat`, `--numstat`, `--shortstat`, `--dirstat`) are not
+	currently implemented.
 +
 Patch formatting options such as `--word-diff`, `--color-moved`,
 `--no-prefix`, and whitespace options (`-w`, `-b`) are supported,
-as are pickaxe options (`-S`, `-G`).
+as are pickaxe options (`-S`, `-G`) and `--diff-filter`.
 +
 include::line-range-format.adoc[]
revision.c +3 -1
index c903f7a1b4..f26fc1f4d5 100644
@@ -3181,7 +3181,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (revs->line_level_traverse &&
 	    (revs->full_diff ||
 	     (revs->diffopt.output_format &
-	      ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT))))
+	      ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT |
+		DIFF_FORMAT_RAW | DIFF_FORMAT_NAME |
+		DIFF_FORMAT_NAME_STATUS | DIFF_FORMAT_SUMMARY))))
 		die(_("-L does not yet support the requested diff format"));
 
 	if (revs->expand_tabs_in_log < 0)
t/t4211-line-log.sh +45 -2

57 lines not on this page · raw message

Signature · 1 line
gitgitgadget
#12 lore raw
In reply to Michael Montalbo via GitGitGadget
Quoted 64 lines · Michael Montalbo via GitGitGadget

Since its introduction, git log -L has short-circuited from log_tree_commit() into its own output function, bypassing log_tree_diff() and log_tree_diff_flush(). This skips no_free save/restore, always_show_header, diff_free() cleanup, and means that pickaxe (-S, -G, --find-object) and --diff-filter cannot suppress commits whose pairs are all filtered out, because show_log() runs before diffcore_std().

This series restructures the flow so that -L goes through the same log_tree_diff() -> log_tree_diff_flush() path as normal single-parent and merge diffs, then uses that to enable several non-patch diff formats.

Patch 1: revision: move -L setup before output_format-to-diff derivation

Preparatory reorder in setup_revisions(). The -L block sets a default DIFF_FORMAT_PATCH when no format is requested; move it before the derivation of revs->diff from output_format so the default is visible to that check. No behavior change on its own.

Patch 2: line-log: integrate -L output with the standard log-tree pipeline

Rename line_log_print() to line_log_queue_pairs(), stripping it down to only queue pre-computed filepairs. log_tree_diff_flush() handles show_log(), diffcore_std(), and diff_flush(). This fixes pickaxe and --diff-filter suppression, and aligns the commit/diff separator with the rest of log output. Rejects --full-diff, which is not yet supported when filepairs are pre-computed.

Patch 3: line-log: allow non-patch diff formats with -L

Expand the allowlist to accept --raw, --name-only, --name-status, and --summary. These only read filepair metadata already set by the line-log machinery. Diff stat formats (--stat, --numstat, --shortstat, --dirstat) remain blocked because they call compute_diffstat() on full blob content and would show whole-file statistics rather than range-scoped ones.

Changes since v1:

* Patch 2: use !opt->loginfo return convention in log_tree_diff() to match the existing single-parent and merge codepaths, instead of returning log_tree_diff_flush() directly.
* Patch 2: reword the early-return removal to explicitly tie it to the pipeline change.
* Patch 2: soften --full-diff rejection to "not yet supported".
* Patches 2-3: use test_grep consistently in new tests.
* Patch 2: replace sed | grep pipe with sed > file && test_grep for proper exit status handling.

Michael Montalbo (3):
revision: move -L setup before output_format-to-diff derivation
line-log: integrate -L output with the standard log-tree pipeline
line-log: allow non-patch diff formats with -L

 Documentation/line-range-options.adoc         |  10 +-
 line-log.c                                    |  30 ++----
 line-log.h                                    |   2 +-
 log-tree.c                                    |  10 +-
 revision.c                                    |  24 +++--
 t/t4211-line-log.sh                           | 100 +++++++++++++++---
 t/t4211/sha1/expect.parallel-change-f-to-main |   1 -
 .../sha256/expect.parallel-change-f-to-main   |   1 -
 8 files changed, 121 insertions(+), 57 deletions(-)
Quoted 6 lines

base-commit: 9f223ef1c026d91c7ac68cc0211bde255dda6199
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2094%2Fmmontalbo%2Fmm%2Fline-log-use-log-tree-diff-flush-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2094/mmontalbo/mm/line-log-use-log-tree-diff-flush-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2094

Range-diff vs v1:

Looks good.

Quoted 108 lines
 1:  9633eb62c6 = 1:  9633eb62c6 revision: move -L setup before output_format-to-diff derivation
 2:  2d9e0ca015 ! 2:  7acfc5376e line-log: integrate -L output with the standard log-tree pipeline

@@ Commit message
log_tree_diff_flush(), mirroring the diff_tree_oid() + flush pattern used by the single-parent and merge codepaths.

- - Remove the early return in log_tree_commit() that bypassed
- no_free save/restore, always_show_header, and diff_free().
+ - Remove the early return in log_tree_commit() that is no longer
+ needed now that -L output flows through log_tree_diff() and
+ log_tree_diff_flush(); this restores no_free save/restore,
+ always_show_header, and diff_free() cleanup.

Because show_log() is now deferred until after diffcore_std() inside log_tree_diff_flush(), pickaxe (-S, -G, --find-object) and
@@ Commit message
log_tree_diff_flush() only emits one for verbose headers. This matches the rest of log output.

- Also reject --full-diff, which is meaningless with -L: the filepairs
- are pre-computed during the history walk and scoped to tracked paths,
- so there is no tree diff to widen.
+ Also reject --full-diff, which is not yet supported with -L: the
+ filepairs are pre-computed during the history walk and scoped to
+ tracked line ranges, so there is currently no full-tree diff to
+ fall back to for display.

Update tests accordingly.

@@ log-tree.c: static int log_tree_diff(struct rev_info *opt, struct commit *commit

+ if (opt->line_level_traverse) {
+ line_log_queue_pairs(opt, commit);
-+ return log_tree_diff_flush(opt);
++ log_tree_diff_flush(opt);
++ return !opt->loginfo;
+ }
+
parse_commit_or_die(commit);
@@ log-tree.c: int log_tree_commit(struct rev_info *opt, struct commit *commit)

## revision.c ##
@@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
+ die(_("the option '%s' requires '%s'"), "--grep-reflog", "--walk-reflogs");
+
if (revs->line_level_traverse &&
- (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
- die(_("-L does not yet support diff formats besides -p and -s"));
-+ if (revs->line_level_traverse && revs->full_diff)
-+ die(_("-L is not compatible with --full-diff"));
+- (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
+- die(_("-L does not yet support diff formats besides -p and -s"));
++ (revs->full_diff ||
++ (revs->diffopt.output_format &
++ ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT))))
++ die(_("-L does not yet support the requested diff format"));

if (revs->expand_tabs_in_log < 0)
revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
@@ t/t4211-line-log.sh: test_expect_success '-L with -G filters to diff-text matche
+ test_cmp expect actual
+'
+
-+test_expect_success '--full-diff is not supported with -L' ' ++test_expect_success '--full-diff is not yet supported with -L' '
+ test_must_fail git log -L1,24:b.c --full-diff 2>err &&
-+ test_grep "not compatible with --full-diff" err
++ test_grep "does not yet support" err
+'
+
+test_expect_success '-L --oneline has no extra blank line before diff' '
+ git checkout parent-oids &&
+ git log --oneline -L:func2:file.c -1 >actual &&
+ # Oneline header on line 1, diff starts immediately on line 2
-+ sed -n 2p actual | grep "^diff --git"
++ sed -n 2p actual >line2 &&
++ test_grep "^diff --git" line2
+'
+
test_done
3: 06c24b416f ! 3: 10a3d8dde2 line-log: allow non-patch diff formats with -L
@@ Documentation/line-range-options.adoc

## revision.c ##
@@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
- die(_("the option '%s' requires '%s'"), "--grep-reflog", "--walk-reflogs");
-
if (revs->line_level_traverse &&
-- (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
-- die(_("-L does not yet support diff formats besides -p and -s"));
-+ (revs->diffopt.output_format &
-+ ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT |
-+ DIFF_FORMAT_RAW | DIFF_FORMAT_NAME |
-+ DIFF_FORMAT_NAME_STATUS | DIFF_FORMAT_SUMMARY)))
-+ die(_("-L does not yet support the requested diff format"));
- if (revs->line_level_traverse && revs->full_diff)
- die(_("-L is not compatible with --full-diff"));
+ (revs->full_diff ||
+ (revs->diffopt.output_format &
+- ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT))))
++ ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT |
++ DIFF_FORMAT_RAW | DIFF_FORMAT_NAME |
++ DIFF_FORMAT_NAME_STATUS | DIFF_FORMAT_SUMMARY))))
+ die(_("-L does not yet support the requested diff format"));

+ if (revs->expand_tabs_in_log < 0)

Quoted 12 lines

## t/t4211-line-log.sh ##
@@ t/t4211-line-log.sh: test_expect_success '-p shows the default patch output' '
@@ t/t4211-line-log.sh: test_expect_success '-p shows the default patch output' '
- test_must_fail git log -L1,24:b.c --raw
+test_expect_success '--raw shows mode, oid, status and path' '
+ git log -L1,24:b.c --raw --format= >actual &&
-+ grep "^:100644 100644 [0-9a-f]\{7\} [0-9a-f]\{7\} M b.c$" actual &&
-+ ! grep "^diff --git" actual &&
-+ ! grep "^@@" actual
++ test_grep "^:100644 100644 [0-9a-f]\{7\} [0-9a-f]\{7\} M b.c$" actual &&
++ ! test_grep "^diff --git" actual &&
++ ! test_grep "^@@" actual

I wish we had docs for all the little test helpers… in particular, I think this is supposed to be "test_grep !" ?

Quoted 40 lines
      +'
      +
      +test_expect_success '--name-only shows path' '
      + git log -L1,24:b.c --name-only --format= >actual &&
     -+ grep "^b.c$" actual &&
     -+ ! grep "^diff --git" actual &&
     -+ ! grep "^@@" actual
     ++ test_grep "^b.c$" actual &&
     ++ ! test_grep "^diff --git" actual &&
     ++ ! test_grep "^@@" actual
      +'
      +
      +test_expect_success '--name-status shows status and path' '
      + git log -L1,24:b.c --name-status --format= >actual &&
     -+ grep "^M        b.c$" actual &&
     -+ ! grep "^diff --git" actual &&
     -+ ! grep "^@@" actual
     ++ test_grep "^M   b.c$" actual &&
     ++ ! test_grep "^diff --git" actual &&
     ++ ! test_grep "^@@" actual
      +'
      +
      +test_expect_success '--stat is not yet supported with -L' '
     @@ t/t4211-line-log.sh: test_expect_success '-p shows the default patch output' '

       test_expect_success 'setup for checking fancy rename following' '
      @@ t/t4211-line-log.sh: test_expect_success '-L --oneline has no extra blank line before diff' '
     -  sed -n 2p actual | grep "^diff --git"
     +  test_grep "^diff --git" line2
       '

      +test_expect_success '--summary shows new file on root commit' '
      + git checkout parent-oids &&
      + git log -L:func2:file.c --summary --format= >actual &&
     -+ grep "create mode 100644 file.c" actual
     ++ test_grep "create mode 100644 file.c" actual
      +'
      +
       test_done
Quoted 2 lines

--
gitgitgadget

Thanks

Signature · 1 line
D. Ben Knoble
#13 lore raw
Quoted 5 lines · D. Ben Knoble
Quoted 2 lines
     ++ ! test_grep "^diff --git" actual &&
     ++ ! test_grep "^@@" actual

I wish we had docs for all the little test helpers… in particular, I think this is supposed to be "test_grep !" ?

Good eyes. I wonder if we teach test-lint to catch these.

#14 lore raw
Quoted 8 lines · Junio C Hamano
Quoted 5 lines · D. Ben Knoble
Quoted 2 lines
     ++ ! test_grep "^diff --git" actual &&
     ++ ! test_grep "^@@" actual

I wish we had docs for all the little test helpers… in particular, I think this is supposed to be "test_grep !" ?

Good catch, thank you! Will submit a fix and try to address the lack of docs in a follow-up.

Quoted 2 lines

Good eyes. I wonder if we teach test-lint to catch these.

I had the same thought, and started working on a series that does this and migrates
existing offenders. I should be able to submit it soon.

#15 lore raw

v3 cover line-log: integrate -L with the standard log output pipeline

In reply to Michael Montalbo via GitGitGadget

Since its introduction, git log -L has short-circuited from log_tree_commit() into its own output function, bypassing log_tree_diff() and log_tree_diff_flush(). This skips no_free save/restore, always_show_header, diff_free() cleanup, and means that pickaxe (-S, -G, --find-object) and --diff-filter cannot suppress commits whose pairs are all filtered out, because show_log() runs before diffcore_std().

This series restructures the flow so that -L goes through the same log_tree_diff() -> log_tree_diff_flush() path as normal single-parent and merge diffs, then uses that to enable several non-patch diff formats.

Patch 1: revision: move -L setup before output_format-to-diff derivation

Preparatory reorder in setup_revisions(). The -L block sets a default DIFF_FORMAT_PATCH when no format is requested; move it before the derivation of revs->diff from output_format so the default is visible to that check. No behavior change on its own.

Patch 2: line-log: integrate -L output with the standard log-tree pipeline

Rename line_log_print() to line_log_queue_pairs(), stripping it down to only queue pre-computed filepairs. log_tree_diff_flush() handles show_log(), diffcore_std(), and diff_flush(). This fixes pickaxe and --diff-filter suppression, and aligns the commit/diff separator with the rest of log output. Rejects --full-diff, which is not yet supported when filepairs are pre-computed.

Patch 3: line-log: allow non-patch diff formats with -L

Expand the allowlist to accept --raw, --name-only, --name-status, and --summary. These only read filepair metadata already set by the line-log machinery. Diff stat formats (--stat, --numstat, --shortstat, --dirstat) remain blocked because they call compute_diffstat() on full blob content and would show whole-file statistics rather than range-scoped ones.

Changes since v2:

* Switch "! test_grep" to "test_grep !" in tests.

Michael Montalbo (3):
revision: move -L setup before output_format-to-diff derivation
line-log: integrate -L output with the standard log-tree pipeline
line-log: allow non-patch diff formats with -L

 Documentation/line-range-options.adoc         |  10 +-
 line-log.c                                    |  30 ++----
 line-log.h                                    |   2 +-
 log-tree.c                                    |  10 +-
 revision.c                                    |  24 +++--
 t/t4211-line-log.sh                           | 100 +++++++++++++++---
 t/t4211/sha1/expect.parallel-change-f-to-main |   1 -
 .../sha256/expect.parallel-change-f-to-main   |   1 -
 8 files changed, 121 insertions(+), 57 deletions(-)

base-commit: 9f223ef1c026d91c7ac68cc0211bde255dda6199
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2094%2Fmmontalbo%2Fmm%2Fline-log-use-log-tree-diff-flush-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2094/mmontalbo/mm/line-log-use-log-tree-diff-flush-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2094

Range-diff vs v2:

 1:  9633eb62c6 = 1:  9633eb62c6 revision: move -L setup before output_format-to-diff derivation
 2:  7acfc5376e = 2:  7acfc5376e line-log: integrate -L output with the standard log-tree pipeline
 3:  10a3d8dde2 ! 3:  ae0b7f3ca8 line-log: allow non-patch diff formats with -L
     @@ t/t4211-line-log.sh: test_expect_success '-p shows the default patch output' '
      +test_expect_success '--raw shows mode, oid, status and path' '
      +	git log -L1,24:b.c --raw --format= >actual &&
      +	test_grep "^:100644 100644 [0-9a-f]\{7\} [0-9a-f]\{7\} M	b.c$" actual &&
     -+	! test_grep "^diff --git" actual &&
     -+	! test_grep "^@@" actual
     ++	test_grep ! "^diff --git" actual &&
     ++	test_grep ! "^@@" actual
      +'
      +
      +test_expect_success '--name-only shows path' '
      +	git log -L1,24:b.c --name-only --format= >actual &&
      +	test_grep "^b.c$" actual &&
     -+	! test_grep "^diff --git" actual &&
     -+	! test_grep "^@@" actual
     ++	test_grep ! "^diff --git" actual &&
     ++	test_grep ! "^@@" actual
      +'
      +
      +test_expect_success '--name-status shows status and path' '
      +	git log -L1,24:b.c --name-status --format= >actual &&
      +	test_grep "^M	b.c$" actual &&
     -+	! test_grep "^diff --git" actual &&
     -+	! test_grep "^@@" actual
     ++	test_grep ! "^diff --git" actual &&
     ++	test_grep ! "^@@" actual
      +'
      +
      +test_expect_success '--stat is not yet supported with -L' '
Signature · 1 line
gitgitgadget
#16 lore raw

PATCH v3 1/3 revision: move -L setup before output_format-to-diff derivation

From: Michael Montalbo <[email protected]>

The line_level_traverse block sets a default DIFF_FORMAT_PATCH when no output format has been explicitly requested. This default must be visible to the "Did the user ask for any diff output?" check that derives revs->diff from revs->diffopt.output_format.

Currently the -L block runs after that derivation, so revs->diff stays 0 when no explicit format is given. This does not matter yet because log_tree_commit() short-circuits into line_log_print() before consulting revs->diff, but the next commit will route -L through the normal log_tree_diff() path, which checks revs->diff.

Move the block above the derivation so the default DIFF_FORMAT_PATCH is in place when revs->diff is computed. No behavior change on its own.

---
 revision.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
revision.c +8 -8
index 599b3a66c3..4a8e24bc38 100644
@@ -3112,6 +3112,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		object_context_release(&oc);
 	}
 
+	if (revs->line_level_traverse) {
+		if (want_ancestry(revs))
+			revs->limited = 1;
+		revs->topo_order = 1;
+		if (!revs->diffopt.output_format)
+			revs->diffopt.output_format = DIFF_FORMAT_PATCH;
+	}
+
 	/* Did the user ask for any diff output? Run the diff! */
 	if (revs->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT)
 		revs->diff = 1;
@@ -3125,14 +3133,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (revs->diffopt.objfind)
 		revs->simplify_history = 0;
 
-	if (revs->line_level_traverse) {
-		if (want_ancestry(revs))
-			revs->limited = 1;
-		revs->topo_order = 1;
-		if (!revs->diffopt.output_format)
-			revs->diffopt.output_format = DIFF_FORMAT_PATCH;
-	}
-
 	if (revs->topo_order && !generation_numbers_enabled(the_repository))
 		revs->limited = 1;
 
Signature · 1 line
gitgitgadget
#17 lore raw

PATCH v3 2/3 line-log: integrate -L output with the standard log-tree pipeline

In reply to Michael Montalbo via GitGitGadget

From: Michael Montalbo <[email protected]>

git log -L has bypassed log_tree_diff() and log_tree_diff_flush() since the feature was introduced, short-circuiting from log_tree_commit() directly into line_log_print(). This skips the no_free save/restore (noted in a NEEDSWORK comment added by f8781bfda3), the always_show_header fallback, show_diff_of_diff(), and diff_free() cleanup.

Restructure so that -L flows through log_tree_diff() -> log_tree_diff_flush(), the same path used by the normal single-parent and merge diff codepaths:

- Rename line_log_print() to line_log_queue_pairs() and strip it down to just queuing pre-computed filepairs. The show_log(), separator, diffcore_std(), and diff_flush() calls are removed since log_tree_diff_flush() handles all of those.

- In log_tree_diff(), call line_log_queue_pairs() then log_tree_diff_flush(), mirroring the diff_tree_oid() + flush pattern used by the single-parent and merge codepaths.

- Remove the early return in log_tree_commit() that is no longer needed now that -L output flows through log_tree_diff() and log_tree_diff_flush(); this restores no_free save/restore, always_show_header, and diff_free() cleanup.

Because show_log() is now deferred until after diffcore_std() inside log_tree_diff_flush(), pickaxe (-S, -G, --find-object) and --diff-filter now properly suppress commits when all pairs are filtered out.

The blank-line separator between commit header and diff changes
slightly: the old code printed one unconditionally, while log_tree_diff_flush() only emits one for verbose headers. This matches the rest of log output.

Also reject --full-diff, which is not yet supported with -L: the filepairs are pre-computed during the history walk and scoped to tracked line ranges, so there is currently no full-tree diff to fall back to for display.

Update tests accordingly.

---
 line-log.c                                    | 30 ++++-------
 line-log.h                                    |  2 +-
 log-tree.c                                    | 10 ++--
 revision.c                                    |  6 ++-
 t/t4211-line-log.sh                           | 53 ++++++++++++++-----
 t/t4211/sha1/expect.parallel-change-f-to-main |  1 -
 .../sha256/expect.parallel-change-f-to-main   |  1 -
 7 files changed, 60 insertions(+), 43 deletions(-)
line-log.c +9 -21
index 858a899cd2..7ee55b05cc 100644
@@ -13,7 +13,6 @@
 #include "revision.h"
 #include "xdiff-interface.h"
 #include "strbuf.h"
-#include "log-tree.h"
 #include "line-log.h"
 #include "setup.h"
 #include "strvec.h"
@@ -1004,29 +1003,18 @@ static int process_all_files(struct line_log_data **range_out,
 	return changed;
 }
 
-int line_log_print(struct rev_info *rev, struct commit *commit)
+void line_log_queue_pairs(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);
-		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);
-			}
-		}
+	struct line_log_data *range = lookup_line_range(rev, commit);
+	struct line_log_data *r;
 
-		diffcore_std(&rev->diffopt);
-		diff_flush(&rev->diffopt);
+	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);
+		}
 	}
-	return 1;
 }
 
 static int bloom_filter_check(struct rev_info *rev,
line-log.h +1 -1
index 04a6ea64d3..99e1755ce3 100644
@@ -46,7 +46,7 @@ int line_log_filter(struct rev_info *rev);
 int line_log_process_ranges_arbitrary_commit(struct rev_info *rev,
 						    struct commit *commit);
 
-int line_log_print(struct rev_info *rev, struct commit *commit);
+void line_log_queue_pairs(struct rev_info *rev, struct commit *commit);
 
 void line_log_free(struct rev_info *rev);
 
log-tree.c +6 -4
index 7e048701d0..88b3019293 100644
@@ -1105,6 +1105,12 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
 	if (!all_need_diff && !opt->merges_need_diff)
 		return 0;
 
+	if (opt->line_level_traverse) {
+		line_log_queue_pairs(opt, commit);
+		log_tree_diff_flush(opt);
+		return !opt->loginfo;
+	}
+
 	parse_commit_or_die(commit);
 	oid = get_commit_tree_oid(commit);
 
@@ -1179,10 +1185,6 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 	opt->loginfo = &log;
 	opt->diffopt.no_free = 1;
 
-	/* NEEDSWORK: no restoring of no_free?  Why? */
-	if (opt->line_level_traverse)
-		return line_log_print(opt, commit);
-
 	if (opt->track_linear && !opt->linear && !opt->reverse_output_stage)
 		fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
 	shown = log_tree_diff(opt, commit, &log);
revision.c +4 -2
index 4a8e24bc38..c903f7a1b4 100644
@@ -3179,8 +3179,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		die(_("the option '%s' requires '%s'"), "--grep-reflog", "--walk-reflogs");
 
 	if (revs->line_level_traverse &&
-	    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
-		die(_("-L does not yet support diff formats besides -p and -s"));
+	    (revs->full_diff ||
+	     (revs->diffopt.output_format &
+	      ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT))))
+		die(_("-L does not yet support the requested diff format"));
 
 	if (revs->expand_tabs_in_log < 0)
 		revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
t/t4211-line-log.sh +40 -13

95 lines not on this page · raw message

t/t4211/sha1/expect.parallel-change-f-to-main +0 -1
index 65a8cc673a..6d7a201036 100644
@@ -5,7 +5,6 @@ Date:   Fri Apr 12 16:16:24 2013 +0200
 
     Merge across the rename
 
-
 commit 6ce3c4ff690136099bb17e1a8766b75764726ea7
 Author: Thomas Rast <[email protected]>
 Date:   Thu Feb 28 10:49:50 2013 +0100
t/t4211/sha256/expect.parallel-change-f-to-main +0 -1
index 3178989253..c93e03bef4 100644
@@ -5,7 +5,6 @@ Date:   Fri Apr 12 16:16:24 2013 +0200
 
     Merge across the rename
 
-
 commit 4f7a58195a92c400e28a2354328587f1ff14fb77f5cf894536f17ccbc72931b9
 Author: Thomas Rast <[email protected]>
 Date:   Thu Feb 28 10:49:50 2013 +0100
Signature · 1 line
gitgitgadget
#18 lore raw

PATCH v3 3/3 line-log: allow non-patch diff formats with -L

In reply to Michael Montalbo via GitGitGadget

From: Michael Montalbo <[email protected]>

Now that -L flows through log_tree_diff_flush() and diff_flush(), metadata-only diff formats work because they only read filepair fields (status, mode, path, oid) already set on the pre-computed pairs.

Expand the allowlist in setup_revisions() to also accept --raw, --name-only, --name-status, and --summary. Diff stat formats (--stat, --numstat, --shortstat, --dirstat) remain blocked because they call compute_diffstat() on full blob content and would show whole-file statistics rather than range-scoped ones.

---
 Documentation/line-range-options.adoc | 10 +++---
 revision.c                            |  4 ++-
 t/t4211-line-log.sh                   | 47 +++++++++++++++++++++++++--
 3 files changed, 54 insertions(+), 7 deletions(-)
Documentation/line-range-options.adoc +6 -4
index ecb2c79fb9..72f639b5e7 100644
@@ -8,12 +8,14 @@
 	give zero or one positive revision arguments, and
 	_<start>_ and _<end>_ (or _<funcname>_) must exist in the starting revision.
 	You can specify this option more than once. Implies `--patch`.
-	Patch output can be suppressed using `--no-patch`, but other diff formats
-	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
-	`--name-only`, `--name-status`, `--check`) are not currently implemented.
+	Patch output can be suppressed using `--no-patch`.
+	Non-patch diff formats `--raw`, `--name-only`, `--name-status`,
+	and `--summary` are supported.  Diff stat formats
+	(`--stat`, `--numstat`, `--shortstat`, `--dirstat`) are not
+	currently implemented.
 +
 Patch formatting options such as `--word-diff`, `--color-moved`,
 `--no-prefix`, and whitespace options (`-w`, `-b`) are supported,
-as are pickaxe options (`-S`, `-G`).
+as are pickaxe options (`-S`, `-G`) and `--diff-filter`.
 +
 include::line-range-format.adoc[]
revision.c +3 -1
index c903f7a1b4..f26fc1f4d5 100644
@@ -3181,7 +3181,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (revs->line_level_traverse &&
 	    (revs->full_diff ||
 	     (revs->diffopt.output_format &
-	      ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT))))
+	      ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT |
+		DIFF_FORMAT_RAW | DIFF_FORMAT_NAME |
+		DIFF_FORMAT_NAME_STATUS | DIFF_FORMAT_SUMMARY))))
 		die(_("-L does not yet support the requested diff format"));
 
 	if (revs->expand_tabs_in_log < 0)
t/t4211-line-log.sh +45 -2

57 lines not on this page · raw message

Signature · 1 line
gitgitgadget
#19 lore raw
In reply to Michael Montalbo via GitGitGadget
Quoted 38 lines · Michael Montalbo via GitGitGadget

Since its introduction, git log -L has short-circuited from log_tree_commit() into its own output function, bypassing log_tree_diff() and log_tree_diff_flush(). This skips no_free save/restore, always_show_header, diff_free() cleanup, and means that pickaxe (-S, -G, --find-object) and --diff-filter cannot suppress commits whose pairs are all filtered out, because show_log() runs before diffcore_std().

This series restructures the flow so that -L goes through the same log_tree_diff() -> log_tree_diff_flush() path as normal single-parent and merge diffs, then uses that to enable several non-patch diff formats.

Patch 1: revision: move -L setup before output_format-to-diff derivation

Preparatory reorder in setup_revisions(). The -L block sets a default DIFF_FORMAT_PATCH when no format is requested; move it before the derivation of revs->diff from output_format so the default is visible to that check. No behavior change on its own.

Patch 2: line-log: integrate -L output with the standard log-tree pipeline

Rename line_log_print() to line_log_queue_pairs(), stripping it down to only queue pre-computed filepairs. log_tree_diff_flush() handles show_log(), diffcore_std(), and diff_flush(). This fixes pickaxe and --diff-filter suppression, and aligns the commit/diff separator with the rest of log output. Rejects --full-diff, which is not yet supported when filepairs are pre-computed.

Patch 3: line-log: allow non-patch diff formats with -L

Expand the allowlist to accept --raw, --name-only, --name-status, and --summary. These only read filepair metadata already set by the line-log machinery. Diff stat formats (--stat, --numstat, --shortstat, --dirstat) remain blocked because they call compute_diffstat() on full blob content and would show whole-file statistics rather than range-scoped ones.

Quoted 3 lines

Changes since v2:

* Switch "! test_grep" to "test_grep !" in tests.

Thanks ! I did not read the tests carefully for semantic value, but the rationale and overall code looks good to me as discussed previously.

The range-diff here looks good, too.

Quoted 58 lines

Michael Montalbo (3):
revision: move -L setup before output_format-to-diff derivation
line-log: integrate -L output with the standard log-tree pipeline
line-log: allow non-patch diff formats with -L

Documentation/line-range-options.adoc         |  10 +-
line-log.c                                    |  30 ++----
line-log.h                                    |   2 +-
log-tree.c                                    |  10 +-
revision.c                                    |  24 +++--
t/t4211-line-log.sh                           | 100 +++++++++++++++---
t/t4211/sha1/expect.parallel-change-f-to-main |   1 -
.../sha256/expect.parallel-change-f-to-main   |   1 -
8 files changed, 121 insertions(+), 57 deletions(-)

base-commit: 9f223ef1c026d91c7ac68cc0211bde255dda6199
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2094%2Fmmontalbo%2Fmm%2Fline-log-use-log-tree-diff-flush-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2094/mmontalbo/mm/line-log-use-log-tree-diff-flush-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2094

Range-diff vs v2:

1:  9633eb62c6 = 1:  9633eb62c6 revision: move -L setup before output_format-to-diff derivation
2:  7acfc5376e = 2:  7acfc5376e line-log: integrate -L output with the standard log-tree pipeline
3:  10a3d8dde2 ! 3:  ae0b7f3ca8 line-log: allow non-patch diff formats with -L
    @@ t/t4211-line-log.sh: test_expect_success '-p shows the default patch output' '
     +test_expect_success '--raw shows mode, oid, status and path' '
     +    git log -L1,24:b.c --raw --format= >actual &&
     +    test_grep "^:100644 100644 [0-9a-f]\{7\} [0-9a-f]\{7\} M    b.c$" actual &&
    -+    ! test_grep "^diff --git" actual &&
    -+    ! test_grep "^@@" actual
    ++    test_grep ! "^diff --git" actual &&
    ++    test_grep ! "^@@" actual
     +'
     +
     +test_expect_success '--name-only shows path' '
     +    git log -L1,24:b.c --name-only --format= >actual &&
     +    test_grep "^b.c$" actual &&
    -+    ! test_grep "^diff --git" actual &&
    -+    ! test_grep "^@@" actual
    ++    test_grep ! "^diff --git" actual &&
    ++    test_grep ! "^@@" actual
     +'
     +
     +test_expect_success '--name-status shows status and path' '
     +    git log -L1,24:b.c --name-status --format= >actual &&
     +    test_grep "^M    b.c$" actual &&
    -+    ! test_grep "^diff --git" actual &&
    -+    ! test_grep "^@@" actual
    ++    test_grep ! "^diff --git" actual &&
    ++    test_grep ! "^@@" actual
     +'
     +
     +test_expect_success '--stat is not yet supported with -L' '

--
gitgitgadget

#20 lore raw
Quoted 9 lines · Ben Knoble
Quoted 3 lines

Changes since v2:

* Switch "! test_grep" to "test_grep !" in tests.

Thanks ! I did not read the tests carefully for semantic value, but the rationale and overall code looks good to me as discussed previously.

The range-diff here looks good, too.

Thanks, both. Let's mark it for 'next' then.