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

Sergey Organov <sorganov@gmail.com> writes:

"causes more troubles"? When there is no trouble, and no "more" trouble, concretely mentioned, it is a quite weak justfiication.

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.

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".