On Wed, Jan 17, 2024 at 05:07:16PM -0800, Junio C Hamano wrote:
> When the user misspells "--no-renames" as "--no-rename", it seems
> that the rename detection (which is ont by default these days) still
> kicks in, which means the misspelt option is silently ignored.
> Should we show an error message instead?
I wondered if "--no-foo" complained, and it does. I think this is a
subtle corner case in parse-options.
The issue is that we have "--rename-empty", which of course also
provides "--no-rename-empty". And parse-options is happy to let you
abbreviate names as long as they are unambiguous. But "--no-rename" _is_
ambiguous with "--no-renames". Why don't we catch it?
I'd guess it is because we do not have "--renames" as an option, but
explicitly generate an entry for "--no-renames" (since the non-negative
version is actually "--find-renames"). I know there is some special code
to handle these pre-negated cases, but I would not be surprised if the
ambiguity checker does not.
So I think it's likely just a bug in parse-options which should be
fixed.
We could also work around it by providing --renames. ;) E.g., if we let
the find-renames callback handle negation, then --renames becomes a
synonym, like so:
diff --git a/diff.c b/diff.c
index a89a6a6128..cdec9bfbd9 100644
--- a/diff.c
+++ b/diff.c
@@ -5292,7 +5292,11 @@ static int diff_opt_find_renames(const struct option *opt,
{
struct diff_options *options = opt->value;
- BUG_ON_OPT_NEG(unset);
+ if (unset) {
+ options->detect_rename = 0;
+ return 0;
+ }
+
if (!arg)
arg = "";
options->rename_score = parse_rename_score(&arg);
@@ -5686,7 +5690,7 @@ struct option *add_diff_options(const struct option *opts,
diff_opt_break_rewrites),
OPT_CALLBACK_F('M', "find-renames", options, N_("<n>"),
N_("detect renames"),
- PARSE_OPT_NONEG | PARSE_OPT_OPTARG,
+ PARSE_OPT_OPTARG,
diff_opt_find_renames),
OPT_SET_INT_F('D', "irreversible-delete", &options->irreversible_delete,
N_("omit the preimage for deletes"),
@@ -5697,9 +5701,10 @@ struct option *add_diff_options(const struct option *opts,
diff_opt_find_copies),
OPT_BOOL(0, "find-copies-harder", &options->flags.find_copies_harder,
N_("use unmodified files as source to find copies")),
- OPT_SET_INT_F(0, "no-renames", &options->detect_rename,
- N_("disable rename detection"),
- 0, PARSE_OPT_NONEG),
+ OPT_CALLBACK_F(0, "renames", options, N_("<n>"),
+ N_("synonym for --find-renames"),
+ PARSE_OPT_OPTARG,
+ diff_opt_find_renames),
OPT_BOOL(0, "rename-empty", &options->flags.rename_empty,
N_("use empty blobs as rename source")),
OPT_CALLBACK_F(0, "follow", options, NULL,
And you get the expected output:
$ git show f920b0289b --oneline --raw --no-rename
error: ambiguous option: no-rename (could be --no-renames or --no-rename-empty)
And as a bonus, now "--renames" works. :) It might pollute the output of
"-h" more, but I am not sure if we ever actually show these diff options
via "-h" (they are parsed quite indirectly, and "-h" is handled by the
main command's parse-options list).
Still, it seems like it's worth fixing the parse-options bug.
-Peff
