Junio C Hamano <gitster@xxxxxxxxx> writes:
> Sergey Organov <sorganov@xxxxxxxxx> 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
