git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jul 13, 2020, 04:26 UTC
Message-ID
<xmqqeepgoxan.fsf@gitster.c.googlers.com>
In-Reply-To
<20200713024909.3714837-32-sandals@crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 19 quoted lines
> Currently we detect the hash algorithm in use by the length of the
> object ID.  This is inelegant and prevents us from using a different
> hash algorithm that is also 256 bits in length.
>
> 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.
>
> For compatibility, default to the v2 format for SHA-1 and require v3
> for SHA-256.
>
> In t5510, always use format v3 so we can be sure we produce consistent
> results across hash algorithms.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
Wow, that was quick ;-)
> +capability   = "@" key ["=" value] LF
> +key          = 1*(ALPHA / DIGIT / "-")
> +value        = *(%01-09 / %0b-FF)
OK.
Show 25 quoted lines
> --- a/builtin/bundle.c
> +++ b/builtin/bundle.c
> @@ -60,6 +60,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
>  	int all_progress_implied = 0;
>  	int progress = isatty(STDERR_FILENO);
>  	struct argv_array pack_opts;
> +	int version = -1;
>  
>  	struct option options[] = {
>  		OPT_SET_INT('q', "quiet", &progress,
> @@ -71,6 +72,8 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
>  		OPT_BOOL(0, "all-progress-implied",
>  			 &all_progress_implied,
>  			 N_("similar to --all-progress when progress meter is shown")),
> +		OPT_INTEGER(0, "version", &version,
> +			    N_("specify bundle format version")),
>  		OPT_END()
>  	};
>  	const char* bundle_file;
> @@ -91,7 +94,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
>  
>  	if (!startup_info->have_repository)
>  		die(_("Need a repository to create a bundle."));
> -	return !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts);
> +	return !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);

OK, so the builtin command itself does not know how the default version is picked; it is left to the code in bundle.c which is at a lower layer. Makes sense.

Show 30 quoted lines
> diff --git a/bundle.c b/bundle.c
> index 2a0d744d3f..5636ea5f7c 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -12,7 +12,8 @@
>  #include "refs.h"
>  #include "argv-array.h"
>  
> -static const char bundle_signature[] = "# v2 git bundle\n";
> +static const char v2_bundle_signature[] = "# v2 git bundle\n";
> +static const char v3_bundle_signature[] = "# v3 git bundle\n";
>  
>  static void add_to_ref_list(const struct object_id *oid, const char *name,
>  		struct ref_list *list)
> @@ -23,17 +24,20 @@ static void add_to_ref_list(const struct object_id *oid, const char *name,
>  	list->nr++;
>  }
>  
> -static const struct git_hash_algo *detect_hash_algo(struct strbuf *buf)
> +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"));
> +		header->hash_algo = &hash_algos[algo];
> +		return 0;
> +	}
> +	return error(_("unknown capability '%s'"), capability);

This certainly enforces the earlier EBNF we saw on the value side by the caller stopping at the first LF and feeding the bytes before it to the function, but there is no sanity checking for the key part.

In a sense, there is no need to. As we always reject what is unknown to us, the only thing we need is to ensure that skip_prefix() we use here are the syntactically correct key followed by '='.

