Re: [PATCH v4 3/4] builtin: introduce diff-pairs command

[Patrick Steinhardt <ps@pks.im>]

On Thu, Feb 27, 2025 at 06:26:03PM -0600, Justin Tobler wrote:
> diff --git a/builtin/diff-pairs.c b/builtin/diff-pairs.c
> new file mode 100644
> index 0000000000..5a993b7c9d
> --- /dev/null
> +++ b/builtin/diff-pairs.c
[snip]
> +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 option *parseopts;
> +	struct rev_info revs;
> +	int line_term = '\0';
> +	int ret;
> +
> +	const char * const usagestr[] = {
> +		N_("git diff-pairs -z [<diff-options>]"),
> +		NULL
> +	};

We tend to call these `builtin_*_usage`, so in your case it would be
`builtin_diff_pairs_usage`.

> +	struct option options[] = {
> +		OPT_END()
> +	};
> +
> +	repo_init_revisions(repo, &revs, prefix);
> +
> +	/*
> +	 * Diff options are usually parsed implicitly as part of
> +	 * setup_revisions(). Explicitly handle parsing to ensure options are
> +	 * printed in the usage message.
> +	 */
> +	parseopts = add_diff_options(options, &revs.diffopt);
> +	show_usage_with_options_if_asked(argc, argv, usagestr, parseopts);
> +
> +	repo_config(repo, git_diff_basic_config, NULL);
> +	revs.disable_stdin = 1;
> +	revs.abbrev = 0;
> +	revs.diff = 1;
> +
> +	argc = parse_options(argc, argv, prefix, parseopts, usagestr,
> +			     PARSE_OPT_KEEP_UNKNOWN_OPT |
> +			     PARSE_OPT_KEEP_DASHDASH |
> +			     PARSE_OPT_KEEP_ARGV0);
> 
> +	if (setup_revisions(argc, argv, &revs, NULL) > 1)
> +		usagef(_("unrecognized argument: %s"), argv[0]);

Okay, we now use `parse_options()` to parse stuff for us, and
`setup_revisions()` only really does the setup for us as we know that
all relevant diff options should've already been parsed for us. This
looks much nicer to me.

I wonder though: we keep unknown options when calling `parse_options()`
and then end up passing them to `setup_revisions()`. But are there even
any options handled by `setup_revisions()` that would make sense in our
context? And if not, shouldn't we rather make `parse_options()` die in
case it sees unknown options?

If there are, we should probably document this because it isn't obvious
to me.

> diff --git a/t/t4070-diff-pairs.sh b/t/t4070-diff-pairs.sh
> new file mode 100755
> index 0000000000..8f17e55c7d
> --- /dev/null
> +++ b/t/t4070-diff-pairs.sh
> @@ -0,0 +1,81 @@
> +#!/bin/sh
> +
> +test_description='basic diff-pairs tests'
> +. ./test-lib.sh
> +
> +# This creates a diff with added, modified, deleted, renamed, copied, and
> +# typechange entries. This includes a submodule to test submodule diff support.
> +test_expect_success 'setup' '
> +	test_config_global protocol.file.allow always &&
> +	test_create_repo sub &&

Use of `test_create_repo ()` is deprecated, as it is merely a wrapper
around git-init(1).

Patrick