From: Junio C Hamano <email@example.com> To: Sergey Organov <firstname.lastname@example.org> Cc: Jeff King <email@example.com>, firstname.lastname@example.org Subject: Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off Date: Thu, 06 Aug 2020 10:10:25 -0700 [thread overview] Message-ID: <email@example.com> (raw) In-Reply-To: <firstname.lastname@example.org> (Sergey Organov's message of "Thu, 06 Aug 2020 11:34:39 +0300") Sergey Organov <email@example.com> writes: > 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. Yeah, I didn't explain this very well. The thing is that the way "--first-parent" interacts with other options that countermand "--diff-merges" (i.e. "--no-diff-merges" and "--diff-merges=off") needs to be highlighted with extra clarity, simply because "--first-parent" is different from a simple "combination" of "follow the first parent commit chain while traversing" and "--diff-merges" [*1*] It merely "implies" (or "defaults", which is the usual word we use) "--diff-merges", so anything the user says explicitly on the command line countermands it, i.e. git log --first-parent --no-diff-merges -p does not trigger "--diff-merges" because the user explicitly says that diffs for patches are unwanted after saying "--first-parent" (i.e. without "--no-diff-merges" later, we would have done the "diff-merges" implied by "--first-parent"). And git log --no-diff-merges --first-parent -p does not trigger "--diff-merges" because the user explicitly said that diffs for patches are unwanted by the time "--first-parent" is seen (i.e. without "--no-diff-merges" upfront, "--first-parent" would have weighed in to enable "--diff-merges" behavior, but because the user already said "no", it wouldn't). This is quite different from options that are orthogonal to each other. We do not need permutations of "log --root --cc" vs "log --cc --root" tested. We know from the code that they cannot possibly interact with each other. We could still test the permutations, and it may give us more "complete" coverage, but it may be of very low value. As to permutations of additive options, we do want to make sure they are truly additive (as opposed to being the "last one wins") and as a side effect of ensuring the additive nature, we would end up testing the effect of the order of options given, e.g. "git commit -m paragraph1 -m paragraph2". As to permutations of options that directly overrides or opposes each other, we may want to ensure e.g. that "git log -p -U2 -U4" uses four context lines and "git log -p -U4 -U2" uses two or that "git format-patch --attach --no-attach" does not use attachment and "git format-patch --no-attach --attach" does (i.e. "last one wins"). But again, especially for commands that use parse-options API correctly to implement their command line options, it is hard to get these two cases wrong (e.g. you need to deliberately write a callback to make "log -p -U2 -U4" additive to use six context lines), so it may be of lower value to throughly test permutations. It would not be as low as testing permutations of unrelated options, though. [Footnote] *1* It makes it worse that the "-m" option itself changes behavior (whether it is given explicitly or implied) when used together with "--first-parent". Unlike "git log -m -p" without "--first-parent", you cannot see the difference the mainline brought in to the side branch when the "--first-parent" option is in use.
next prev parent reply other threads:[~2020-08-06 17:11 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 2020-08-06 17:10 ` Junio C Hamano [this message] 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).