Show 6 quoted lines
>  }
>  
> +
>  static int parse_bundle_header(int fd, struct bundle_header *header,
>  			       const char *report_path)
>  {

I do not particularly mind two blank lines between functions but let's be consistent, and this patch is not the place to make that change.

Show 14 quoted lines
> @@ -42,14 +46,18 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
>  
>  	/* The bundle header begins with the signature */
>  	if (strbuf_getwholeline_fd(&buf, fd, '\n') ||
> -	    strcmp(buf.buf, bundle_signature)) {
> +	    (strcmp(buf.buf, v2_bundle_signature) &&
> +	     strcmp(buf.buf, v3_bundle_signature))) {
>  		if (report_path)
> -			error(_("'%s' does not look like a v2 bundle file"),
> +			error(_("'%s' does not look like a v2 or v3 bundle file"),
>  			      report_path);
>  		status = -1;
>  		goto abort;
>  	}
OK.
> +	header->version = buf.buf[3] - '0';

Hmmmmm. So in addition to the vN_bundle_signature[], the code also knows where the '2' and '3' are? It may be overkill to have a helper function just for this but

	if (strbuf_getwholeline_fd(&buf, fd, '\n') ||
	    parse_bundle_signature(buf.buf, header)) {
		... does not look like a supported bundle file ...
	}
with
	static struct {
		int version;
	        const char *signature;
	} bundle_sigs[] = {
		{ 2, "# v2 git bundle\n" },
		{ 3, "# v3 git bundle\n" },
	};
	static int parse_bundle_signature(const char *s, struct bundle_header *h)
	{
		int i;
                for (i = 0; i < ARRAY_SIZE(bundle_sigs); i++) {
			if (!strcmp(s, bundle_sigs[i].signature) {
				h->version = bundle_sigs[i].version;
				return 0;
			}
		}
		return -1;
	}
or something like that?
Show 11 quoted lines
> +	header->hash_algo = the_hash_algo;
> +
>  	/* The bundle header ends with an empty line */
>  	while (!strbuf_getwholeline_fd(&buf, fd, '\n') &&
>  	       buf.len && buf.buf[0] != '\n') {
> @@ -57,21 +65,21 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
>  		int is_prereq = 0;
>  		const char *p;
>  
> +		if (header->version == 3 && *buf.buf == '@') {
> +			buf.buf[buf.len - 1] = '\0';

This is chomping the final LF. I know the current code does not care, but after this, buf.len is inconsistent with buf.buf in that the NUL we placed here is not part of the payload on the line. Doing strbuf_rtrim(&buf) is certainly too much (it also drops trailing whitespaces), but it somewhat feels disturbing. It might be sufficient to say

			buf.buf[--buf.len] = '\0';
to make it palatable.  I dunno.
Show 63 quoted lines
> +			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);
>  
> -		if (!header->hash_algo) {
> -			header->hash_algo = detect_hash_algo(&buf);
> -			if (!header->hash_algo) {
> -				error(_("unknown hash algorithm length"));
> -				status = -1;
> -				break;
> -			}
> -		}
> -
>  		/*
>  		 * Tip lines have object name, SP, and refname.
>  		 * Prerequisites have object name that is optionally
> @@ -449,13 +457,14 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
>  }
>  
>  int create_bundle(struct repository *r, const char *path,
> -		  int argc, const char **argv, struct argv_array *pack_options)
> +		  int argc, const char **argv, struct argv_array *pack_options, int version)
>  {
>  	struct lock_file lock = LOCK_INIT;
>  	int bundle_fd = -1;
>  	int bundle_to_stdout;
>  	int ref_count = 0;
>  	struct rev_info revs;
> +	int default_version = the_hash_algo == &hash_algos[GIT_HASH_SHA1] ? 2 : 3;
>  
>  	bundle_to_stdout = !strcmp(path, "-");
>  	if (bundle_to_stdout)
> @@ -464,8 +473,22 @@ int create_bundle(struct repository *r, const char *path,
>  		bundle_fd = hold_lock_file_for_update(&lock, path,
>  						      LOCK_DIE_ON_ERROR);
>  
> -	/* write signature */
> -	write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
> +	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);
> +	} else if (version == 2) {
> +		write_or_die(bundle_fd, v2_bundle_signature, strlen(v2_bundle_signature));
> +	} else {
> +		const char *capability = "@object-format=";
> +		write_or_die(bundle_fd, v3_bundle_signature, strlen(v3_bundle_signature));
> +		write_or_die(bundle_fd, capability, strlen(capability));
> +		write_or_die(bundle_fd, the_hash_algo->name, strlen(the_hash_algo->name));
> +		write_or_die(bundle_fd, "\n", 1);
> +	}
Quite straight-forward.
Show 23 quoted lines
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index a66dbe0bde..9284b64e7e 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -281,15 +281,16 @@ test_expect_success 'create bundle 1' '
>  	cd "$D" &&
>  	echo >file updated again by origin &&
>  	git commit -a -m "tip" &&
> -	git bundle create bundle1 master^..master
> +	git bundle create --version=3 bundle1 master^..master
>  '
>  
>  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 "^$"
> +	head -n 2 "$D"/bundle1 | grep "^@object-format=" &&
> +	head -n 3 "$D"/bundle1 | grep "^-$OID_REGEX " &&
> +	head -n 4 "$D"/bundle1 | grep "^$OID_REGEX " &&
> +	head -n 5 "$D"/bundle1 | grep "^$"
>  '

This is not new, but because "head -n $n" is not "look at the $n-th line", but "look at the first $n lines", the construct look somewhat bogus. Wouldn't we want something more like

	sed -n -e 1p "$D/bundle1" | grep "^#" &&
	sed -n -e 2p "$D/bundle1" | grep "^@object-format=" &&
	...
I wonder.
> +test_expect_success 'git bundle uses expected default format' '
> +	git bundle create bundle HEAD^.. &&
> +	head -n1 bundle | grep "^# v$(test_oid version) git bundle$"
> +'
Nice.
Show 6 quoted lines
> +test_expect_success 'git bundle v3 has expected contents' '
> +	git branch side HEAD &&
> +	git bundle create --version=3 bundle HEAD^..side &&
> +	head -n2 bundle >actual &&
> +	echo "# v3 git bundle" >expected &&
> +	echo "@object-format=$(test_oid algo)" >>expected &&
Rather than two separate echo,
	cat >expect <<-EOF &&
	# v3 git bundle
	@object-format=$(test_oid algo)
	EOF
might be more direct and easier to see what is expected.
Show 8 quoted lines
> +	test_cmp actual expected &&
> +	git bundle verify bundle
> +'
> +
> +test_expect_success 'git bundle v3 rejects unknown extensions' '
> +	head -n2 bundle >new &&
> +	echo "@unknown=silly" >>new &&
> +	sed "1,2d" >>new &&
What are we feeding this sed?  You meant to feed 'bundle'?
Show 5 quoted lines
> +	test_must_fail git bundle verify new 2>output &&
> +	grep "unknown capability .unknown=silly." output
> +'
> +
>  test_done
Previous: brian m. carlsonNext: SZEDER Gábor
Message 40 of 44 in “SHA-256, part 3/3”
  1. 00/39 SHA-256, part 3/3brian m. carlson, Jul 13, 2020
  2. 01/39 t: make test-bloom initialize repositorybrian m. carlson, Jul 13, 2020
  3. 09/39 t6501: avoid hard-coded objectsbrian m. carlson, Jul 13, 2020
  4. 10/39 t7003: compute appropriate length constantbrian m. carlson, Jul 13, 2020
  5. 14/39 t7400: make hash size independentbrian m. carlson, Jul 13, 2020
  6. 06/39 t6101: make hash size independentbrian m. carlson, Jul 13, 2020
  7. 18/39 t8002: make hash size independentbrian m. carlson, Jul 13, 2020
  8. 11/39 t7063: make hash size independentbrian m. carlson, Jul 13, 2020
  9. 17/39 t7508: use $ZERO_OID instead of hard-coded constantbrian m. carlson, Jul 13, 2020
  10. 16/39 t7506: avoid checking for SHA-1-specific constantsbrian m. carlson, Jul 13, 2020
  11. 12/39 t7201: abstract away SHA-1-specific constantsbrian m. carlson, Jul 13, 2020
  12. 02/39 t1001: use $ZERO_OIDbrian m. carlson, Jul 13, 2020
  13. 21/39 t9300: abstract away SHA-1-specific constantsbrian m. carlson, Jul 13, 2020
  14. 22/39 t9300: use $ZERO_OID instead of hard-coded object IDbrian m. carlson, Jul 13, 2020
  15. 19/39 t8003: make hash size independentbrian m. carlson, Jul 13, 2020
  16. 37/39 ci: run tests with SHA-256brian m. carlson, Jul 13, 2020
  17. 38/39 docs: add documentation for extensions.objectFormatbrian m. carlson, Jul 13, 2020
  18. 39/39 t: remove test_oid_init in testsbrian m. carlson, Jul 13, 2020
  19. 07/39 t6301: make hash size independentbrian m. carlson, Jul 13, 2020
  20. 03/39 t3305: make hash agnosticbrian m. carlson, Jul 13, 2020
  21. 05/39 t6100: make hash size independentbrian m. carlson, Jul 13, 2020
  22. 04/39 t3404: prepare 'short SHA-1 collision' tests for SHA-256brian m. carlson, Jul 13, 2020
  23. 34/39 t: add test_oid option to select hash algorithmbrian m. carlson, Jul 13, 2020
  24. 29/39 http-fetch: set up git directory before parsing pack hashesbrian m. carlson, Jul 13, 2020
  25. 08/39 t6500: specify test values for SHA-256brian m. carlson, Jul 13, 2020
  26. 15/39 t7405: make hash size independentbrian m. carlson, Jul 13, 2020
  27. 33/39 Enable SHA-256 support by defaultbrian m. carlson, Jul 13, 2020
  28. 13/39 t7102: abstract away SHA-1-specific constantsbrian m. carlson, Jul 13, 2020
  29. 24/39 t9350: make hash size independentbrian m. carlson, Jul 13, 2020
  30. 25/39 t9500: ensure that algorithm info is preserved in configbrian m. carlson, Jul 13, 2020
  31. 20/39 t8011: make hash size independentbrian m. carlson, Jul 13, 2020
  32. 26/39 t9700: make hash size independentbrian m. carlson, Jul 13, 2020
  33. 27/39 t5308: make test work with SHA-256brian m. carlson, Jul 13, 2020
  34. 28/39 t0410: mark test with SHA1 prerequisitebrian m. carlson, Jul 13, 2020
  35. 35/39 t: allow testing different hash algorithms via environmentbrian m. carlson, Jul 13, 2020
  36. 36/39 t: make SHA1 prerequisite depend on default hashbrian m. carlson, Jul 13, 2020
  37. 32/39 setup: add support for reading extensions.objectformatbrian m. carlson, Jul 13, 2020
  38. 30/39 builtin/verify-pack: implement an --object-format optionbrian m. carlson, Jul 13, 2020
  39. 31/39 bundle: add new version for use with SHA-256brian m. carlson, Jul 13, 2020
  40. Junio C HamanoJul 13, 2020
  41. SZEDER GáborJul 13, 2020
  42. Johannes SchindelinJul 15, 2020
  43. Junio C HamanoJul 15, 2020
  44. 23/39 t9301: make hash size independentbrian m. carlson, Jul 13, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.