Re: [PATCH 4/6] t: use hash-object --literally when created malformed objects

From: Taylor Blau <me@ttaylorr.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, "René Scharfe" <l.s.r@web.de>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH 4/6] t: use hash-object --literally when created malformed objects
Date: Wed, 18 Jan 2023 16:19:20 -0500	[thread overview]
Message-ID: <Y8hiWGOArYqzen58@nand.local> (raw)
In-Reply-To: <Y8hZlN9rRg1msc0L@coredump.intra.peff.net>

On Wed, Jan 18, 2023 at 03:41:56PM -0500, Jeff King wrote:
> Many test scripts use hash-object to create malformed objects to see how
> we handle the results in various commands. In some cases we already have
> to use "hash-object --literally", because it does some rudimentary
> quality checks. But let's use "--literally" more consistently to
> future-proof these tests against hash-object learning to be more
> careful.

Heh, I suppose this is a good illustration of how loose our checks our
even without `--literally` ;-).

> ---
> This patch is worth looking at because it shows the kinds of things the
> new hash-object from patch 6 will reject.

Obviously we could avoid this patch entirely by making the new behavior
of fscking the incoming objects hidden behind a `--fsck` flag or
something. But I think the decision not to is a good one.

We already have `--literally`, and it makes sense that passing that
should let us write anything, and that not passing it should perform
some validity checks. But I think exactly *what* those checks are is
ambiguous enough that the absence of `--literally` implying fsck checks
isn't out of the question.

You address this in the last patch more thoroughly, but I figure that it
is worth stating some of this here during review to indicate that I
think the direction you pursued here is a good one.

>  t/t1450-fsck.sh                 | 28 ++++++++++++++--------------
>  t/t4054-diff-bogus-tree.sh      |  2 +-
>  t/t4058-diff-duplicates.sh      |  2 +-
>  t/t4212-log-corrupt.sh          |  4 ++--
>  t/t5302-pack-index.sh           |  2 +-
>  t/t5504-fetch-receive-strict.sh |  2 +-
>  t/t5702-protocol-v2.sh          |  2 +-
>  t/t6300-for-each-ref.sh         |  2 +-
>  t/t7509-commit-authorship.sh    |  2 +-
>  t/t7510-signed-commit.sh        |  2 +-
>  t/t7528-signed-commit-ssh.sh    |  2 +-
>  t/t8003-blame-corner-cases.sh   |  2 +-
>  t/t9350-fast-export.sh          |  2 +-
>  13 files changed, 27 insertions(+), 27 deletions(-)

And these all look good, too. Each of the spots you touch here is
limited to replacing "git hash-object" with "git hash-object --literally".

Thanks,
Taylor