Re: [PATCH v3 2/3] builtin: introduce diff-pairs command

On 25/02/26 10:24AM, Junio C Hamano wrote:

Justin Tobler [off-list ref] writes:

quoted

+static void flush_diff_queue(struct diff_options *options)
+{
+	/*
+	 * If rename detection is not requested, use rename information from the
+	 * raw diff formatted input. Setting found_follow ensures diffcore_std()
+	 * does not mess with rename information already present in queued
+	 * filepairs.
+	 */
+	if (!options->detect_rename)
+		options->found_follow = 1;

An ugly design decision that may be suboptimal from maintainability
point of view.

The parts of diffcore_std() that --follow wants to bypass may happen
to be the same as the parts that this new caller wants to bypass,
but who guarantees that they will stay that way in the future?

Good point. When invoking diffcore_std(), we really just need to be able
to skip diff_resolve_rename_copy() as that is what is updating the diff
filepair statuses. In the next version, instead of relying on
`found_follow`, I think I'll introduce a new diff_options field,
`skip_resolving_statuses` for this specific purpose.

quoted

+	while (1) {
+		struct object_id oid_a, oid_b;
+		struct diff_filepair *pair;
+		unsigned mode_a, mode_b;
+		const char *p;
+		char status;
+
+		if (strbuf_getline_nul(&meta, stdin) == EOF)
+			break;

There should be a variant of this function that takes delimiter
parameter.  By declaring an int variable that is initialized to '\0'
(because you only deal with "-z" input) and passing that delimiter
variable to strbuf_getwholeline() would future-proof this code path.

How builtin/update-ref.c:update_refs_stdin() works may be inspiring.

Makes sense, I'll swap to using strbuf_getwholeline() with a defined
line terminator variable in the next version. This way it can help make
supporting the "normal" raw diff format as input easier in the future.

quoted

+test_expect_success 'diff-pairs recreates --raw' '
+	git diff-tree -r -M -C -C -z base new >expect &&
+	git diff-tree -r -M -C -C -z base new |
+	git diff-pairs --raw -z >actual &&
+	test_cmp expect actual
+'

Amusing ;-)  But a very obvious and important thing to test.
I would have fed <expect to diff-pairs for this test, though.

Will adjust in the next version.

Other than that, nicely done.

Thanks for the review!
-Justin