Re: [PATCH v4 3/4] builtin: introduce diff-pairs command
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Feb 28, 2025, 08:29 UTC
- Message-ID
- <Z8Fz7CQimWgzrvz7@pks.im>
- In-Reply-To
- <20250228002604.3859939-4-jltobler@gmail.com>
On Thu, Feb 27, 2025 at 06:26:03PM -0600, Justin Tobler wrote:
Show 5 quoted lines
> 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]
Show 15 quoted lines
> +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`.
Show 26 quoted lines
> + 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.
Show 16 quoted lines
> 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