From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [Bug?] "git diff --no-rename A B"
Date: Fri, 19 Jan 2024 20:18:00 -0500 [thread overview]
Message-ID: <20240120011800.GF117170@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqq34uvtpob.fsf@gitster.g>
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
next prev parent reply other threads:[~2024-01-20 1:18 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-18 1:07 [Bug?] "git diff --no-rename A B" Junio C Hamano
2024-01-18 6:26 ` Dragan Simic
2024-01-20 1:18 ` Jeff King [this message]
2024-01-20 14:39 ` René Scharfe
2024-01-20 17:55 ` Junio C Hamano
2024-01-21 17:56 ` René Scharfe
2024-01-22 23:00 ` Jeff King
2024-01-23 0:28 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240120011800.GF117170@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).