[PATCH 00/10] pack v4 UI support

[PATCH 00/10] pack v4 UI support

[Nguyễn Thái Ngọc Duy]

This adds

 - clone, fetch and repack learn --pack-version (backends also learn
   new options, check out the patches for details)

 - core.preferredPackVersion to specify the default pack version for
   fetch, clone, repack and the receiver of push.
   
   This config is supposed to be used by porcelain commands only (with
   the exception of receive-pack). All other plumbing will default to
   pack v2 forever.

 - git and remote helper protocols learn about packv4 capabilities

I think the UI changes for pack v4 are done with this series. Well,
git-bundle needs --pack-version too but it's not part of core
user interaction. The remaining work for v4 would be optimization
(that includes multi-base tree support) and more v4 tests. Let me know
if I missed anything here.

I'm aware that repack is being rewritten in C, so it'll cause
conflicts when np/pack-v4 is re-pulled to 'pu' again. I suggest that
the new tests in t7700 are marked failed at the merge. We can add
--pack-version to the C-based repack and enable the tests later.

About the "packv4" capability in git protocol. I considered a more
generic cap "packver=ver[,ver[,ver...]]" that represents all supported
versions, with the order of preference from the first ver (most preferred)
to the last (least preferred). But it adds more parsing code
and frankly I don't think we'll have pack v5 in the next five years.
So I dropped it.

Multi-base tree support is not part of "packv4" capability. Let's see
if such support comes before the series is merged to master. If so we
can drop that line from protocol-capabilities.txt. Otherwise a new
capability can be added for multi-base trees.

Another capability could be added for sending the actual number of
objects in a thin pack for more accurate display in index-pack. Low
priority in my opinion.

There's also the pack conversion issue. Suppose the client requests v4
but, for performance purposes, the server sends v2. Should index-pack
convert the received pack to v4? My answer is no because there's no
way we can do it without saving v2 first. And if we have to save v2
anyway, pack-objects (or repack) will do the conversion better. We can
adjust git-gc to maybe repack more often when the number of v2 packs
is over a limit, or just repack v2 packs into one v4 pack.

Nguyễn Thái Ngọc Duy (10):
  test-dump: new test program to examine binary data
  config: add core.preferredPackVersion
  upload-pack: new capability to send pack v4
  fetch: new option to set preferred pack version for transfer
  clone: new option to set preferred pack version for transfer
  fetch: pack v4 support on smart http
  receive-pack: request for packv4 if it's the preferred version
  send-pack: support pack v4
  repack: add --pack-version and fall back to core.preferredPackVersion
  count-objects: report pack v4 usage

 .gitignore                                        |  1 +
 Documentation/config.txt                          |  5 ++++
 Documentation/fetch-options.txt                   |  5 ++++
 Documentation/git-clone.txt                       |  4 +++
 Documentation/git-count-objects.txt               |  4 +++
 Documentation/git-fetch-pack.txt                  |  4 +++
 Documentation/git-repack.txt                      |  6 +++-
 Documentation/gitremote-helpers.txt               |  3 ++
 Documentation/technical/protocol-capabilities.txt | 14 +++++++++
 Makefile                                          |  1 +
 builtin/clone.c                                   | 19 ++++++++++++
 builtin/count-objects.c                           | 23 +++++++++++++--
 builtin/fetch-pack.c                              |  7 +++++
 builtin/fetch.c                                   | 10 +++++++
 builtin/receive-pack.c                            |  3 +-
 cache.h                                           |  1 +
 config.c                                          |  5 ++++
 environment.c                                     |  1 +
 fetch-pack.c                                      |  3 ++
 fetch-pack.h                                      |  1 +
 git-repack.sh                                     |  8 +++++-
 remote-curl.c                                     | 14 ++++++++-
 send-pack.c                                       |  5 ++++
 send-pack.h                                       |  1 +
 t/t5510-fetch.sh                                  | 13 +++++++++
 t/t5516-fetch-push.sh                             | 12 ++++++++
 t/t5551-http-fetch.sh                             |  9 ++++++
 t/t5601-clone.sh                                  | 12 ++++++++
 t/t7700-repack.sh                                 | 35 +++++++++++++++++++++++
 test-dump.c (new)                                 | 27 +++++++++++++++++
 transport-helper.c                                |  3 ++
 transport.c                                       |  4 +++
 transport.h                                       |  5 ++++
 upload-pack.c                                     | 16 +++++++++--
 34 files changed, 274 insertions(+), 10 deletions(-)
 create mode 100644 test-dump.c

-- 
1.8.2.82.gc24b958

[PATCH 01/10] test-dump: new test program to examine binary data

[Nguyễn Thái Ngọc Duy]

This will come handy for verifying binary files like .pack, .idx or
$GIT_DIR/index. For now the only supported command is "ntohl". This
command looks into the given file at the given offset, do ntohl() and
return the value in decimal.

More commands may be added later to decode varint, or decompress a
zlib stream and so on...

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 .gitignore        |  1 +
 Makefile          |  1 +
 test-dump.c (new) | 27 +++++++++++++++++++++++++++
 3 files changed, 29 insertions(+)
 create mode 100644 test-dump.c

diff --git a/.gitignore b/.gitignore
index 2e2ae2b..6d835e2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -182,6 +182,7 @@
 /test-ctype
 /test-date
 /test-delta
+/test-dump
 /test-dump-cache-tree
 /test-scrap-cache-tree
 /test-genrandom
diff --git a/Makefile b/Makefile
index af2e3e3..a07a194 100644
--- a/Makefile
+++ b/Makefile
@@ -560,6 +560,7 @@ TEST_PROGRAMS_NEED_X += test-chmtime
 TEST_PROGRAMS_NEED_X += test-ctype
 TEST_PROGRAMS_NEED_X += test-date
 TEST_PROGRAMS_NEED_X += test-delta
+TEST_PROGRAMS_NEED_X += test-dump
 TEST_PROGRAMS_NEED_X += test-dump-cache-tree
 TEST_PROGRAMS_NEED_X += test-genrandom
 TEST_PROGRAMS_NEED_X += test-index-version
