Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off

From: Sergey Organov <sorganov@gmail.com>
Date: Aug 6, 2020, 08:34 UTC
Message-ID: <87ft90uq8w.fsf@osv.gnss.ru>
In-Reply-To: <xmqqtuxglksy.fsf@gitster.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 15 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>>> Dunno, why original Jeff series didn't need 
>>>
>>>      log --first-parent --no-diff-merges -p master
>>>
>>> then?
>>
>> Who said it didn't need it?
>
> To elaborate a bit more, we are all humans, and even reviewers
> should be given the second chance to suggest improvements to things
> that can use them, which s/he previously missed.  If we keep saying
> "you say this is wrong, but the other guy did the same wrong thing
> last week", we can never make the world better.

I asked because I thought you see some essential difference between two tests, as you didn't suggest to add similar permutation test to the original. I think this reply resolves my doubt.

I still think this additional test not needed unless we are going to test all the permutations, and there are already 6 of them even for this simple test, so we'd need to add 10 more tests to the 2 originals. If somebody is willing to implement machinery to test all the option permutations automatically... But that's out of scope of these patches.

To be honest, if we don't test all the permutations anyway, I'd rather test most "human" variant only, that to me is:

  log --first-parent -p --no-diff-merges master

because --first-parent tells how to travel in history, and the rest tells how to represent results, starting from generic "show me diffs" and followed by "oh, but don't bother me with merge diffs, please".

And now we are back to my original suggestion to invert the order of parameters in these tests, but approached from another direction ;)

Overall, I can obviously add such a test to the series if you insist, but to me it still looks pointless. What doesn't look pointless is replacing the original with the version you suggested, for the reasons that I've already explained in preliminary discussions of these patches.

Finally, if you still insist on additional test(s), please also tell if I should add similar permutation to the original --no-diff-merges test.

Thanks, -- Sergey