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

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