diff --git a/test-dump.c b/test-dump.c
new file mode 100644
index 0000000..71c6f8f
--- /dev/null
+++ b/test-dump.c
@@ -0,0 +1,27 @@
+#include "cache.h"
+
+static inline uint32_t ntoh_l_force_align(void *p)
+{
+	uint32_t x;
+	memcpy(&x, p, sizeof(x));
+	return ntohl(x);
+}
+
+int main(int ac, char **av)
+{
+	unsigned char *p;
+	int fd = open(av[2], O_RDONLY);
+	struct stat st;
+	if (lstat(av[2], &st))
+		return 1;
+	p = mmap(0, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	if (p == (unsigned char*)MAP_FAILED)
+		return 2;
+	if (!strcmp(av[1], "ntohl"))
+		printf("%u\n", ntoh_l_force_align(p + atoi(av[3])));
+	else {
+		fprintf(stderr, "unknown command %s\n", av[1]);
+		return 3;
+	}
+	return 0;
+}
-- 
1.8.2.82.gc24b958

[PATCH 02/10] config: add core.preferredPackVersion

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt | 5 +++++
 cache.h                  | 1 +
 config.c                 | 5 +++++
 environment.c            | 1 +
 4 files changed, 12 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ec57a15..6e7037b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -644,6 +644,11 @@ core.abbrev::
 	for abbreviated object names to stay unique for sufficiently long
 	time.
 
+core.preferredPackVersion::
+	This is the preferred pack format version for various operations
+	that may produce pack files such as clone, fetch, push or repack.
+	Valid values are 2 and 4. The default value is 2.
+
 add.ignore-errors::
 add.ignoreErrors::
 	Tells 'git add' to continue adding files when some files cannot be
diff --git a/cache.h b/cache.h
index b68ad5a..20c2d6d 100644
--- a/cache.h
+++ b/cache.h
@@ -597,6 +597,7 @@ extern int fsync_object_files;
 extern int core_preload_index;
 extern int core_apply_sparse_checkout;
 extern int precomposed_unicode;
+extern int core_default_pack_version;
 
 /*
  * The character that begins a commented line in user-editable file
diff --git a/config.c b/config.c
index e13a7b6..02af0d1 100644
--- a/config.c
+++ b/config.c
@@ -831,6 +831,11 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.preferredpackversion")) {
+		core_default_pack_version = git_config_int(var, value);
+		return 0;
+	}
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/environment.c b/environment.c
index 5398c36..24c43ba 100644
--- a/environment.c
+++ b/environment.c
@@ -62,6 +62,7 @@ int merge_log_config = -1;
 int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
 struct startup_info *startup_info;
 unsigned long pack_size_limit_cfg;
+int core_default_pack_version = 2;
 
 /*
  * The character that begins a commented line in user-editable file
-- 
1.8.2.82.gc24b958

[PATCH 03/10] upload-pack: new capability to send pack v4

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/technical/protocol-capabilities.txt | 10 ++++++++++
 upload-pack.c                                     | 16 +++++++++++++---
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index fd8ffa5..be09792 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -217,3 +217,13 @@ allow-tip-sha1-in-want
 If the upload-pack server advertises this capability, fetch-pack may
 send "want" lines with SHA-1s that exist at the server but are not
 advertised by upload-pack.
+
+packv4
+------
+
+The upload-server can generate .pack version 4. If the client accepts
+this capability, the server may send a pack version 4. The server can
+choose to send pack version 2 even if the client accepts this
+capability.
+
+This capability does not include multi-base tree support.
diff --git a/upload-pack.c b/upload-pack.c
index ccf76d9..291d366 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -32,7 +32,7 @@ static unsigned long oldest_have;
 
 static int multi_ack;
 static int no_done;
-static int use_thin_pack, use_ofs_delta, use_include_tag;
+static int use_thin_pack, use_ofs_delta, use_include_tag, packv4_ok;
 static int no_progress, daemon_mode;
 static int allow_tip_sha1_in_want;
 static int shallow_nr;
@@ -140,7 +140,7 @@ static void create_pack_file(void)
 		"corruption on the remote side.";
 	int buffered = -1;
 	ssize_t sz;
-	const char *argv[10];
+	const char *argv[11];
 	int arg = 0;
 
 	argv[arg++] = "pack-objects";
@@ -157,6 +157,14 @@ static void create_pack_file(void)
 		argv[arg++] = "--delta-base-offset";
 	if (use_include_tag)
 		argv[arg++] = "--include-tag";
+	/*
+	 * The client may have requested pack v4. But if this is a v2
+	 * repo on a busy machine, we may want to send v2 and let the
+	 * client do the v2->v4 conversion to reduce the cost on the
+	 * server side.
+	 */
+	if (packv4_ok)
+		argv[arg++] = "--version=4";
 	argv[arg++] = NULL;
 
 	memset(&pack_objects, 0, sizeof(pack_objects));
@@ -633,6 +641,8 @@ static void receive_needs(void)
 			no_progress = 1;
 		if (parse_feature_request(features, "include-tag"))
 			use_include_tag = 1;
+		if (parse_feature_request(features, "packv4"))
+			packv4_ok = 1;
 
 		o = parse_object(sha1_buf);
 		if (!o)
@@ -738,7 +748,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 {
 	static const char *capabilities = "multi_ack thin-pack side-band"
 		" side-band-64k ofs-delta shallow no-progress"
-		" include-tag multi_ack_detailed";
+		" include-tag multi_ack_detailed packv4";
 	const char *refname_nons = strip_namespace(refname);
 	unsigned char peeled[20];
 
-- 
1.8.2.82.gc24b958

[PATCH 04/10] fetch: new option to set preferred pack version for transfer

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/fetch-options.txt |  5 +++++
 builtin/fetch.c                 | 10 ++++++++++
 fetch-pack.c                    |  3 +++
 fetch-pack.h                    |  1 +
 t/t5510-fetch.sh                | 13 +++++++++++++
 transport.c                     |  4 ++++
 transport.h                     |  5 +++++
 7 files changed, 41 insertions(+)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index ba1fe49..47d55e5 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -17,6 +17,11 @@
 	Convert a shallow repository to a complete one, removing all
 	the limitations imposed by shallow repositories.
 
+--pack-version=<n>::
+	Define the preferred pack format version for data transfer.
+	Valid values are 2 and 4. Default value is specified by
+	core.preferredPackVersion setting. See linkgit:git-config[1].
+
 ifndef::git-pull[]
 --dry-run::
 	Show what would be done, without making any changes.
diff --git a/builtin/fetch.c b/builtin/fetch.c
index d784b2e..695fbf1 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -31,6 +31,7 @@ enum {
 };
 
 static int all, append, dry_run, force, keep, multiple, prune, update_head_ok, verbosity;
+static int pack_version;
 static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int tags = TAGS_DEFAULT, unshallow;
 static const char *depth;
@@ -90,6 +91,8 @@ static struct option builtin_fetch_options[] = {
 	{ OPTION_STRING, 0, "recurse-submodules-default",
 		   &recurse_submodules_default, NULL,
 		   N_("default mode for recursion"), PARSE_OPT_HIDDEN },
+	OPT_INTEGER(0, "pack-version", &pack_version,
+		    N_("preferred pack version for transfer")),
 	OPT_END()
 };
 
@@ -957,6 +960,8 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)
 		set_option(TRANS_OPT_KEEP, "yes");
 	if (depth)
 		set_option(TRANS_OPT_DEPTH, depth);
+	if (pack_version == 4)
+		set_option(TRANS_OPT_PACKV4, "yes");
 
 	if (argc > 0) {
 		int j = 0;
@@ -1010,6 +1015,11 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix,
 			     builtin_fetch_options, builtin_fetch_usage, 0);
 
+	if (!pack_version)
+		pack_version = core_default_pack_version;
+	if (pack_version != 2 && pack_version != 4)
+		die(_("invalid pack version %d"), pack_version);
+
 	if (unshallow) {
 		if (depth)
 			die(_("--depth and --unshallow cannot be used together"));
diff --git a/fetch-pack.c b/fetch-pack.c
index 6684348..16aa3d0 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -324,6 +324,7 @@ static int find_common(struct fetch_pack_args *args,
 			if (args->no_progress)   strbuf_addstr(&c, " no-progress");
 			if (args->include_tag)   strbuf_addstr(&c, " include-tag");
 			if (prefer_ofs_delta)   strbuf_addstr(&c, " ofs-delta");
+			if (args->packv4)       strbuf_addstr(&c, " packv4");
 			if (agent_supported)    strbuf_addf(&c, " agent=%s",
 							    git_user_agent_sanitized());
 			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
@@ -869,6 +870,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 		args->no_progress = 0;
 	if (!server_supports("include-tag"))
 		args->include_tag = 0;
+	if (!server_supports("packv4"))
+		args->packv4 = 0;
 	if (server_supports("ofs-delta")) {
 		if (args->verbose)
 			fprintf(stderr, "Server supports ofs-delta\n");
diff --git a/fetch-pack.h b/fetch-pack.h
index 40f08ba..5f03739 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -17,6 +17,7 @@ struct fetch_pack_args {
 		no_progress:1,
 		include_tag:1,
 		stateless_rpc:1,
+		packv4:1,
 		check_self_contained_and_connected:1,
 		self_contained_and_connected:1;
 };
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index fde6891..d3da088 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -512,4 +512,17 @@ test_expect_success 'all boundary commits are excluded' '
 	test_bundle_object_count .git/objects/pack/pack-${pack##pack	}.pack 3
 '
 
+test_expect_success 'fetch --pack-version=4' '
+	git init pv4 &&
+	(
+		cd pv4 &&
+		git fetch --pack-version=4 --keep file://"$D"/.git &&
+		P=`ls .git/objects/pack/pack-*.pack` &&
+		# Offset 4 is pack version
+		test-dump ntohl "$P" 4 >ver.actual &&
+		echo 4 >ver.expected &&
+		test_cmp ver.expected ver.actual
+	)
+'
+
 test_done
diff --git a/transport.c b/transport.c
index e15db98..ad5a4f1 100644
--- a/transport.c
+++ b/transport.c
@@ -473,6 +473,9 @@ static int set_git_option(struct git_transport_options *opts,
 	} else if (!strcmp(name, TRANS_OPT_KEEP)) {
 		opts->keep = !!value;
 		return 0;
+	} else if (!strcmp(name, TRANS_OPT_PACKV4)) {
+		opts->packv4 = !!value;
+		return 0;
 	} else if (!strcmp(name, TRANS_OPT_DEPTH)) {
 		if (!value)
 			opts->depth = 0;
@@ -534,6 +537,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.quiet = (transport->verbose < 0);
 	args.no_progress = !transport->progress;
 	args.depth = data->options.depth;
+	args.packv4 = data->options.packv4;
 	args.check_self_contained_and_connected =
 		data->options.check_self_contained_and_connected;
 
diff --git a/transport.h b/transport.h
index ea70ea7..59785c0 100644
--- a/transport.h
+++ b/transport.h
@@ -8,6 +8,7 @@ struct git_transport_options {
 	unsigned thin : 1;
 	unsigned keep : 1;
 	unsigned followtags : 1;
+	unsigned packv4 : 1;
 	unsigned check_self_contained_and_connected : 1;
 	unsigned self_contained_and_connected : 1;
 	int depth;
@@ -135,6 +136,10 @@ struct transport *transport_get(struct remote *, const char *);
 /* Aggressively fetch annotated tags if possible */
 #define TRANS_OPT_FOLLOWTAGS "followtags"
 
+/* Prefer pack version 4 as the transport format */
+#define TRANS_OPT_PACKV4 "packv4"
+
+
 /**
  * Returns 0 if the option was used, non-zero otherwise. Prints a
  * message to stderr if the option is not used.
-- 
1.8.2.82.gc24b958

[PATCH 05/10] clone: new option to set preferred pack version for transfer

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-clone.txt |  4 ++++
 builtin/clone.c             | 19 +++++++++++++++++++
 t/t5601-clone.sh            | 12 ++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 450f158..6299a70 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -217,6 +217,10 @@ objects from the source repository into a pack in the cloned repository.
 	The result is Git repository can be separated from working
 	tree.
 
+--pack-version=<n>::
+	Define the preferred pack format version for data transfer.
+	Valid values are 2 and 4. Default value is specified by
+	core.preferredPackVersion setting. See linkgit:git-config[1].
 
 <repository>::
 	The (possibly remote) repository to clone from.  See the
diff --git a/builtin/clone.c b/builtin/clone.c
index 430307b..9c0e296 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -46,6 +46,7 @@ static const char *real_git_dir;
 static char *option_upload_pack = "git-upload-pack";
 static int option_verbosity;
 static int option_progress = -1;
+static int pack_version;
 static struct string_list option_config;
 static struct string_list option_reference;
 
@@ -98,6 +99,8 @@ static struct option builtin_clone_options[] = {
 		   N_("separate git dir from working tree")),
 	OPT_STRING_LIST('c', "config", &option_config, N_("key=value"),
 			N_("set config inside the new repository")),
+	OPT_INTEGER(0, "pack-version", &pack_version,
+		    N_("preferred pack version for transfer")),
 	OPT_END()
 };
 
@@ -764,6 +767,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		usage_msg_opt(_("You must specify a repository to clone."),
 			builtin_clone_usage, builtin_clone_options);
 
+	if (!pack_version)
+		pack_version = core_default_pack_version;
+	if (pack_version != 2 && pack_version != 4)
+		die(_("invalid pack version %d"), pack_version);
+
 	if (option_single_branch == -1)
 		option_single_branch = option_depth ? 1 : 0;
 
@@ -794,6 +802,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	is_local = option_local != 0 && path && !is_bundle;
 	if (is_local && option_depth)
 		warning(_("--depth is ignored in local clones; use file:// instead."));
+	if (is_local && pack_version == 4)
+		warning(_("--pack-version is ignored in local clones; use file:// instead."));
 	if (option_local > 0 && !is_local)
 		warning(_("--local is ignored"));
 
@@ -868,6 +878,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	} else {
 		strbuf_addf(&branch_top, "refs/remotes/%s/", option_origin);
 	}
+	if (pack_version != core_default_pack_version) {
+		struct strbuf sb = STRBUF_INIT;
+		strbuf_addf(&sb, "%d", pack_version);
+		git_config_set("core.preferredPackVersion", sb.buf);
+		strbuf_release(&sb);
+	}
 
 	strbuf_addf(&value, "+%s*:%s*", src_ref_prefix, branch_top.buf);
 	strbuf_addf(&key, "remote.%s.url", option_origin);
@@ -903,6 +919,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			transport_set_option(transport, TRANS_OPT_UPLOADPACK,
 					     option_upload_pack);
 
+		if (pack_version == 4)
+			transport_set_option(transport, TRANS_OPT_PACKV4, "yes");
+
 		if (transport->smart_options && !option_depth)
 			transport->smart_options->check_self_contained_and_connected = 1;
 	}
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 0629149..8118524 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -285,4 +285,16 @@ test_expect_success NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '
 	git clone "./foo:bar" foobar
 '
 
+test_expect_success 'clone --pack-version=4' '
+	git clone --pack-version=4 --no-local src pv4 &&
+	P=`ls pv4/.git/objects/pack/pack-*.pack` &&
+	# Offset 4 is pack version
+	test-dump ntohl "$P" 4 >ver.actual &&
+	echo 4 >ver.expected &&
+	test_cmp ver.expected ver.actual &&
+	git --git-dir=pv4/.git config --int core.preferredPackVersion >cfgver.actual &&
+	echo 4 >cfgver.expected &&
+	test_cmp cfgver.expected cfgver.actual
+'
+
 test_done
-- 
1.8.2.82.gc24b958

[PATCH 06/10] fetch: pack v4 support on smart http

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-fetch-pack.txt    |  4 ++++
 Documentation/gitremote-helpers.txt |  3 +++
 builtin/fetch-pack.c                |  7 +++++++
 remote-curl.c                       | 14 +++++++++++++-
 t/t5551-http-fetch.sh               |  9 +++++++++
 transport-helper.c                  |  3 +++
 6 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
index 1e71754..b68cd45 100644
--- a/Documentation/git-fetch-pack.txt
+++ b/Documentation/git-fetch-pack.txt
@@ -87,6 +87,10 @@ be in a separate packet, and the list must end with a flush packet.
 	'git-upload-pack' treats the special depth 2147483647 as
 	infinite even if there is an ancestor-chain that long.
 
+--pack-version=<n>::
+	Define the preferred pack format version for data transfer.
+	Valid values are 2 and 4. Default is 2.
+
 --no-progress::
 	Do not show the progress.
 
diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index 0827f69..90dfc2e 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -416,6 +416,9 @@ set by Git if the remote helper has the 'option' capability.
 	must not rely on this option being set before
 	connect request occurs.
 
+'option packv4 \{'true'|'false'\}::
+	Prefer pack version 4 (true) or 2 (false) for data transfer.
+
 SEE ALSO
 --------
 linkgit:git-remote[1]
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index aba4465..bfe940a 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -100,6 +100,13 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			pack_lockfile_ptr = &pack_lockfile;
 			continue;
 		}
+		if (!prefixcmp(arg, "--pack-version=")) {
+			int ver = strtol(arg + 15, NULL, 0);
+			if (ver != 2 && ver != 4)
+				die(_("invalid pack version %d"), ver);
+			args.packv4 = ver == 4;
+			continue;
+		}
 		usage(fetch_pack_usage);
 	}
 
diff --git a/remote-curl.c b/remote-curl.c
index 5b3ce9e..1a3d215 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -18,6 +18,7 @@ struct options {
 	unsigned progress : 1,
 		followtags : 1,
 		dry_run : 1,
+		packv4 : 1,
 		thin : 1;
 };
 static struct options options;
@@ -67,6 +68,15 @@ static int set_option(const char *name, const char *value)
 			return -1;
 		return 0;
 	}
+	else if (!strcmp(name, "packv4")) {
+		if (!strcmp(value, "true"))
+			options.packv4 = 1;
+		else if (!strcmp(value, "false"))
+			options.packv4 = 0;
+		else
+			return -1;
+		return 0;
+	}
 	else {
 		return 1 /* unsupported */;
 	}
@@ -654,7 +664,7 @@ static int fetch_git(struct discovery *heads,
 	struct strbuf preamble = STRBUF_INIT;
 	char *depth_arg = NULL;
 	int argc = 0, i, err;
-	const char *argv[15];
+	const char *argv[16];
 
 	argv[argc++] = "fetch-pack";
 	argv[argc++] = "--stateless-rpc";
@@ -676,6 +686,8 @@ static int fetch_git(struct discovery *heads,
 		depth_arg = strbuf_detach(&buf, NULL);
 		argv[argc++] = depth_arg;
 	}
+	if (options.packv4)
+		argv[argc++] = "--pack-version=4";
 	argv[argc++] = url;
 	argv[argc++] = NULL;
 
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index 55a866a..5b4e6aa 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -222,5 +222,14 @@ test_expect_success EXPENSIVE 'clone the 50,000 tag repo to check OS command lin
 	)
 '
 
+test_expect_success 'clone http repository with pack v4' '
+	git -c transfer.unpackLimit=1 clone --pack-version=4 $HTTPD_URL/smart/repo.git pv4 &&
+	P=`ls pv4/.git/objects/pack/pack-*.pack` &&
+	# Offset 4 is pack version
+	test-dump ntohl "$P" 4 >ver.actual &&
+	echo 4 >ver.expected &&
+	test_cmp ver.expected ver.actual
+'
+
 stop_httpd
 test_done
diff --git a/transport-helper.c b/transport-helper.c
index 63cabc3..07fbf6e 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -350,6 +350,9 @@ static int fetch_with_fetch(struct transport *transport,
 
 	standard_options(transport);
 
+	set_helper_option(transport, "packv4",
+			  data->transport_options.packv4 ? "true" : "false");
+
 	for (i = 0; i < nr_heads; i++) {
 		const struct ref *posn = to_fetch[i];
 		if (posn->status & REF_STATUS_UPTODATE)
-- 
1.8.2.82.gc24b958

[PATCH 07/10] receive-pack: request for packv4 if it's the preferred version

[Nguyễn Thái Ngọc Duy]

This is the only plumbing command that is controlled by
core.preferredPackVersion so far.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/technical/protocol-capabilities.txt | 4 ++++
 builtin/receive-pack.c                            | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index be09792..32153cd 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -226,4 +226,8 @@ this capability, the server may send a pack version 4. The server can
 choose to send pack version 2 even if the client accepts this
 capability.
 
+The receive-pack server advertises this capability if it wants to
+receive the pack in format version 4 and the client should send in
+this format.
+
 This capability does not include multi-base tree support.
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e3eb5fc..288b0bc 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -130,10 +130,11 @@ static void show_ref(const char *path, const unsigned char *sha1)
 	if (sent_capabilities)
 		packet_write(1, "%s %s\n", sha1_to_hex(sha1), path);
 	else
-		packet_write(1, "%s %s%c%s%s agent=%s\n",
+		packet_write(1, "%s %s%c%s%s%s agent=%s\n",
 			     sha1_to_hex(sha1), path, 0,
 			     " report-status delete-refs side-band-64k quiet",
 			     prefer_ofs_delta ? " ofs-delta" : "",
+			     core_default_pack_version == 4 ? " packv4" : "",
 			     git_user_agent_sanitized());
 	sent_capabilities = 1;
 }
-- 
1.8.2.82.gc24b958

[PATCH 08/10] send-pack: support pack v4

[Nguyễn Thái Ngọc Duy]

Contrary to the fetch direction, whether send-pack sends packv4 is
totally controlled by the server. This is in favor of lowering load at
the server side. More logic may be added later to allow the client to
stick to v2 even if the server requests v4.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 send-pack.c           |  5 +++++
 send-pack.h           |  1 +
 t/t5516-fetch-push.sh | 12 ++++++++++++
 3 files changed, 18 insertions(+)

diff --git a/send-pack.c b/send-pack.c
index 7d172ef..977c14b 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -44,6 +44,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 		NULL,
 		NULL,
 		NULL,
+		NULL,
 	};
 	struct child_process po;
 	int i;
@@ -57,6 +58,8 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 		argv[i++] = "-q";
 	if (args->progress)
 		argv[i++] = "--progress";
+	if (args->packv4)
+		argv[i++] = "--version=4";
 	memset(&po, 0, sizeof(po));
 	po.argv = argv;
 	po.in = -1;
@@ -205,6 +208,8 @@ int send_pack(struct send_pack_args *args,
 		quiet_supported = 1;
 	if (server_supports("agent"))
 		agent_supported = 1;
+	if (server_supports("packv4"))
+		args->packv4 = 1;
 
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
diff --git a/send-pack.h b/send-pack.h
index 05d7ab1..cda770c 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -11,6 +11,7 @@ struct send_pack_args {
 		use_thin_pack:1,
 		use_ofs_delta:1,
 		dry_run:1,
+		packv4:1,
 		stateless_rpc:1;
 };
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 4691d51..d0c116f 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1172,4 +1172,16 @@ test_expect_success 'push --follow-tag only pushes relevant tags' '
 	test_cmp expect actual
 '
 
+test_expect_success 'push pack v4' '
+	git init pv4 &&
+	git --git-dir pv4/.git config core.preferredPackVersion 4 &&
+	git --git-dir pv4/.git config transfer.unpackLimit 1 &&
+	git push pv4 HEAD:refs/heads/head &&
+	P=`ls pv4/.git/objects/pack/pack-*.pack` &&
+	# Offset 4 is pack version
+	test-dump ntohl "$P" 4 >ver.actual &&
+	echo 4 >ver.expected &&
+	test_cmp ver.expected ver.actual
+'
+
 test_done
-- 
1.8.2.82.gc24b958

[PATCH 09/10] repack: add --pack-version and fall back to core.preferredPackVersion

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-repack.txt |  6 +++++-
 git-repack.sh                |  8 +++++++-
 t/t7700-repack.sh            | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index 4c1aff6..c43eb4a 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -9,7 +9,7 @@ git-repack - Pack unpacked objects in a repository
 SYNOPSIS
 --------
 [verse]
-'git repack' [-a] [-A] [-d] [-f] [-F] [-l] [-n] [-q] [--window=<n>] [--depth=<n>]
+'git repack' [options]
 
 DESCRIPTION
 -----------
@@ -110,6 +110,10 @@ other objects in that pack they already have locally.
 	The default is unlimited, unless the config variable
 	`pack.packSizeLimit` is set.
 
+--pack-version=<version>::
+	Force the version for the generated pack.
+	Valid values are 2 and 4. Default value is specified by
+	core.preferredPackVersion setting. See linkgit:git-config[1].
 
 Configuration
 -------------
diff --git a/git-repack.sh b/git-repack.sh
index 7579331..0d898eb 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -21,12 +21,13 @@ window=         size of the window used for delta compression
 window-memory=  same as the above, but limit memory size instead of entries count
 depth=          limits the maximum delta depth
 max-pack-size=  maximum size of each packfile
+pack-version=   format version of the output pack
 "
 SUBDIRECTORY_OK='Yes'
 . git-sh-setup
 
 no_update_info= all_into_one= remove_redundant= unpack_unreachable=
-local= no_reuse= extra=
+local= no_reuse= extra= packver=
 while test $# != 0
 do
 	case "$1" in
@@ -43,6 +44,8 @@ do
 	-l)	local=--local ;;
 	--max-pack-size|--window|--window-memory|--depth)
 		extra="$extra $1=$2"; shift ;;
+	--pack-version)
+		packver="$2"; shift ;;
 	--) shift; break;;
 	*)	usage ;;
 	esac
@@ -92,6 +95,9 @@ esac
 
 mkdir -p "$PACKDIR" || exit
 
+[ -n "$packver" ] || packver="`git config --int core.preferredPackVersion`"
+[ -n "$packver" ] && args="$args --version=$packver"
+
 args="$args $local ${GIT_QUIET:+-q} $no_reuse$extra"
 names=$(git pack-objects --keep-true-parents --honor-pack-keep --non-empty --all --reflog $args </dev/null "$PACKTMP") ||
 	exit 1
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index d954b84..8383e1b 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -164,5 +164,40 @@ test_expect_success 'objects made unreachable by grafts only are kept' '
 	git cat-file -t $H1
 	'
 
+test_expect_success 'repack respects core.preferredPackVersion' '
+	git init pv4 &&
+	(
+		unset GIT_TEST_PACKV4 &&
+		cd pv4 &&
+		test_commit one &&
+		test_commit two &&
+		test_commit three &&
+		git config core.preferredPackVersion 4 &&
+		git repack -ad &&
+		P=`ls .git/objects/pack/pack-*.pack` &&
+		# Offset 4 is pack version
+		test-dump ntohl "$P" 4 >ver.actual &&
+		echo 4 >ver.expected &&
+		test_cmp ver.expected ver.actual
+	)
+'
+
+test_expect_success 'repack --pack-version=4' '
+	git init pv4.2 &&
+	(
+		unset GIT_TEST_PACKV4 &&
+		cd pv4.2 &&
+		test_commit one &&
+		test_commit two &&
+		test_commit three &&
+		git repack -ad --pack-version=4 &&
+		P=`ls .git/objects/pack/pack-*.pack` &&
+		# Offset 4 is pack version
+		test-dump ntohl "$P" 4 >ver.actual &&
+		echo 4 >ver.expected &&
+		test_cmp ver.expected ver.actual
+	)
+'
+
 test_done
 
-- 
1.8.2.82.gc24b958

[PATCH 10/10] count-objects: report pack v4 usage

[Nguyễn Thái Ngọc Duy]

An ideal repository is v4 only, no v2 packs. Show pack v4 statistics
so people know if the repository is mixed with v2 and repack it. Note
that "in-pack" and "size-pack" include all pack versions, not just v2.

Only display v4 info when there are v4 packs. It's still experimental
so don't polute the output with v4 that's never used by 99.99% of
users. We can make it unconditional later when v4 is officially
supported and recommended.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-count-objects.txt |  4 ++++
 builtin/count-objects.c             | 23 ++++++++++++++++++++---
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt
index b300e84..d15b4dc 100644
--- a/Documentation/git-count-objects.txt
+++ b/Documentation/git-count-objects.txt
@@ -28,8 +28,12 @@ size: disk space consumed by loose objects, in KiB (unless -H is specified)
 +
 in-pack: the number of in-pack objects
 +
+in-packv4: the number of objects in packs version 4
++
 size-pack: disk space consumed by the packs, in KiB (unless -H is specified)
 +
+size-packv4: disk space consumed by the packs version 4
++
 prune-packable: the number of loose objects that are also present in
 the packs. These objects could be pruned using `git prune-packed`.
 +
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index a7f70cb..db73ce7 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -89,7 +89,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 	const char *objdir = get_object_directory();
 	int len = strlen(objdir);
 	char *path = xmalloc(len + 50);
-	unsigned long loose = 0, packed = 0, packed_loose = 0;
+	unsigned long loose = 0, packed_loose = 0;
 	off_t loose_size = 0;
 	struct option opts[] = {
 		OPT__VERBOSE(&verbose, N_("be verbose")),
@@ -119,10 +119,12 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 	}
 	if (verbose) {
 		struct packed_git *p;
-		unsigned long num_pack = 0;
-		off_t size_pack = 0;
+		unsigned long num_pack = 0, num_pack_v4 = 0;
+		unsigned long packed = 0, packed_v4 = 0;
+		off_t size_pack = 0, size_pack_v4 = 0;
 		struct strbuf loose_buf = STRBUF_INIT;
 		struct strbuf pack_buf = STRBUF_INIT;
+		struct strbuf pack_buf_v4 = STRBUF_INIT;
 		struct strbuf garbage_buf = STRBUF_INIT;
 		if (!packed_git)
 			prepare_packed_git();
@@ -134,17 +136,25 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 			packed += p->num_objects;
 			size_pack += p->pack_size + p->index_size;
 			num_pack++;
+			if (p->version == 4) {
+				packed_v4 += p->num_objects;
+				size_pack_v4 += p->pack_size + p->index_size;
+				num_pack_v4++;
+			}
 		}
 
 		if (human_readable) {
 			strbuf_humanise_bytes(&loose_buf, loose_size);
 			strbuf_humanise_bytes(&pack_buf, size_pack);
+			strbuf_humanise_bytes(&pack_buf_v4, size_pack_v4);
 			strbuf_humanise_bytes(&garbage_buf, size_garbage);
 		} else {
 			strbuf_addf(&loose_buf, "%lu",
 				    (unsigned long)(loose_size / 1024));
 			strbuf_addf(&pack_buf, "%lu",
 				    (unsigned long)(size_pack / 1024));
+			strbuf_addf(&pack_buf_v4, "%lu",
+				    (unsigned long)(size_pack_v4 / 1024));
 			strbuf_addf(&garbage_buf, "%lu",
 				    (unsigned long)(size_garbage / 1024));
 		}
@@ -152,13 +162,20 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 		printf("count: %lu\n", loose);
 		printf("size: %s\n", loose_buf.buf);
 		printf("in-pack: %lu\n", packed);
+		if (num_pack_v4)
+			printf("in-packv4: %lu\n", packed_v4);
 		printf("packs: %lu\n", num_pack);
+		if (num_pack_v4)
+			printf("v4-packs: %lu\n", num_pack_v4);
 		printf("size-pack: %s\n", pack_buf.buf);
+		if (num_pack_v4)
+			printf("size-packv4: %s\n", pack_buf_v4.buf);
 		printf("prune-packable: %lu\n", packed_loose);
 		printf("garbage: %lu\n", garbage);
 		printf("size-garbage: %s\n", garbage_buf.buf);
 		strbuf_release(&loose_buf);
 		strbuf_release(&pack_buf);
+		strbuf_release(&pack_buf_v4);
 		strbuf_release(&garbage_buf);
 	} else {
 		struct strbuf buf = STRBUF_INIT;
-- 
1.8.2.82.gc24b958

Re: [PATCH 00/10] pack v4 UI support

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2337 bytes --]

On Thu, 26 Sep 2013, Nguyễn Thái Ngọc Duy wrote:

> About the "packv4" capability in git protocol. I considered a more
> generic cap "packver=ver[,ver[,ver...]]" that represents all supported
> versions, with the order of preference from the first ver (most preferred)
> to the last (least preferred). But it adds more parsing code
> and frankly I don't think we'll have pack v5 in the next five years.
> So I dropped it.

Agreed.  In fact the receiver should only indicate the highest pack 
version it is ready to accept.  We'll have to support the sending and 
receiving of any previous pack versions forever anyway.

> Multi-base tree support is not part of "packv4" capability. Let's see
> if such support comes before the series is merged to master. If so we
> can drop that line from protocol-capabilities.txt. Otherwise a new
> capability can be added for multi-base trees.

What is that for?  Multi-base trees are not created yet, but the code to 
parse them is already there.  So I don't see the point of having a 
capability in the protocol for this.

> Another capability could be added for sending the actual number of
> objects in a thin pack for more accurate display in index-pack. Low
> priority in my opinion.

That just cannot be communicated during capability exchange.  This 
number is known only after object enumeration.  Hence my suggestion of a 
['T', 'H', 'I', 'N', htonl(<number_of_sent_objects>)] special header 
prepended to the actual pack on the wire.  And that has to be decided 
before formalizing the pack v4 capability.  That makes it a somewhat 
higher priority.

> There's also the pack conversion issue. Suppose the client requests v4
> but, for performance purposes, the server sends v2. Should index-pack
> convert the received pack to v4? My answer is no because there's no
> way we can do it without saving v2 first. And if we have to save v2
> anyway, pack-objects (or repack) will do the conversion better. We can
> adjust git-gc to maybe repack more often when the number of v2 packs
> is over a limit, or just repack v2 packs into one v4 pack.

Yeah... those are questions that certainly can wait.  Only 
experimentation will tell what is best, and that won't be conclusive 
until the core git code learns to parse pack v4 data natively.

I'll try to review your patches soon.


Nicolas

Re: [PATCH 00/10] pack v4 UI support

[Duy Nguyen]

On Thu, Sep 26, 2013 at 11:51 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
>> Multi-base tree support is not part of "packv4" capability. Let's see
>> if such support comes before the series is merged to master. If so we
>> can drop that line from protocol-capabilities.txt. Otherwise a new
>> capability can be added for multi-base trees.
>
> What is that for?  Multi-base trees are not created yet, but the code to
> parse them is already there.  So I don't see the point of having a
> capability in the protocol for this.

pv4_get_tree() can. index-pack cannot.

>> Another capability could be added for sending the actual number of
>> objects in a thin pack for more accurate display in index-pack. Low
>> priority in my opinion.
>
> That just cannot be communicated during capability exchange.  This
> number is known only after object enumeration.  Hence my suggestion of a
> ['T', 'H', 'I', 'N', htonl(<number_of_sent_objects>)] special header
> prepended to the actual pack on the wire.  And that has to be decided
> before formalizing the pack v4 capability.  That makes it a somewhat
> higher priority.

The capability is to let the server know the client understands ['T',
'H', 'I', 'N' ..]. The server can choose not to send it later, but
that's ok. Hence the new capability. I'm somewhat reluctant to do it
because of peeking code in fetch-pack and receive-pack and some
refactoring may be needed first. But I could certainly put it on
higher priority.
-- 
Duy

[PATCH] repack: Add --version parameter

[Stefan Beller]

This is just a direct translation of
http://article.gmane.org/gmane.comp.version-control.git/235396
So I don't consider this is ready for inclusion.

Some notes:
We need to have more error checking, repack shall be 0, 2 or 4 but nothing
else. If 0 is given, no argument is passed to pack-objects, in case of
2 or 4 --version=<n> is passed.

Do we really want to call it "--version"? This parameter sounds so much
like questioning for the program version, similar to
	git --version
	1.8.4
So I'd rather use "--repack-version".
---
 builtin/repack.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/builtin/repack.c b/builtin/repack.c
index 3e56614..fd05e9a 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -9,6 +9,7 @@
 #include "argv-array.h"
 
 static int delta_base_offset = 1;
+static int pack_version;
 static char *packdir, *packtmp;
 
 static const char *const git_repack_usage[] = {
@@ -22,6 +23,9 @@ static int repack_config(const char *var, const char *value, void *cb)
 		delta_base_offset = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "core.preferredPackVersion")) {
+		pack_version = git_config_int(var, value);
+	}
 	return git_default_config(var, value, cb);
 }
 
