From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Jeff King <peff@peff.net>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
"Jiang Xin" <zhiyou.jx@alibaba-inc.com>
Subject: Re: [PATCH 07/12] test-lib-functions: move function to lib-bitmap.sh
Date: Thu, 11 Feb 2021 20:38:20 +0100 [thread overview]
Message-ID: <20210211193820.GH1015009@szeder.dev> (raw)
In-Reply-To: <YCRL4lq5eQnPLwK1@coredump.intra.peff.net>
On Wed, Feb 10, 2021 at 04:10:58PM -0500, Jeff King wrote:
> On Wed, Feb 10, 2021 at 09:56:45PM +0100, SZEDER Gábor wrote:
>
> > > +test_bitmap_traversal () {
> > > + if test "$1" = "--no-confirm-bitmaps"
> > > + then
> > > + shift
> > > + elif cmp "$1" "$2"
> > > + then
> > > + echo >&2 "identical raw outputs; are you sure bitmaps were used?"
> > > + return 1
> >
> > Since you converted two 'error's to BUG earlier in this series...
> >
> > This helper function was added in ea047a8eb4 (t5310: factor out bitmap
> > traversal comparison, 2020-02-14), and so I Cc: its author and quote
> > part of its commit message:
> >
> > While we're at it, let's also try to confirm that the bitmap output did
> > indeed use bitmaps. Since the code internally falls back to the
> > non-bitmap path in some cases, the tests are at risk of becoming trivial
> > noops.
> >
> > This is a bit fragile, as not all outputs will differ (e.g., looking at
> > only the commits from a fully-bitmapped pack will end up exactly the
> > same as the normal traversal order, since it also matches the pack
> > order). So we'll provide an escape hatch by which tests can disable this
> > check (which should only be used after manually confirming that bitmaps
> > kicked in).
> >
> > Shouldn't this be a BUG as well? I think this should be a BUG.
>
> I dunno. I guess you are thinking of the case where somebody adds a new
> caller but fails to invoke Git correctly. Which would be a bug in the
> test script.
>
> But it could also easily be due to Git changing how it behaves in a
> certain circumstance. And maybe the solution is changing the test to
> adapt, or maybe we just found a bug in Git.
Well, I though that if the raw output is the same with and without
pack bitmaps, then the missing '--no-confirm-bitmaps' option is the
bug in the test script.
> TBH, I do not care that much either way. Either way, I think somebody
> would notice the problem.
Me neither, I just don't like test helper functions that simply return
in the middle, becauase that makes suppressing their trace output even
more awkward than it already is :)
next prev parent reply other threads:[~2021-02-11 19:41 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-09 21:41 [PATCH 00/12] test-lib: misc improvements Ævar Arnfjörð Bjarmason
2021-02-09 21:41 ` [PATCH 01/12] test-lib: remove check_var_migration Ævar Arnfjörð Bjarmason
2021-02-10 22:06 ` Junio C Hamano
2021-02-09 21:41 ` [PATCH 02/12] test lib: change "error" to "BUG" as appropriate Ævar Arnfjörð Bjarmason
2021-02-09 21:41 ` [PATCH 03/12] test-lib-functions: move test_set_index_version() to its user Ævar Arnfjörð Bjarmason
2021-02-09 21:41 ` [PATCH 04/12] test-lib-functions: remove generate_zero_bytes() wrapper Ævar Arnfjörð Bjarmason
2021-02-09 21:41 ` [PATCH 05/12] test libs: rename bundle helper to "lib-bundle.sh" Ævar Arnfjörð Bjarmason
2021-02-09 21:41 ` [PATCH 06/12] test libs: rename gitweb-lib.sh to lib-gitweb.sh Ævar Arnfjörð Bjarmason
2021-02-09 21:41 ` [PATCH 07/12] test-lib-functions: move function to lib-bitmap.sh Ævar Arnfjörð Bjarmason
2021-02-10 20:56 ` SZEDER Gábor
2021-02-10 21:10 ` Jeff King
2021-02-11 19:38 ` SZEDER Gábor [this message]
2021-02-09 21:41 ` [PATCH 08/12] t/.gitattributes: sort lines Ævar Arnfjörð Bjarmason
2021-02-09 21:41 ` [PATCH 09/12] test libs: rename "diff-lib" to "lib-diff" Ævar Arnfjörð Bjarmason
2021-02-10 21:56 ` Junio C Hamano
2021-02-11 22:13 ` Johannes Schindelin
2021-02-11 22:45 ` Junio C Hamano
2021-02-09 21:41 ` [PATCH 10/12] test-lib-functions: remove bug-inducing "diagnostics" helper param Ævar Arnfjörð Bjarmason
2021-02-09 21:41 ` [PATCH 11/12] test-lib-functions: assert correct parameter count Ævar Arnfjörð Bjarmason
2021-02-09 21:41 ` [PATCH 12/12] test-lib-functions: split out {debug,path,text} helpers Ævar Arnfjörð Bjarmason
2021-02-09 23:37 ` Denton Liu
2021-02-10 0:06 ` Junio C Hamano
2021-02-11 19:27 ` SZEDER Gábor
2021-02-11 22:18 ` Johannes Schindelin
2021-02-13 14:39 ` Ævar Arnfjörð Bjarmason
2021-02-12 13:29 ` [PATCH v2 00/11] test-lib: misc improvements Ævar Arnfjörð Bjarmason
2021-02-12 22:35 ` Junio C Hamano
2021-02-12 13:29 ` [PATCH v2 01/11] test-lib: remove check_var_migration Ævar Arnfjörð Bjarmason
2021-02-12 13:29 ` [PATCH v2 02/11] test lib: change "error" to "BUG" as appropriate Ævar Arnfjörð Bjarmason
2021-02-12 13:29 ` [PATCH v2 03/11] test-lib-functions: move test_set_index_version() to its user Ævar Arnfjörð Bjarmason
2021-02-12 13:29 ` [PATCH v2 04/11] test-lib-functions: remove generate_zero_bytes() wrapper Ævar Arnfjörð Bjarmason
2021-02-12 13:29 ` [PATCH v2 05/11] test libs: rename bundle helper to "lib-bundle.sh" Ævar Arnfjörð Bjarmason
2021-02-12 13:29 ` [PATCH v2 06/11] test libs: rename gitweb-lib.sh to lib-gitweb.sh Ævar Arnfjörð Bjarmason
2021-02-12 13:29 ` [PATCH v2 07/11] test-lib-functions: move function to lib-bitmap.sh Ævar Arnfjörð Bjarmason
2021-02-12 13:29 ` [PATCH v2 08/11] t/.gitattributes: sort lines Ævar Arnfjörð Bjarmason
2021-02-12 13:29 ` [PATCH v2 09/11] test libs: rename "diff-lib" to "lib-diff" Ævar Arnfjörð Bjarmason
2021-02-12 13:29 ` [PATCH v2 10/11] test-lib-functions: remove bug-inducing "diagnostics" helper param Ævar Arnfjörð Bjarmason
2021-02-12 13:29 ` [PATCH v2 11/11] test-lib-functions: assert correct parameter count Ævar Arnfjörð Bjarmason
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=20210211193820.GH1015009@szeder.dev \
--to=szeder.dev@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
--cc=zhiyou.jx@alibaba-inc.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public 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).