list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Sergey Organov <>
To: Junio C Hamano <>
Cc: Jeff King <>,
Subject: Re: [PATCH  3/3] t/t4013: add test for --diff-merges=off
Date: Thu, 06 Aug 2020 11:34:39 +0300	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <> (Junio C. Hamano's message of "Wed, 05 Aug 2020 16:44:45 -0700")

Junio C Hamano <> writes:

> Junio C Hamano <> 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.

-- Sergey

  reply	other threads:[~2020-08-06  8:34 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-05 22:08 [PATCH 0/3] Change "--diff-merges" to require parameter Sergey Organov
2020-08-05 22:08 ` [PATCH 1/3] revision: change "--diff-merges" option " Sergey Organov
2020-08-05 22:08 ` [PATCH 2/3] doc/git-log: describe --diff-merges=off Sergey Organov
2020-08-12  0:06   ` Junio C Hamano
2020-08-12  0:48     ` Junio C Hamano
2020-08-12  8:08       ` Sergey Organov
2020-08-05 22:08 ` [PATCH 3/3] t/t4013: add test for --diff-merges=off Sergey Organov
2020-08-05 22:31   ` Junio C Hamano
2020-08-05 22:44     ` Sergey Organov
2020-08-05 23:19       ` Junio C Hamano
2020-08-05 23:44         ` Junio C Hamano
2020-08-06  8:34           ` Sergey Organov [this message]
2020-08-06 17:10             ` Junio C Hamano
2020-08-06 20:52               ` Sergey Organov
2020-08-07 23:11               ` Sergey Organov
2020-08-17 17:17                 ` Junio C Hamano
2020-08-17 20:36                   ` Sergey Organov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \
    --subject='Re: [PATCH  3/3] t/t4013: add test for --diff-merges=off' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).