From: Sergey Organov <sorganov@gmail•com>
To: Junio C Hamano <gitster@pobox•com>
Cc: Jeff King <peff@peff•net>, git@vger•kernel.org
Subject: Re: [PATCH v2] revision: add separate field for "-m" of "diff-index -m"
Date: Mon, 31 Aug 2020 15:45:33 +0300 [thread overview]
Message-ID: <87a6ybugpe.fsf@osv.gnss.ru> (raw)
In-Reply-To: <xmqqo8mrh12b.fsf@gitster.c.googlers.com> (Junio C. Hamano's message of "Sun, 30 Aug 2020 21:49:32 -0700")
Junio C Hamano <gitster@pobox•com> writes:
> Sergey Organov <sorganov@gmail•com> writes:
>
>> Historically, in "diff-index -m", "-m" does not mean "do not ignore
>> merges", but
>> "match missing". Despite this, diff-index abuses 'ignore_merges'
>> field being set
>> by "-m", that in turn causes more troubles.
>
> "causes more troubles"? When there is no trouble, and no "more"
> trouble, concretely mentioned, it is a quite weak justfiication.
Well, existed comment says "Backward compatibility wart" that sounds
like a trouble to me already. No?
Then, since "--[no-]diff-merges" is introduced, we have:
$ git diff-index HEAD
:100644 000000 4aec621a6d1a9a5892f0b4b6feb2ed329fd04bf2 0000000000000000000000000000000000000000 D main/main.cc
$ git diff-index -m HEAD
$ git diff-index -m --no-diff-merges HEAD
:100644 000000 4aec621a6d1a9a5892f0b4b6feb2ed329fd04bf2 0000000000000000000000000000000000000000 D main/main.cc
that sounds like yet another trouble. That's why I used "more trouble" in my
commit message.
If you say "compatibility wart" is not a trouble by itself, -- I'm fine
with it, -- then "more" in my commit message is misplaced indeed.
>
> There is no reason to say "historically" here, as it has been like
> so from beginning of the time, it still is so and it is relied
> upon. "diff-{files,index,tree}" are about comparing two things, and
> not about history (where a "merge" might influence "now we are
> showing this commit. which parent do we compare it with?"), so
> giving short-and-sweet "-m" its own meaning that is sensible within
> the context of "diff" was and is perfectly sensible thing to do.
Well, if "historically" makes you feel uncomfortable, -- I'm willing to
get rid of it.
>
> What is worth fixing is not "-m" in diff-index means "match missing"
> while "-m" in log wants to mean "show merges". It is that, even both
> commands use the same option parsing machinery, and the use of these
> two options are mutually exclusive so there is no risk of confusion,
> the flag internally used to record the presense of the "em" option is
> not named neutrally (e.g. "revs->seen_em_option").
>
> The "log" family of commands and "diff" family of commands
> share the same command line parsiong machinery. For the
> former, "-m" means "show merges" while for the latter it
> means "match missing". Tnis is not a problem at the UI
> level, as "show/not show merges" is meaningless in the
> context of "diff", and similarly "match/not match missing"
> is meaningless in the context of "log".
>
> But there are two problems with this arrangement.
>
> 1. the field the presense of the option on the command line
> is recorded in has to be given a name. It is currently
> called "ignore_merges", which gives an incorrect
> impression that using it for "diff" family is somehow a
> mistake, and renaming it to "match_missing" would not be
> a solution, as it will give an incorrect impression that
> "log" family is abusing it. However, naming the field to
> something neutral, e.g. "em_option", would make the code
> harder to understand.
>
> 2. because it uses the same command line parser, giving a
> default for "diff -m" in a way that is different from the
> default for "log -m" is quite cumbersome if they use the
> same field to record it.
>
> Introduce a separate "match_missing" field, and flip it and
> "ignore_merges" when we see the "-m" option on the command
> line. That way, even when ignore_merges's default is
> affected by end-user configuration, the default for
> "match_missing" would not be affected.
>
> I think the above would be in line with what you wanted to say but
> didn't, and I think it supports the split fairly well.
>
> I have a very strong objection against changing the built-in default
> of "log -m", but I do agree that this split of the single field into
> two is a fairly good idea. So I do not want to be in the position
> that must reject this change because "log -m" and "diff-index -m"
> will never be on by default. Basing the justification of this
> change on end-user configurability would be a good way to sidestep
> the issue, and avoids taking this change hostage to the discussion
> on what should be the built-in default for "log/diff-index -m".
This change has nothing to do with defaults. It rather about correct and
clear code.
I'll re-roll with better commit message.
Thanks,
-- Sergey
next prev parent reply other threads:[~2020-08-31 12:45 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-29 19:46 [PATCH] revision: add separate field for "-m" of "diff-index -m" Sergey Organov
2020-08-29 20:11 ` [PATCH v2] " Sergey Organov
2020-08-31 4:49 ` Junio C Hamano
2020-08-31 12:45 ` Sergey Organov [this message]
2020-08-31 17:00 ` Junio C Hamano
2020-08-31 12:53 ` [PATCH v3] " Sergey Organov
2020-08-31 17:23 ` Junio C Hamano
2020-08-31 19:41 ` Sergey Organov
2020-08-31 20:14 ` [PATCH v4] " Sergey Organov
2020-08-31 20:42 ` Junio C Hamano
2020-08-31 21:01 ` Sergey Organov
2020-10-23 4:03 ` [PATCH] revision: wording tweak in comment for parsing "-m" Junio C Hamano
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=87a6ybugpe.fsf@osv.gnss.ru \
--to=sorganov@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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