On 25/02/28 09:29AM, Patrick Steinhardt wrote:
> 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`.
Good to know, will adapt in a followup version.
>
> > + 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?
Good catch, there should not be any actaully needed options left for
`setup_revisions()` to parse as they should all be handled by
`parse_options()`. I'll remove the `PARSE_OPT_KEEP_UNKNOWN_OPT` flag.
> 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).
Good to know! I'll swap to using git-init(1) instead.
Thanks
-Justin