@@ -165,6 +169,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 				N_("limits the maximum delta depth")),
 		OPT_INTEGER(0, "max-pack-size", &max_pack_size,
 				N_("maximum size of each packfile")),
+		OPT_INTEGER(0, "pack-version", &pack_version,
+				N_("format version of the output pack")),
 		OPT_END()
 	};
 
@@ -220,6 +226,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		argv_array_push(&cmd_args,  "--quiet");
 	if (delta_base_offset)
 		argv_array_push(&cmd_args,  "--delta-base-offset");
+	if (pack_version)
+		argv_array_pushf(&cmd_args, "--version=%u", pack_version);
 
 	argv_array_push(&cmd_args, packtmp);
 
-- 
1.8.4.474.g128a96c

Re: [PATCH] repack: Add --version parameter

[Felipe Contreras]

On Thu, Sep 26, 2013 at 3:32 AM, Stefan Beller
<stefanbeller@googlemail.com> wrote:
> This is just a direct translation of
> http://article.gmane.org/gmane.comp.version-control.git/235396
> So I don't consider this is ready for inclusion.
>
> Some notes:
> We need to have more error checking, repack shall be 0, 2 or 4 but nothing
> else. If 0 is given, no argument is passed to pack-objects, in case of
> 2 or 4 --version=<n> is passed.
>
> Do we really want to call it "--version"? This parameter sounds so much
> like questioning for the program version, similar to
>         git --version
>         1.8.4
> So I'd rather use "--repack-version".
> ---
>  builtin/repack.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 3e56614..fd05e9a 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -9,6 +9,7 @@
>  #include "argv-array.h"
>
>  static int delta_base_offset = 1;
> +static int pack_version;
>  static char *packdir, *packtmp;
>
>  static const char *const git_repack_usage[] = {
> @@ -22,6 +23,9 @@ static int repack_config(const char *var, const char *value, void *cb)
>                 delta_base_offset = git_config_bool(var, value);
>                 return 0;
>         }
> +       if (!strcmp(var, "core.preferredPackVersion")) {
> +               pack_version = git_config_int(var, value);
> +       }

