From: Sergey Organov <firstname.lastname@example.org> To: Junio C Hamano <email@example.com> Cc: Jeff King <firstname.lastname@example.org>, email@example.com 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: <firstname.lastname@example.org> (raw) In-Reply-To: <email@example.com> (Junio C. Hamano's message of "Wed, 05 Aug 2020 16:44:45 -0700") Junio C Hamano <firstname.lastname@example.org> writes: > Junio C Hamano <email@example.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
next prev parent 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: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * 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: https://80x24.org/mirrors/git.git 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).