git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "brian m. carlson" <sandals@crustytoothpaste.net>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH v4 31/39] bundle: add new version for use with SHA-256
Date: Sun, 26 Jul 2020 22:59:10 +0000	[thread overview]
Message-ID: <20200726225910.GD6540@camp.crustytoothpaste.net> (raw)
In-Reply-To: <CAPig+cR+e9XGNCgtDMHUsaAgbKDi=-bztwtd64fVZj7S05ppKQ@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2281 bytes --]

On 2020-07-26 at 22:18:23, Eric Sunshine wrote:
> This error message would be more helpful if it provided more context,
> such as the name it tried looking up. For instance:
> 
>     return error(_("unrecognized bundle header hash algorithm: "
>         "@object-format=%s"), arg);
> 
> or something.

I've improved it somewhat.

> The reader has to think through this unconditional NUL-termination
> more carefully than would be the case if...
> ... you just moved this strbuf_rtrim() call above the capability check
> conditional.

That is indeed much easier.

> 
> This conditional will become fragile when bundle version v4 is
> introduced and the git-bundle invocation somehow triggers v4 to be
> assigned to 'default_version', in which case:
> 
>     git bundle --version=3 ...
> 
> will complain:
> 
>     cannot write bundle version 3 with algorithm sha256
> 
> which is clearly wrong and misleading.

I've changed this variable to min_version.  There is one place we use it
as the default, but I think it's easy to change that if we want it to be
something different in the future, and all the rest of the uses are of a
minimum version.

> Do you still need the "head -n 4 ... &&" check at the very top of this
> list? Is that providing something that we don't get from the new code
> which uses test_cmp with 'expect' and 'actual' files?

Removed.

> In my earlier review when I suggested using an "expect" file and
> converting the object ID to some literal string such as "OID", the
> example I gave did indeed also use literal "message", though my use of
> "message" was meant as a placeholder that you would fill in with the
> real values, like this:
> 
>     -OID updated by origin
>     OID refs/heads/master
> 
> I probably should have been clearer about "message" being a
> placeholder (since I was too lazy to look up the actual values). I
> suppose the generic "message" you use here is no worse than the
> original code with its 'grep' invocations which didn't care about the
> message either. It makes me a bit uncomfortable for the test to
> unnecessarily be loose like this when it doesn't have to be, but it's
> not necessarily worth a re-roll.