The style is without braces if the block is a single line.

-- 
Felipe Contreras

Re: [PATCH] repack: Add --version parameter

[Duy Nguyen]

On Thu, Sep 26, 2013 at 3:32 PM, Stefan Beller
<stefanbeller@googlemail.com> wrote:
> This is just a direct translation of
> http://article.gmane.org/gmane.comp.version-control.git/235396
> So I don't consider this is ready for inclusion.
>
> Some notes:
> We need to have more error checking, repack shall be 0, 2 or 4 but nothing
> else. If 0 is given, no argument is passed to pack-objects, in case of
> 2 or 4 --version=<n> is passed.

It's not that bad. If you don't catch it, pack-objects will.

>
> Do we really want to call it "--version"? This parameter sounds so much
> like questioning for the program version, similar to
>         git --version
>         1.8.4
> So I'd rather use "--repack-version".

Hmm.. I think it's "git repack --pack-version"? Or if you meant "git
pack-objects --version", I drop the "pack-" out because there's
already "pack" in "pack-objects". But I'm OK renaming --version to
--pack-version too. Maybe later.

> @@ -22,6 +23,9 @@ static int repack_config(const char *var, const char *value, void *cb)
>                 delta_base_offset = git_config_bool(var, value);
>                 return 0;
>         }
> +       if (!strcmp(var, "core.preferredPackVersion")) {
> +               pack_version = git_config_int(var, value);
> +       }
>         return git_default_config(var, value, cb);

