From: Eric Sunshine <sunshine@sunshineco.com>
To: "brian m. carlson" <sandals@crustytoothpaste.net>
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 18:18:23 -0400 [thread overview]
Message-ID: <CAPig+cR+e9XGNCgtDMHUsaAgbKDi=-bztwtd64fVZj7S05ppKQ@mail.gmail.com> (raw)
In-Reply-To: <20200726195424.626969-32-sandals@crustytoothpaste.net>
On Sun, Jul 26, 2020 at 3:56 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> [...]
> Since we cannot extend the v2 format in a backward-compatible way, let's
> add a v3 format, which is identical, except for the addition of
> capabilities, which are prefixed by an at sign. We add "object-format"
> as the only capability and reject unknown capabilities, since we do not
> have a network connection and therefore cannot negotiate with the other
> side.
> [...]
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> diff --git a/bundle.c b/bundle.c
> @@ -23,15 +32,30 @@ static void add_to_ref_list(const struct object_id *oid, const char *name,
> +static int parse_capability(struct bundle_header *header, const char *capability)
> {
> + const char *arg;
> + if (skip_prefix(capability, "object-format=", &arg)) {
> + int algo = hash_algo_by_name(arg);
> + if (algo == GIT_HASH_UNKNOWN)
> + return error(_("unable to detect hash algorithm"));
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.
> @@ -57,21 +83,21 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
> + if (header->version == 3 && *buf.buf == '@') {
> + buf.buf[--buf.len] = '\0';
The reader has to think through this unconditional NUL-termination
more carefully than would be the case if...
> + if (parse_capability(header, buf.buf + 1)) {
> + status = -1;
> + break;
> + }
> + continue;
> + }
> +
> if (*buf.buf == '-') {
> is_prereq = 1;
> strbuf_remove(&buf, 0, 1);
> }
> strbuf_rtrim(&buf);
... you just moved this strbuf_rtrim() call above the capability check
conditional.
> @@ -449,13 +475,14 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
> + int default_version = the_hash_algo == &hash_algos[GIT_HASH_SHA1] ? 2 : 3;
> @@ -464,8 +491,22 @@ int create_bundle(struct repository *r, const char *path,
> + if (version == -1)
> + version = default_version;
> +
> + if (version < 2 || version > 3) {
> + die(_("unsupported bundle version %d"), version);
> + } else if (version < default_version) {
> + die(_("cannot write bundle version %d with algorithm %s"), version, the_hash_algo->name);
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.
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> @@ -281,15 +281,20 @@ test_expect_success 'create bundle 1' '
> test_expect_success 'header of bundle looks right' '
> head -n 4 "$D"/bundle1 &&
> - head -n 1 "$D"/bundle1 | grep "^#" &&
> - head -n 2 "$D"/bundle1 | grep "^-$OID_REGEX " &&
> - head -n 3 "$D"/bundle1 | grep "^$OID_REGEX " &&
> - head -n 4 "$D"/bundle1 | grep "^$"
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?
> + cat >expect <<-EOF &&
> + # v3 git bundle
> + @object-format=$(test_oid algo)
> + -OID message
> + OID message
> +
> + EOF
> + sed -e "s/$OID_REGEX .*/OID message/g" -e "5q" "$D"/bundle1 >actual &&
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.
next prev parent reply other threads:[~2020-07-26 22:18 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 [this message]
2020-07-26 22:59 ` brian m. carlson
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAPig+cR+e9XGNCgtDMHUsaAgbKDi=-bztwtd64fVZj7S05ppKQ@mail.gmail.com' \
--to=sunshine@sunshineco.com \
--cc=git@vger.kernel.org \
--cc=sandals@crustytoothpaste.net \
/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 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 NNTP newsgroup(s).