From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com> To: Sergey Organov <sorganov@gmail.com> Cc: Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>, "Philip Oakley" <philipoakley@iee.email>, Elijah Newren <newren@gmail.com>, Felipe Contreras <felipe.contreras@gmail.com>, git@vger.kernel.org Subject: Re: [PATCH 7/9] t9902: fix completion tests for log.d* to match log.diffMerges Date: Thu, 08 Apr 2021 21:50:18 +0200 [thread overview] Message-ID: <87sg40imit.fsf@evledraar.gmail.com> (raw) In-Reply-To: <875z0wdekf.fsf@osv.gnss.ru> On Thu, Apr 08 2021, Sergey Organov wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> On Thu, Apr 08 2021, Sergey Organov wrote: >> >>> There were 3 completion tests failures due to introduction of >>> log.diffMerges configuration variable that affected the result of >>> completion of log.d. Fixed them accordingly. >>> >>> Signed-off-by: Sergey Organov <sorganov@gmail.com> >>> --- >>> t/t9902-completion.sh | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh >>> index 04ce884ef5ac..4d732d6d4f81 100755 >>> --- a/t/t9902-completion.sh >>> +++ b/t/t9902-completion.sh >>> @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' ' >>> test_completion "git config log.d" <<-\EOF >>> log.date Z >>> log.decorate Z >>> + log.diffMerges Z >>> EOF >>> ' >>> >>> @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' ' >>> test_completion "git -c log.d" <<-\EOF >>> log.date=Z >>> log.decorate=Z >>> + log.diffMerges=Z >>> EOF >>> ' >>> >>> @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' ' >>> test_completion "git clone --config=log.d" <<-\EOF >>> log.date=Z >>> log.decorate=Z >>> + log.diffMerges=Z >>> EOF >>> ' >> >> Commits should be made in such a way as to not break the build/tests >> partway through a series, which it seems is happening until this >> fixup. > > Yep. > > Could these tests be somehow written in a more robust manner, to be > protected against future additions of configuration variables that are > unrelated to the features being tested? If so, I'd prefer to fix them as > a prerequisite to the series rather than adding fixes to unrelated > existing tests into my patches. Hrm? I mean if you have a commit fixing up failing tests in an earlier commit then that change should in one way or the other be made as part of that earlier change. Yes we can skip the tests or something in the meantime, which we do sometimes as part of some really large changes, but these can just be squashed, no? >> Having read this far most of what you have in this 9 patch series >> could/should be squashed into something much smaller, e.g. tests being >> added for code added in previous steps, let's add the tests along with >> the code since this isn't such a large change. > > In general, I try to make commits as small as possible, but if you > prefer tests to be included with the code in the same commit, – that's > fine with me too. > > Will meld new tests into code commits for the next re-roll. I'm probably the last person to give advice on this list about not overly splitting up ones commits :) Having said that, some sage advice: It's really helpful to split commits into discrete understandable pieces when it aids in reviewing/understanding the code. But something like say your 8/9 is IMNSHO a step to far, you're just adding a feature earlier and then docs for it later. That doesn't help to review or understand the change, now you just need to look in two places for what's one logical change. Ditto for e.g. the 5/9 here. That's just a test for a feature added earlier. So let's add it to the commit where we add that feature. There *are* cases where it helps to split up these changes, but they're things like adding tests for existing behavior before changing something, as an aid to demonstrate what the behavior was before & after. In those cases it's a lot better to split the commits, because nobody wants to waste time discerning what's a test for existing v.s. new behavior.
next prev parent reply other threads:[~2021-04-08 19:50 UTC|newest] Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-07 22:55 [PATCH 0/9] git log: configurable default format for merge diffs Sergey Organov 2021-04-07 22:56 ` [PATCH 1/9] diff-merges: introduce --diff-merges=def Sergey Organov 2021-04-08 11:48 ` Philip Oakley 2021-04-08 14:21 ` Sergey Organov 2021-04-08 17:27 ` Junio C Hamano 2021-04-08 17:38 ` Sergey Organov 2021-04-07 22:56 ` [PATCH 2/9] diff-merges: refactor set_diff_merges() Sergey Organov 2021-04-07 22:56 ` [PATCH 3/9] diff-merges: introduce log.diffMerges config variable Sergey Organov 2021-04-08 21:37 ` SZEDER Gábor 2021-04-08 21:51 ` SZEDER Gábor 2021-04-08 22:01 ` Junio C Hamano 2021-04-08 23:04 ` Sergey Organov 2021-04-07 22:56 ` [PATCH 4/9] diff-merges: adapt -m to enable default diff format Sergey Organov 2021-04-07 22:56 ` [PATCH 5/9] t4013: add test for --diff-merges=def Sergey Organov 2021-04-07 22:56 ` [PATCH 6/9] t4013: add tests for log.diffMerges config Sergey Organov 2021-04-07 23:06 ` Ævar Arnfjörð Bjarmason 2021-04-07 23:35 ` Junio C Hamano 2021-04-08 14:25 ` Sergey Organov 2021-04-07 22:56 ` [PATCH 7/9] t9902: fix completion tests for log.d* to match log.diffMerges Sergey Organov 2021-04-07 23:05 ` Ævar Arnfjörð Bjarmason 2021-04-08 14:41 ` Sergey Organov 2021-04-08 19:50 ` Ævar Arnfjörð Bjarmason [this message] 2021-04-08 20:26 ` Sergey Organov 2021-04-08 22:13 ` SZEDER Gábor 2021-04-08 23:07 ` Sergey Organov 2021-04-07 22:56 ` [PATCH 8/9] doc/diff-options: document new --diff-merges features Sergey Organov 2021-04-07 22:56 ` [PATCH 9/9] doc/config: document log.diffMerges Sergey Organov 2021-04-10 17:16 ` [PATCH v1 0/5] git log: configurable default format for merge diffs Sergey Organov 2021-04-10 17:16 ` [PATCH v1 1/5] diff-merges: introduce --diff-merges=default Sergey Organov 2021-04-10 17:16 ` [PATCH v1 2/5] diff-merges: refactor set_diff_merges() Sergey Organov 2021-04-10 17:16 ` [PATCH v1 3/5] diff-merges: adapt -m to enable default diff format Sergey Organov 2021-04-10 17:16 ` [PATCH v1 4/5] diff-merges: introduce log.diffMerges config variable Sergey Organov 2021-04-10 17:16 ` [PATCH v1 5/5] doc/diff-options: document new --diff-merges features Sergey Organov 2021-04-11 16:13 ` [PATCH v1 0/5] git log: configurable default format for merge diffs Junio C Hamano 2021-04-11 18:04 ` Sergey Organov 2021-04-11 19:02 ` Junio C Hamano 2021-04-11 20:38 ` Sergey Organov 2021-04-11 21:58 ` Sergey Organov 2021-04-13 11:41 ` [PATCH v2 " Sergey Organov 2021-04-13 11:41 ` [PATCH v2 1/5] diff-merges: introduce --diff-merges=on Sergey Organov 2021-04-13 23:18 ` Junio C Hamano 2021-04-13 11:41 ` [PATCH v2 2/5] diff-merges: refactor set_diff_merges() Sergey Organov 2021-04-13 11:41 ` [PATCH v2 3/5] diff-merges: adapt -m to enable default diff format Sergey Organov 2021-04-13 11:41 ` [PATCH v2 4/5] diff-merges: introduce log.diffMerges config variable Sergey Organov 2021-04-15 20:21 ` Junio C Hamano 2021-04-16 8:30 ` Sergey Organov 2021-04-13 11:41 ` [PATCH v2 5/5] doc/diff-options: document new --diff-merges features 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 \ --in-reply-to=87sg40imit.fsf@evledraar.gmail.com \ --to=avarab@gmail.com \ --cc=felipe.contreras@gmail.com \ --cc=git@vger.kernel.org \ --cc=gitster@pobox.com \ --cc=newren@gmail.com \ --cc=peff@peff.net \ --cc=philipoakley@iee.email \ --cc=sorganov@gmail.com \ --subject='Re: [PATCH 7/9] t9902: fix completion tests for log.d* to match log.diffMerges' \ /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).