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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 26, 2025, 18:24 UTC
Message-ID: <xmqqjz9cd0nl.fsf@gitster.g>
In-Reply-To: <20250225233925.1345086-3-jltobler@gmail.com>

Justin Tobler <jltobler@gmail.com> writes:

Show 10 quoted lines

> +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?

Show 36 quoted lines

> +	diffcore_std(options);
> +	diff_flush(options);
> +}
> +
> +int cmd_diff_pairs(int argc, const char **argv, const char *prefix,
> +		   struct repository *repo)
> +{
> +	struct strbuf path_dst = STRBUF_INIT;
> +	struct strbuf path = STRBUF_INIT;
> +	struct strbuf meta = STRBUF_INIT;
> +	struct rev_info revs;
> +	int ret;
> +
> +	const char * const usage[] = {
> +		N_("git diff-pairs -z [<diff-options>]"),
> +		NULL
> +	};
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	struct option *parseopts = add_diff_options(options, &revs.diffopt);
> +
> +	show_usage_with_options_if_asked(argc, argv, usage, parseopts);
> +
> +	repo_init_revisions(repo, &revs, prefix);
> +	repo_config(repo, git_diff_basic_config, NULL);
> +	revs.disable_stdin = 1;
> +	revs.abbrev = 0;
> +	revs.diff = 1;
> +
> +	if (setup_revisions(argc, argv, &revs, NULL) > 1)
> +		usage_with_options(usage, parseopts);
> +
> +	/*
> +	 * With the -z option, both command input and raw output are
> +	 * NUL-delimited (this mode does not effect patch output). At present

Probably "effect" -> "affect".

Show 31 quoted lines

> +	 * only NUL-delimited raw diff formatted input is supported.
> +	 */
> +	if (revs.diffopt.line_termination) {
> +		error(_("working without -z is not supported"));
> +		usage_with_options(usage, parseopts);
> +	}
> +
> +	if (revs.prune_data.nr) {
> +		error(_("pathspec arguments not supported"));
> +		usage_with_options(usage, parseopts);
> +	}
> +
> +	if (revs.pending.nr || revs.max_count != -1 ||
> +	    revs.min_age != (timestamp_t)-1 ||
> +	    revs.max_age != (timestamp_t)-1) {
> +		error(_("revision arguments not allowed"));
> +		usage_with_options(usage, parseopts);
> +	}
> +
> +	if (!revs.diffopt.output_format)
> +		revs.diffopt.output_format = DIFF_FORMAT_PATCH;
> +
> +	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.

Show 13 quoted lines

> +		switch (status) {
> +		case DIFF_STATUS_ADDED:
> +			pair = diff_queue_addremove(&diff_queued_diff,
> +						    &revs.diffopt, '+', mode_b,
> +						    &oid_b, 1, path.buf, 0);
> +			if (pair)
> +				pair->status = status;
> +			break;
> + ...
> +		default:
> +			die(_("unknown diff status: %c"), status);
> +		}
> +	}

Amusing; looking good.

Show 11 quoted lines

> +	flush_diff_queue(&revs.diffopt);
> +	ret = diff_result_code(&revs);
> +
> +	strbuf_release(&path_dst);
> +	strbuf_release(&path);
> +	strbuf_release(&meta);
> +	release_revisions(&revs);
> +	FREE_AND_NULL(parseopts);
> +
> +	return ret;
> +}

Nice. It is surprisingly compact and had everything I expected it to have ;-).

Show 6 quoted lines

> +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.

Other than that, nicely done.