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
