Re: [PATCH v2] revision: add separate field for "-m" of "diff-index -m"

[Sergey Organov <sorganov@gmail.com>]

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