list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <>
To: Sergey Organov <>
Cc: Jeff King <>,
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: <> (raw)
In-Reply-To: <> (Sergey Organov's message of "Thu, 06 Aug 2020 11:34:39 +0300")

Sergey Organov <> 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").


    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,


*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.

  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:

  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).