In np/pack-v4 series (not the one on 'pu' yet) git_default_config will
do this and save the value in core_default_pack_format. So you don't
need to set it here.

> @@ -220,6 +226,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>                 argv_array_push(&cmd_args,  "--quiet");
>         if (delta_base_offset)
>                 argv_array_push(&cmd_args,  "--delta-base-offset");
> +       if (pack_version)
> +               argv_array_pushf(&cmd_args, "--version=%u", pack_version);

but then you may need "if (!pack_version) pack_version =
core_defaul_pack_format;" before this "if".

>
>         argv_array_push(&cmd_args, packtmp);
-- 
Duy

Re: [PATCH 00/10] pack v4 UI support

[Nicolas Pitre]

On Thu, 26 Sep 2013, Duy Nguyen wrote:

> On Thu, Sep 26, 2013 at 11:51 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> >> Multi-base tree support is not part of "packv4" capability. Let's see
> >> if such support comes before the series is merged to master. If so we
> >> can drop that line from protocol-capabilities.txt. Otherwise a new
> >> capability can be added for multi-base trees.
> >
> > What is that for?  Multi-base trees are not created yet, but the code to
> > parse them is already there.  So I don't see the point of having a
> > capability in the protocol for this.
> 
> pv4_get_tree() can. index-pack cannot.

