All of lore.kernel.org
 help / color / mirror / Atom feed
From: Matthew DeVore <matvore@google.com>
To: rybak.a.v@gmail.com
Cc: git@vger.kernel.org, gitster@pobox.com, kirr@nexedi.com,
	peff@peff.net, szeder.dev@gmail.com
Subject: Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
Date: Wed, 22 Aug 2018 11:14:44 -0700	[thread overview]
Message-ID: <20180822181444.169611-1-matvore@google.com> (raw)
In-Reply-To: <4d6ffc81-a1e8-a60f-d53f-2ec159160fcd@gmail.com>

I would encourage use of an existing function to check for emptiness,
but require a particular argument for it to be considered "the right
way:"

test_cmp /dev/null actual

This means less vocabulary to memorize for test writers. It's usually a
code smell to have special logic for a specific value for a specific
argument - a sign that a separate function ought to be created - but
since we want to add an error or warning in test_cmp anyway when
<EXPECTED> is empty, I think this special logic is OK.

As for comparing against a file that *might* be empty, like a utility
function, might I suggest requiring the file name be formatted in a
specific way if it may be empty? Like require a certain substring. Then
the syntax for comparison would be:

# If the emptiness is unconditional
test_cmp /dev/null actual

# If the emptiness is unknown ahead of time
test_cmp maybe_empty_expected actual

Then, issue an error for something like:
> expected && test_cmp expected actual

which says: "
Use test_cmp /dev/null <ACTUAL> to verify a file is empty.
If the <EXPECTED> file may or may not be empty (as in a utility
function), include the string "maybe_empty" in the <EXPECTED> file name.
"

  parent reply	other threads:[~2018-08-22 18:14 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-14 11:47 [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test SZEDER Gábor
2018-08-14 21:49 ` Jeff King
2018-08-16 20:51 ` Andrei Rybak
2018-08-16 22:36   ` Junio C Hamano
2018-08-17 17:39     ` SZEDER Gábor
2018-08-17 19:27       ` Andrei Rybak
2018-08-17 20:09         ` Junio C Hamano
2018-08-19 17:50           ` Andrei Rybak
2018-08-19 20:32             ` Jeff King
2018-08-19 21:37               ` Andrei Rybak
2018-08-19 21:43                 ` Jeff King
2018-08-21 21:52                   ` Junio C Hamano
2018-08-17 20:15         ` SZEDER Gábor
2018-08-22 18:14   ` Matthew DeVore [this message]
2018-08-27 10:22 ` Kirill Smelkov
2018-08-27 23:04   ` Jeff King
2018-08-28  6:37     ` Kirill Smelkov

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

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180822181444.169611-1-matvore@google.com \
    --to=matvore@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kirr@nexedi.com \
    --cc=peff@peff.net \
    --cc=rybak.a.v@gmail.com \
    --cc=szeder.dev@gmail.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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.