On Tue, Jul 28, 2020 at 7:36 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
> @@ -57,19 +83,19 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
> - if (*buf.buf == '-') {
> - is_prereq = 1;
> - strbuf_remove(&buf, 0, 1);
> - }
> strbuf_rtrim(&buf);
>
> - if (!header->hash_algo) {
> - header->hash_algo = detect_hash_algo(&buf);
> - if (!header->hash_algo) {
> - error(_("unknown hash algorithm length"));
> + if (header->version == 3 && *buf.buf == '@') {
> + if (parse_capability(header, buf.buf + 1)) {
> status = -1;
> break;
> }
> + continue;
> + }
> +
> + if (*buf.buf == '-') {
> + is_prereq = 1;
> + strbuf_remove(&buf, 0, 1);
> }
Moving the strbuf_rtrim() earlier in the loop, as suggested in my
previous review, made the diff a lot noisier, uglier, and more
difficult to read, however, the code itself ends up being easier to
reason about than in the previous round. Good.
By the way (I didn't think of this in my previous review), but
wouldn't it be better for this:
if (header->version == 3 && *buf.buf == '@') {
to instead be written as:
if (header->version >= 3 && *buf.buf == '@') {
to future-proof it since versions beyond 3 will also almost certainly
support "@foo" capabilities?
> @@ -449,13 +475,14 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
> + int min_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 = min_version;
> +
> + if (version < 2 || version > 3) {
> + die(_("unsupported bundle version %d"), version);
> + } else if (version < min_version) {
> + die(_("cannot write bundle version %d with algorithm %s"), version, the_hash_algo->name);
> + } else if (version == 2) {
> + write_or_die(bundle_fd, v2_bundle_signature, strlen(v2_bundle_signature));
This looks better than the previous attempt; less likely to be fragile
as new bundle versions are introduced. Good.
