On Sun, Jul 26, 2020 at 3:56 PM brian m. carlson
<sandals@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
> ---
> 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.
