Re: [PATCH v4 31/39] bundle: add new version for use with SHA-256

[Eric Sunshine <sunshine@sunshineco.com>]

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.