Fixed.
-- 
brian m. carlson: Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

  reply	other threads:[~2020-07-26 22:59 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-26 19:53 [PATCH v4 00/39] SHA-256, part 3/3 brian m. carlson
2020-07-26 19:53 ` [PATCH v4 01/39] t: make test-bloom initialize repository brian m. carlson
2020-07-26 19:53 ` [PATCH v4 02/39] t1001: use $ZERO_OID brian m. carlson
2020-07-26 19:53 ` [PATCH v4 03/39] t3305: make hash agnostic brian m. carlson
2020-07-26 19:53 ` [PATCH v4 04/39] t3404: prepare 'short SHA-1 collision' tests for SHA-256 brian m. carlson
2020-07-26 22:31   ` Eric Sunshine
2020-07-26 19:53 ` [PATCH v4 05/39] t6100: make hash size independent brian m. carlson
2020-07-26 19:53 ` [PATCH v4 06/39] t6101: " brian m. carlson
2020-07-26 19:53 ` [PATCH v4 07/39] t6301: " brian m. carlson
2020-07-26 19:53 ` [PATCH v4 08/39] t6500: specify test values for SHA-256 brian m. carlson
2020-07-26 19:53 ` [PATCH v4 09/39] t6501: avoid hard-coded objects brian m. carlson
2020-07-26 19:53 ` [PATCH v4 10/39] t7003: compute appropriate length constant brian m. carlson
2020-07-26 19:53 ` [PATCH v4 11/39] t7063: make hash size independent brian m. carlson
2020-07-26 22:40   ` Eric Sunshine
2020-07-26 19:53 ` [PATCH v4 12/39] t7201: abstract away SHA-1-specific constants brian m. carlson
2020-07-26 22:54   ` Eric Sunshine
2020-07-26 19:53 ` [PATCH v4 13/39] t7102: " brian m. carlson
2020-07-26 19:53 ` [PATCH v4 14/39] t7400: make hash size independent brian m. carlson
2020-07-26 19:54 ` [PATCH v4 15/39] t7405: " brian m. carlson
2020-07-26 19:54 ` [PATCH v4 16/39] t7506: avoid checking for SHA-1-specific constants brian m. carlson
2020-07-26 19:54 ` [PATCH v4 17/39] t7508: use $ZERO_OID instead of hard-coded constant brian m. carlson
2020-07-26 19:54 ` [PATCH v4 18/39] t8002: make hash size independent brian m. carlson
2020-07-26 22:49   ` Eric Sunshine
2020-07-26 19:54 ` [PATCH v4 19/39] t8003: " brian m. carlson
2020-07-26 19:54 ` [PATCH v4 20/39] t8011: " brian m. carlson
2020-07-26 23:00   ` Eric Sunshine
2020-07-26 19:54 ` [PATCH v4 21/39] t9300: abstract away SHA-1-specific constants brian m. carlson
2020-07-26 23:14   ` Eric Sunshine
2020-07-26 19:54 ` [PATCH v4 22/39] t9300: use $ZERO_OID instead of hard-coded object ID brian m. carlson
2020-07-26 19:54 ` [PATCH v4 23/39] t9301: make hash size independent brian m. carlson
2020-07-26 19:54 ` [PATCH v4 24/39] t9350: " brian m. carlson
2020-07-26 19:54 ` [PATCH v4 25/39] t9500: ensure that algorithm info is preserved in config brian m. carlson
2020-07-26 19:54 ` [PATCH v4 26/39] t9700: make hash size independent brian m. carlson
2020-07-26 19:54 ` [PATCH v4 27/39] t5308: make test work with SHA-256 brian m. carlson
2020-07-26 19:54 ` [PATCH v4 28/39] t0410: mark test with SHA1 prerequisite brian m. carlson
2020-07-26 19:54 ` [PATCH v4 29/39] http-fetch: set up git directory before parsing pack hashes brian m. carlson
2020-07-26 23:28   ` Eric Sunshine
2020-07-26 23:30     ` Eric Sunshine
2020-07-26 19:54 ` [PATCH v4 30/39] builtin/verify-pack: implement an --object-format option brian m. carlson
2020-07-26 21:29   ` Eric Sunshine
2020-07-26 22:56     ` brian m. carlson
2020-07-26 19:54 ` [PATCH v4 31/39] bundle: add new version for use with SHA-256 brian m. carlson
2020-07-26 22:18   ` Eric Sunshine
2020-07-26 22:59     ` brian m. carlson [this message]
2020-07-26 19:54 ` [PATCH v4 32/39] setup: add support for reading extensions.objectformat brian m. carlson
2020-07-26 23:34   ` Eric Sunshine
2020-07-26 23:41     ` brian m. carlson
2020-07-26 19:54 ` [PATCH v4 33/39] Enable SHA-256 support by default brian m. carlson
2020-07-26 23:41   ` Eric Sunshine
2020-07-26 19:54 ` [PATCH v4 34/39] t: add test_oid option to select hash algorithm brian m. carlson
2020-07-26 19:54 ` [PATCH v4 35/39] t: allow testing different hash algorithms via environment brian m. carlson
2020-07-26 19:54 ` [PATCH v4 36/39] t: make SHA1 prerequisite depend on default hash brian m. carlson
2020-07-26 23:52   ` Eric Sunshine
2020-07-26 19:54 ` [PATCH v4 37/39] ci: run tests with SHA-256 brian m. carlson
2020-07-26 23:54   ` Eric Sunshine
2020-07-26 19:54 ` [PATCH v4 38/39] docs: add documentation for extensions.objectFormat brian m. carlson
2020-07-26 23:56   ` Eric Sunshine
2020-07-26 19:54 ` [PATCH v4 39/39] t: remove test_oid_init in tests brian m. carlson

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=20200726225910.GD6540@camp.crustytoothpaste.net \
    --to=sandals@crustytoothpaste.net \
    --cc=git@vger.kernel.org \
    --cc=sunshine@sunshineco.com \
    --subject='Re: [PATCH v4 31/39] bundle: add new version for use with SHA-256' \
    /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).