Hmmm... right.

I think this ought to be part of the "packv4" capability nevertheless. 
This is not going into the official git branch right away so there is 
still time to implement it.

> >> Another capability could be added for sending the actual number of
> >> objects in a thin pack for more accurate display in index-pack. Low
> >> priority in my opinion.
> >
> > That just cannot be communicated during capability exchange.  This
> > number is known only after object enumeration.  Hence my suggestion of a
> > ['T', 'H', 'I', 'N', htonl(<number_of_sent_objects>)] special header
> > prepended to the actual pack on the wire.  And that has to be decided
> > before formalizing the pack v4 capability.  That makes it a somewhat
> > higher priority.
> 
> The capability is to let the server know the client understands ['T',
> 'H', 'I', 'N' ..]. The server can choose not to send it later, but
> that's ok. Hence the new capability. I'm somewhat reluctant to do it
> because of peeking code in fetch-pack and receive-pack and some
> refactoring may be needed first. But I could certainly put it on
> higher priority.

Here's what I'm suggesting as a start (untested):

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e3eb5fc..04e5ae1 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -792,9 +792,10 @@ static struct command *read_head_info(void)
 	return commands;
 }
 
-static const char *parse_pack_header(struct pack_header *hdr)
+static const char *parse_pack_header(struct pack_header *hdr,
+				     struct thin_header *thin_hdr)
 {
-	switch (read_pack_header(0, hdr)) {
+	switch (read_pack_header(0, hdr, thin_hdr)) {
 	case PH_ERROR_EOF:
 		return "eof before pack header was fully read";
 
@@ -817,23 +818,25 @@ static const char *pack_lockfile;
 static const char *unpack(int err_fd)
 {
 	struct pack_header hdr;
+	struct thin_header thin_hdr;
 	const char *hdr_err;
-	char hdr_arg[38];
+	char hdr_arg[50];
 	int fsck_objects = (receive_fsck_objects >= 0
 			    ? receive_fsck_objects
 			    : transfer_fsck_objects >= 0
 			    ? transfer_fsck_objects
 			    : 0);
 
-	hdr_err = parse_pack_header(&hdr);
+	hdr_err = parse_pack_header(&hdr, &thin_hdr);
 	if (hdr_err) {
 		if (err_fd > 0)
 			close(err_fd);
 		return hdr_err;
 	}
 	snprintf(hdr_arg, sizeof(hdr_arg),
-			"--pack_header=%"PRIu32",%"PRIu32,
-			ntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));
+			"--pack_header=%"PRIu32",%"PRIu32",%"PRIu32,
+			ntohl(hdr.hdr_version), ntohl(hdr.hdr_entries),
+			ntohl(thin_hdr.hdr_entries));
 
 	if (ntohl(hdr.hdr_entries) < unpack_limit) {
 		int code, i = 0;
diff --git a/fetch-pack.c b/fetch-pack.c
index 6684348..d86e5d1 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -715,12 +715,14 @@ static int get_pack(struct fetch_pack_args *args,
 	*hdr_arg = 0;
 	if (!args->keep_pack && unpack_limit) {
 		struct pack_header header;
+		struct thin_header thin_hdr;
 
-		if (read_pack_header(demux.out, &header))
+		if (read_pack_header(demux.out, &header, &thin_hdr))
 			die("protocol error: bad pack header");
 		snprintf(hdr_arg, sizeof(hdr_arg),
-			 "--pack_header=%"PRIu32",%"PRIu32,
-			 ntohl(header.hdr_version), ntohl(header.hdr_entries));
+			 "--pack_header=%"PRIu32",%"PRIu32",%"PRIu32,
+			 ntohl(header.hdr_version), ntohl(header.hdr_entries),
+			 ntohl(thin_hdr.hdr_entries));
 		if (ntohl(header.hdr_entries) < unpack_limit)
 			do_keep = 0;
 		else
diff --git a/pack.h b/pack.h
index ccefdbe..974b860 100644
--- a/pack.h
+++ b/pack.h
@@ -16,6 +16,18 @@ struct pack_header {
 };
 
 /*
+ * Pack v4 header prefix for thin packs, indicating the actual number
+ * of objects being transmitted.  Expected before the actual pack header
+ * on the wire and not stored on disk upon reception.  This is not
+ * included in the computation of the pack trailing SHA1.
+ */
+#define THIN_SIGNATURE 0x5448494e	/* "THIN" */
+struct thin_header {
+	uint32_t hdr_signature;
+	uint32_t hdr_entries;
+};
+
+/*
  * The first four bytes of index formats later than version 1 should
  * start with this signature, as all older git binaries would find this
  * value illegal and abort reading the file.
@@ -88,7 +100,7 @@ extern int pv4_encode_object_header(enum object_type, uintmax_t, unsigned char *
 #define PH_ERROR_EOF		(-1)
 #define PH_ERROR_PACK_SIGNATURE	(-2)
 #define PH_ERROR_PROTOCOL	(-3)
-extern int read_pack_header(int fd, struct pack_header *);
+extern int read_pack_header(int fd, struct pack_header *, struct thin_header *);
 
 extern struct sha1file *create_tmp_packfile(char **pack_tmp_name);
 extern void finish_tmp_packfile(char *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, struct pack_idx_option *pack_idx_opts, unsigned char sha1[]);
diff --git a/sha1_file.c b/sha1_file.c
index ef6ecc8..5f00c15 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3180,12 +3180,26 @@ int index_path(unsigned char *sha1, const char *path, struct stat *st, unsigned
 	return 0;
 }
 
-int read_pack_header(int fd, struct pack_header *header)
+int read_pack_header(int fd, struct pack_header *header,
+		     struct thin_header *thin_hdr)
 {
 	if (read_in_full(fd, header, sizeof(*header)) < sizeof(*header))
 		/* "eof before pack header was fully read" */
 		return PH_ERROR_EOF;
 
+	if (header->hdr_signature == htonl(THIN_SIGNATURE)) {
+		char *thin_hdr_end = (char *)header + sizeof(*thin_hdr);
+		int leftover = sizeof(*header) - sizeof(*thin_hdr);
+		if (!thin_hdr)
+			return PH_ERROR_PROTOCOL;
+		memcpy(thin_hdr, header, sizeof(*thin_hdr));
+		memcpy(header, thin_hdr_end, leftover);
+		if (read_in_full(fd, thin_hdr_end, sizeof(*header) - leftover)
+				< sizeof(*header) - leftover)
+			return PH_ERROR_EOF;
+	} else if (thin_hdr)
+		memset(thin_hdr, 0, sizeof(*thin_hdr));
+
 	if (header->hdr_signature != htonl(PACK_SIGNATURE))
 		/* "protocol error (pack signature mismatch detected)" */
 		return PH_ERROR_PACK_SIGNATURE;


Nicolas

Re: [PATCH] repack: Add --version parameter

[Stefan Beller]

On 09/26/2013 12:17 PM, Felipe Contreras wrote:
> On Thu, Sep 26, 2013 at 3:32 AM, Stefan Beller

>>  static const char *const git_repack_usage[] = {
>> @@ -22,6 +23,9 @@ static int repack_config(const char *var, const char *value, void *cb)
>>                 delta_base_offset = git_config_bool(var, value);
>>                 return 0;
>>         }
>> +       if (!strcmp(var, "core.preferredPackVersion")) {
>> +               pack_version = git_config_int(var, value);
>> +       }
> 
> The style is without braces if the block is a single line.
> 

Thanks for spotting, I should have put a "return 0;" there.

Stefan

Re: [PATCH] repack: Add --version parameter

[Stefan Beller]

On 09/26/2013 01:42 PM, Duy Nguyen wrote:
> On Thu, Sep 26, 2013 at 3:32 PM, Stefan Beller
> <stefanbeller@googlemail.com> wrote:
>> This is just a direct translation of
>> http://article.gmane.org/gmane.comp.version-control.git/235396
>> So I don't consider this is ready for inclusion.
>>
>> Some notes:
>> We need to have more error checking, repack shall be 0, 2 or 4 but
nothing
>> else. If 0 is given, no argument is passed to pack-objects, in case of
>> 2 or 4 --version=<n> is passed.
>
> It's not that bad. If you don't catch it, pack-objects will.

Ok, noted.

>
>>
>> Do we really want to call it "--version"? This parameter sounds so much
>> like questioning for the program version, similar to
>>         git --version
>>         1.8.4
>> So I'd rather use "--repack-version".
>
> Hmm.. I think it's "git repack --pack-version"? Or if you meant "git
> pack-objects --version", I drop the "pack-" out because there's
> already "pack" in "pack-objects". But I'm OK renaming --version to
> --pack-version too. Maybe later.
>
>> @@ -22,6 +23,9 @@ static int repack_config(const char *var, const
char *value, void *cb)
>>                 delta_base_offset = git_config_bool(var, value);
>>                 return 0;
>>         }
>> +       if (!strcmp(var, "core.preferredPackVersion")) {
>> +               pack_version = git_config_int(var, value);
>> +       }
>>         return git_default_config(var, value, cb);
>
> In np/pack-v4 series (not the one on 'pu' yet) git_default_config will
> do this and save the value in core_default_pack_format. So you don't
> need to set it here.
>
>> @@ -220,6 +226,8 @@ int cmd_repack(int argc, const char **argv, const
char *prefix)
>>                 argv_array_push(&cmd_args,  "--quiet");
>>         if (delta_base_offset)
>>                 argv_array_push(&cmd_args,  "--delta-base-offset");
>> +       if (pack_version)
>> +               argv_array_pushf(&cmd_args, "--version=%u",
pack_version);
>
> but then you may need "if (!pack_version) pack_version =
> core_defaul_pack_format;" before this "if".

The reason I put the pack_version not here is for structural clarity.
("All config is done in either the parse_options section or in the
repack_config function"). This may help having a the actual core logic
easier and more understandable?
If you feel otherwise, I'd change it to your proposal.

Thanks,
Stefan

Re: [PATCH 07/10] receive-pack: request for packv4 if it's the preferred version

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> This is the only plumbing command that is controlled by
> core.preferredPackVersion so far.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/technical/protocol-capabilities.txt | 4 ++++
>  builtin/receive-pack.c                            | 3 ++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
> index be09792..32153cd 100644
> --- a/Documentation/technical/protocol-capabilities.txt
> +++ b/Documentation/technical/protocol-capabilities.txt
> @@ -226,4 +226,8 @@ this capability, the server may send a pack version 4. The server can
>  choose to send pack version 2 even if the client accepts this
>  capability.
>  
> +The receive-pack server advertises this capability if it wants to
> +receive the pack in format version 4 and the client should send in
> +this format.

Technically, "if it can and if it wants to receive" is more correct,
as a v4 capable receiving end can choose to pretend it does not
understand v4 by not sending this capability. Also a v4 incapable
receiver would not advertise it even if it _wants_ to receive.  So
in practice, we see this header only from a receiver that wants to
receive v4, which makes the above statement accurate in a twisted
way.

There needs a bit more explanation on the "should" part, especially
because this is very unusual and unlike all the other capabilities,
which are offered as more freedom of choices without preference on
the advertising side.  Rationale (i.e. reduce load on the receiving
end) and ramifications of non-compliance (e.g. the receiver may
choose to fail the push when its load is too high) are good things
to guide third-party implementors to do the right thing.

>  This capability does not include multi-base tree support.
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index e3eb5fc..288b0bc 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -130,10 +130,11 @@ static void show_ref(const char *path, const unsigned char *sha1)
>  	if (sent_capabilities)
>  		packet_write(1, "%s %s\n", sha1_to_hex(sha1), path);
>  	else
> -		packet_write(1, "%s %s%c%s%s agent=%s\n",
> +		packet_write(1, "%s %s%c%s%s%s agent=%s\n",
>  			     sha1_to_hex(sha1), path, 0,
>  			     " report-status delete-refs side-band-64k quiet",
>  			     prefer_ofs_delta ? " ofs-delta" : "",
> +			     core_default_pack_version == 4 ? " packv4" : "",
>  			     git_user_agent_sanitized());
>  	sent_capabilities = 1;
>  }