Re: [PATCH v3 2/3] diff-merges: introduce '--dd' option

[Junio C Hamano <gitster@pobox.com>]

Sergey Organov <sorganov@gmail.com> writes:

> This option provides a shortcut to request diff with respect to first
> parent for any kind of commit, universally. It's implemented as pure
> synonym for "--diff-merges=first-parent --patch".

That explains what the patch does, but it does not tell us why it is
useful [*].

> NOTE: originally proposed as '-d', and renamed to '--dd' due to Junio
> request to keep "short-and-sweet" '-d' reserved for other uses.

The note is not grammatical, and more importantly, readers of "git
log" 6 months down the road would not care.  I'd rather not see it
in the proposed log message.  It is suitable material to place after
the three-dash line, or in the cover letter for the iteration.

> diff --git a/diff-merges.c b/diff-merges.c
> index ec97616db1df..45507588a279 100644
> --- a/diff-merges.c
> +++ b/diff-merges.c
> @@ -131,6 +131,9 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
>  	} else if (!strcmp(arg, "--cc")) {
>  		set_dense_combined(revs);
>  		revs->merges_imply_patch = 1;
> +	} else if (!strcmp(arg, "--dd")) {
> +		set_first_parent(revs);
> +		revs->merges_imply_patch = 1;

Quite straight-forward as expected.  I do not think "--dd" clicks
for many people as "first parent diffs all over", though.

>  	} else if (!strcmp(arg, "--remerge-diff")) {
>  		set_remerge_diff(revs);
>  		revs->merges_imply_patch = 1;
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index 5de1d190759f..4b474808311e 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -473,6 +473,14 @@ test_expect_success 'log --diff-merges=on matches --diff-merges=separate' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'log --dd matches --diff-merges=1 -p' '
> +	git log --diff-merges=1 -p master >result &&
> +	process_diffs result >expected &&
> +	git log --dd master >result &&
> +	process_diffs result >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'deny wrong log.diffMerges config' '
>  	test_config log.diffMerges wrong-value &&
>  	test_expect_code 128 git log

Looking good.

Thanks.


[Footnote]

* As I said elsewhere, I do not think it is a good idea to encourage
  users' to adopt a screwed-up worldview in which first parent is
  special but not special, and does the wrong thing for reverse
  merges.  If the option were short-hand for "--first-parent -p",
  at least I would be more sympathetic.