git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Sergey Organov <sorganov@gmail.com>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"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: Fri, 9 Apr 2021 00:13:43 +0200	[thread overview]
Message-ID: <20210408221343.GC2947267@szeder.dev> (raw)
In-Reply-To: <87k0pc7cap.fsf@osv.gnss.ru>

On Thu, Apr 08, 2021 at 11:26:38PM +0300, Sergey Organov wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > 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.

Well, actually no: it _starts_ to break with this patch, because
'log.diffMerges' is not documented yet, and it will pass again with
the last patch in the series that adds that missing piece of
documentation.

> >> 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?
> 
> I mean I don't want this change at all.

You'll definitely need this change, though.

> I didn't change completion mechanism, so completion tests should not
> suddenly fail because of my changes.

We auto-generate the list of supported configuration variables from
the documentation and use that list in our Bash completion script to
list possible configuration variables for 'git config <TAB>' and 'git
-c <TAB>'.  And we want to make sure that this feature works as
intended, so we have a couple of tests that try to complete real
config variable sections and names.  You are just unlucky to introduce
a new configuraton variable that happenes to start with the same
prefix that is used in some of those tests.

> I did entirely unrelated change and
> noticed the breakage only by accident, as tests even don't fail unless
> you *install* git, not only make it. So, for example, just "make test"
> doesn't fail, while "make install; make test" will.

It might be related to a bug in the build process that doesn't update
that auto-generated list of supported configuration variables after
e.g. 'Documentation/config/log.txt' was modified; see a proposed fix
at:

  https://public-inbox.org/git/20210408212915.3060286-1-szeder.dev@gmail.com/

> It looks like something is wrong here, a bug or misfeature, or even two,
> and if it's fixed before these series, I won't need this in my series at
> all. Besides, that's yet another reason *not* to squash this change into
> an otherwise unrelated commit.

The introduction of the new configuration variable, its documentation
and this test update should all go into a single patch.  The whole
test suite must pass for every single commit.


  reply	other threads:[~2021-04-08 22:14 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
2021-04-08 20:26         ` Sergey Organov
2021-04-08 22:13           ` SZEDER Gábor [this message]
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=20210408221343.GC2947267@szeder.dev \
    --to=szeder.dev@gmail.com \
    --cc=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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

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

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git