[PATCH 00/17] np/pack-v4 updates

[PATCH 00/17] np/pack-v4 updates

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

This contains many bug fixes or cleanups. Also you can now run the
test suite with v4 by setting GIT_TEST_OPTS=--packv4. The test suite
passes now. pack size limit is not officially not supported with v4.
index-pack also learns to convert appended trees to v4 for completing
thin packs (still need to convert commits though)

PS. Nico do you still take patches and then send pull requests to
Junio occasionally, or should I start to CC Junio?

Nguyễn Thái Ngọc Duy (17):
  fixup! index-pack: record all delta bases in v4 (tree and ref-delta)
  fixup! packv4-parse.c: add tree offset caching
  fixup! pack-objects: support writing pack v4
  fixup! pack-objects: recognize v4 as pack source
  fixup! index-pack: support completing thin packs v4
  fixup! pack v4: move packv4-create.c to libgit.a
  fixup! index-pack, pack-objects: allow creating .idx v2 with .pack v4
  fixup! pack v4: code to obtain a SHA1 from a sha1ref
  fixup! pack-objects: add --version to specify written pack version
  test-lib.sh: add --packv4 for running the tests with pack v4 as default
  packv4-parse: accept ref-delta as base of pv4-tree
  pack-objects: do not add type OBJ_NONE to objects[] in pack v4
  index-pack: encode appended trees using v4 format in pack v4
  t5302: disable sealth corruption tests when run with --packv4
  t5300: avoid testing ofs-delta with --packv4
  pack-objects: disable pack size limit feature on pack v4
  t5303: adapt the tests to run with --packv4

 .gitignore                            |  1 +
 Documentation/git-pack-objects.txt    |  4 +++
 builtin/index-pack.c                  | 49 ++++++++++++++++++++++++++++++-----
 builtin/pack-objects.c                | 35 ++++++++++++++++++++-----
 packv4-create.c                       | 20 ++++++++++++++
 packv4-create.h                       |  2 ++
 packv4-parse.c                        | 25 +++++++++++-------
 sha1_file.c                           |  4 ++-
 t/t5300-pack-object.sh                | 47 ++++++++++++++++++---------------
 t/t5302-pack-index.sh                 | 24 ++++++++---------
 t/t5303-pack-corruption-resilience.sh | 16 ++++++------
 t/test-lib.sh                         | 10 +++++++
 12 files changed, 173 insertions(+), 64 deletions(-)

-- 
1.8.2.83.gc99314b

[PATCH 01/17] fixup! index-pack: record all delta bases in v4 (tree and ref-delta)

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/index-pack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index f071ed9..fbf97f0 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -775,8 +775,8 @@ static void *unpack_raw_entry(struct object_entry *obj,
 		break;
 	case OBJ_OFS_DELTA:
 		if (packv4)
-			die(_("pack version 4 does not support ofs-delta type (offset %lu)"),
-			    (unsigned long)obj->idx.offset);
+			bad_object(obj->idx.offset,
+				   _("pack version 4 does not support ofs-delta type"));
 		offset = obj->idx.offset - read_varint();
 		if (offset <= 0 || offset >= obj->idx.offset)
 			bad_object(obj->idx.offset,
-- 
1.8.2.83.gc99314b

[PATCH 02/17] fixup! packv4-parse.c: add tree offset caching

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

we need to pass an offset pointing to the header because
unpack_entry() needs that..

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 packv4-parse.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/packv4-parse.c b/packv4-parse.c
index 6abd62e..e833cd2 100644
--- a/packv4-parse.c
+++ b/packv4-parse.c
@@ -470,8 +470,7 @@ static int decode_entries(struct packed_git *p, struct pack_window **w_curs,
 
 		/* is this a canonical tree object? */
 		if ((*scp & 0xf) == OBJ_TREE) {
-			offset = obj_offset + (scp - src);
-			return copy_canonical_tree_entries(p, offset,
+			return copy_canonical_tree_entries(p, obj_offset,
 							   start, count,
 							   dstp, sizep);
 		}
@@ -647,7 +646,7 @@ static int decode_entries(struct packed_git *p, struct pack_window **w_curs,
 		c->offset = offset;
 		c->last_copy_base = copy_objoffset;
 	}
-						
+
 	return 0;
 }
 
-- 
1.8.2.83.gc99314b

[PATCH 03/17] fixup! pack-objects: support writing pack v4

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

In pack v4, commits should be forced to be loaded in canonical format
too (iow. deltified commits are flattened by read_sha1_file, we don't
care about object_entry->delta).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/pack-objects.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 6d3c1c8..30559e8 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -260,10 +260,11 @@ static unsigned long write_no_reuse_object(struct sha1file *f, struct object_ent
 
 	if (!usable_delta ||
 	    /*
-	     * Force loading canonical tree. In future we may want to
-	     * read v4 trees directly instead.
+	     * Force loading canonical trees and commits. In future we
+	     * may want to read v4 objects directly instead.
 	     */
-	    (pack_version == 4 && entry->type == OBJ_TREE)) {
+	    (pack_version == 4 && (entry->type == OBJ_TREE ||
+				   entry->type == OBJ_COMMIT))) {
 		if (entry->type == OBJ_BLOB &&
 		    entry->size > big_file_threshold &&
 		    (st = open_istream(entry->idx.sha1, &type, &size, NULL)) != NULL)
-- 
1.8.2.83.gc99314b

[PATCH 04/17] fixup! pack-objects: recognize v4 as pack source

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/pack-objects.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 30559e8..8e2e5e9 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1436,7 +1436,7 @@ static void check_object(struct object_entry *entry)
 				const unsigned char *sha1, *cp;
 				cp = buf + used;
 				sha1 = get_sha1ref(p, &cp);
-				entry->in_pack_header_size = cp - buf;;
+				entry->in_pack_header_size = cp - buf;
 				if (reuse_delta && !entry->preferred_base)
 					base_ref = sha1;
 				break;
@@ -1490,6 +1490,9 @@ static void check_object(struct object_entry *entry)
 			 * never consider reused delta as the base object to
 			 * deltify other objects against, in order to avoid
 			 * circular deltas.
+			 *
+			 * OBJ_TREE is kept in entry->type in v4 so we
+			 * know to when to write OBJ_PV4_TREE.
 			 */
 			if (pack_version < 4 || entry->type != OBJ_TREE)
 				entry->type = entry->in_pack_type;
-- 
1.8.2.83.gc99314b

[PATCH 05/17] fixup! index-pack: support completing thin packs v4

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

!is_delta_tree() is not correct because the tree may have been
resolved in by find_unresolved_deltas() in the previous iteration of
this loop. Check for entry->idx.sha1 instead, that must be non-null
when we resolve the object.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/index-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index fbf97f0..db885b1 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1783,7 +1783,7 @@ static void fix_unresolved_deltas(struct sha1file *f, int nr_unresolved)
 		enum object_type type;
 		struct base_data *base_obj = alloc_base_data();
 
-		if (obj->real_type != OBJ_REF_DELTA && !is_delta_tree(obj))
+		if (!is_null_sha1(obj->idx.sha1))
 			continue;
 		base_obj->data = read_sha1_file(d->base.sha1, &type, &base_obj->size);
 		if (!base_obj->data)
-- 
1.8.2.83.gc99314b

[PATCH 06/17] fixup! pack v4: move packv4-create.c to libgit.a

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 6b1fd1b..2e2ae2b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -190,6 +190,7 @@
 /test-match-trees
 /test-mergesort
 /test-mktemp
+/test-packv4
 /test-parse-options
 /test-path-utils
 /test-prio-queue
-- 
1.8.2.83.gc99314b

[PATCH 07/17] fixup! index-pack, pack-objects: allow creating .idx v2 with .pack v4

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 sha1_file.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index e98eb8b..ef6ecc8 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -605,7 +605,9 @@ static int check_packed_git_idx(const char *path,  struct packed_git *p)
 		}
 		p->sha1_table = pack_map;
 		p->sha1_table += 12;
-	} else
+	} else if (version == 2)
+		p->sha1_table = idx_map + 8 + 4 * 256;
+	else
 		p->sha1_table = NULL;
 
 	p->index_version = version;
-- 
1.8.2.83.gc99314b

Re: [PATCH 07/17] fixup! index-pack, pack-objects: allow creating .idx v2 with .pack v4

[Nicolas Pitre]

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

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

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  sha1_file.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/sha1_file.c b/sha1_file.c
> index e98eb8b..ef6ecc8 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -605,7 +605,9 @@ static int check_packed_git_idx(const char *path,  struct packed_git *p)
>  		}
>  		p->sha1_table = pack_map;
>  		p->sha1_table += 12;
> -	} else
> +	} else if (version == 2)
> +		p->sha1_table = idx_map + 8 + 4 * 256;
> +	else
>  		p->sha1_table = NULL;

I folded it into "pack v4: initial pack index v3 support on the read 
side" rather than "index-pack, pack-objects: allow creating .idx v2 with 
.pack v4".


Nicolas

[PATCH 08/17] fixup! pack v4: code to obtain a SHA1 from a sha1ref

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

Don't die or it'll upset t5303's recovery action by repacking from
loose objects

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 packv4-parse.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/packv4-parse.c b/packv4-parse.c
index e833cd2..88b7aa1 100644
--- a/packv4-parse.c
+++ b/packv4-parse.c
@@ -18,13 +18,18 @@ const unsigned char *get_sha1ref(struct packed_git *p,
 {
 	const unsigned char *sha1;
 
+	if (!p->sha1_table)
+		return NULL;
+
 	if (!**bufp) {
 		sha1 = *bufp + 1;
 		*bufp += 21;
 	} else {
 		unsigned int index = decode_varint(bufp);
-		if (index < 1 || index - 1 > p->num_objects)
-			die("bad index in %s", __func__);
+		if (index < 1 || index - 1 > p->num_objects) {
+			error("bad index in get_sha1ref");
+			return NULL;
+		}
 		sha1 = p->sha1_table + (index - 1) * 20;
 	}
 
-- 
1.8.2.83.gc99314b

[PATCH 09/17] fixup! pack-objects: add --version to specify written pack version

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-pack-objects.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
index d94edcd..bb27fac 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -220,6 +220,10 @@ So does `git bundle` (see linkgit:git-bundle[1]) when it creates a bundle.
 	to force the version for the generated pack index, and to force
 	64-bit index entries on objects located above the given offset.
 
+--version=<version>::
+	Force the version for the generated pack, Valid values are 2,
+	3 and 4. Default is 2.
+
 --keep-true-parents::
 	With this option, parents that are hidden by grafts are packed
 	nevertheless.
-- 
1.8.2.83.gc99314b

[PATCH 10/17] test-lib.sh: add --packv4 for running the tests with pack v4 as default

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

--packv4 also sets prerequisite PACKV4, which can be used to disable
v2-specific tests. You can run the test suite with

make test GIT_TEST_OPTS=--packv4

or just a specific test

./t5300-*.sh --packv4 -v -i

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/pack-objects.c |  4 ++++
 t/test-lib.sh          | 10 ++++++++++
 2 files changed, 14 insertions(+)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 8e2e5e9..1e0c2e6 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2735,6 +2735,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	if (!pack_compression_seen && core_compression_seen)
 		pack_compression_level = core_compression_level;
 
+	/* GIT_TEST_PACKV4 does not override --version */
+	if (getenv("GIT_TEST_PACKV4"))
+		pack_version = 4;
+
 	progress = isatty(2);
 	argc = parse_options(argc, argv, prefix, pack_objects_options,
 			     pack_usage, 0);
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1aa27bd..931dd23 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -222,6 +222,9 @@ do
 	--statusprefix=*)
 		statusprefix=$(expr "z$1" : 'z[^=]*=\(.*\)')
 		shift ;;
+	--packv4)
+		packv4=t
+		shift ;;
 	*)
 		echo "error: unknown test option '$1'" >&2; exit 1 ;;
 	esac
@@ -740,6 +743,13 @@ else
 	mkdir -p "$TRASH_DIRECTORY"
 fi
 
+if test -n "$packv4"
+then
+	GIT_TEST_PACKV4=t
+	export GIT_TEST_PACKV4
+	test_set_prereq PACKV4
+fi
+
 # Gross hack to spawn N sub-instances of the tests in parallel, and
 # summarize the results.  Note that if this is enabled, the script
 # terminates at the end of this 'if' block.
-- 
1.8.2.83.gc99314b

[PATCH 11/17] packv4-parse: accept ref-delta as base of pv4-tree

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 packv4-parse.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/packv4-parse.c b/packv4-parse.c
index 88b7aa1..31c89c7 100644
--- a/packv4-parse.c
+++ b/packv4-parse.c
@@ -473,16 +473,19 @@ static int decode_entries(struct packed_git *p, struct pack_window **w_curs,
 			if (++scp - src >= avail - 20)
 				return -1;
 
+		switch (*scp++ & 0xf) {
 		/* is this a canonical tree object? */
-		if ((*scp & 0xf) == OBJ_TREE) {
+		case OBJ_TREE:
+		case OBJ_REF_DELTA:
 			return copy_canonical_tree_entries(p, obj_offset,
 							   start, count,
 							   dstp, sizep);
-		}
-
 		/* let's still make sure this is actually a pv4 tree */
-		if ((*scp++ & 0xf) != OBJ_PV4_TREE)
+		case OBJ_PV4_TREE:
+			break;
+		default:
 			return -1;
+		}
 
 		nb_entries = decode_varint(&scp);
 		if (!count)
-- 
1.8.2.83.gc99314b

[PATCH 12/17] pack-objects: do not add type OBJ_NONE to objects[] in pack v4

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

This is a longer explation of what is noted in the patch. When object
names are received from stdin, we lazily put OBJ_NONE as type to
objects[]. check_object() is called for each entry in objects[] later,
when it checks for ref-delta and ofs-delta for straight copy to the
pack later.

In pack v4, we don't store commits and trees as ref-delta and we need
a way to know those are delta objects in pack v2 sources are actually
commits or trees. We detect that with "type" field in struct
object_entry, which is correctly filled when --revs is passed. Without
--revs, the "type" field would be OBJ_NONE and we would need another
sha1_object_info() or similar call to detect the true object type.

Because we need object type anyway in this code path for building up
ident and path dictionaries, fill correct type as well.

Without this, the condition "pack_version < 4 || entry->type !=
OBJ_TREE" in check_object() is always true when --revs is not used,
and we will encode trees as OBJ_REF_DELTA with the base either
OBJ_TREE or OBJ_PV4_TREE. That kills pv4 tree format advantages.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/pack-objects.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1e0c2e6..01954cb 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1496,6 +1496,7 @@ static void check_object(struct object_entry *entry)
 			 */
 			if (pack_version < 4 || entry->type != OBJ_TREE)
 				entry->type = entry->in_pack_type;
+			assert(entry->type != OBJ_NONE);
 			entry->delta = base_entry;
 			entry->delta_size = entry->size;
 			entry->delta_sibling = base_entry->delta_child;
@@ -2361,7 +2362,6 @@ static void read_object_list_from_stdin(void)
 			die("expected sha1, got garbage:\n %s", line);
 
 		add_preferred_base_object(line+41);
-		add_object_entry(sha1, 0, line+41, 0);
 
 		if (pack_version == 4) {
 			void *data;
@@ -2370,7 +2370,17 @@ static void read_object_list_from_stdin(void)
 			int (*add_dict_entries)(struct dict_table *, void *, unsigned long);
 			struct dict_table *dict;
 
-			switch (sha1_object_info(sha1, &size)) {
+			type = sha1_object_info(sha1, &size);
+			/*
+			 * In v2, we can afford to keep entry->type ==
+			 * OBJ_NONE and check_object() will fill it
+			 * later. In v4, we need to know the type
+			 * right now, don't waste time looking for
+			 * type again later in check_object() when the
+			 * in-pack type is ref-delta.
+			 */
+			add_object_entry(sha1, type, line+41, 0);
+			switch (type) {
 			case OBJ_COMMIT:
 				add_dict_entries = add_commit_dict_entries;
 				dict = v4.commit_ident_table;
@@ -2389,7 +2399,8 @@ static void read_object_list_from_stdin(void)
 				die("can't process %s object %s",
 				    typename(type), sha1_to_hex(sha1));
 			free(data);
-		}
+		} else
+			add_object_entry(sha1, 0, line+41, 0);
 	}
 }
 
-- 
1.8.2.83.gc99314b

[PATCH 13/17] index-pack: encode appended trees using v4 format in pack v4

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/index-pack.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 packv4-create.c      | 20 ++++++++++++++++++++
 packv4-create.h      |  2 ++
 3 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index db885b1..caec388 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -12,6 +12,7 @@
 #include "streaming.h"
 #include "thread-utils.h"
 #include "packv4-parse.h"
+#include "packv4-create.h"
 #include "varint.h"
 #include "tree-walk.h"
 
@@ -76,6 +77,7 @@ static struct delta_entry *deltas;
 static struct thread_local nothread_data;
 static unsigned char *sha1_table;
 static struct packv4_dict *name_dict, *path_dict;
+static struct packv4_tables pv4_tables;
 static int nr_objects;
 static int nr_deltas;
 static int nr_resolved_deltas;
@@ -1717,18 +1719,48 @@ static int write_compressed(struct sha1file *f, void *in, unsigned int size)
 	return size;
 }
 
+static void initialize_packv4_tables(void)
+{
+	static int initialized;
+	int i, nr = nr_objects_final;
+	if (initialized)
+		return;
+	pv4_tables.commit_ident_table = pv4_dict_to_dict_table(name_dict);
+	pv4_tables.tree_path_table = pv4_dict_to_dict_table(path_dict);
+	pv4_tables.all_objs_nr = nr;
+	pv4_tables.all_objs = xmalloc(nr * sizeof(struct pack_idx_entry));
+	/* for pv4_encode_tree() pv4_tables[].offset is not needed */
+	for (i = 0; i < nr; i++)
+		hashcpy(pv4_tables.all_objs[i].sha1, sha1_table + i * 20);
+	initialized = 1;
+}
+
 static struct object_entry *append_obj_to_pack(struct sha1file *f,
 			       const unsigned char *sha1, void *buf,
 			       unsigned long size, enum object_type type)
 {
 	struct object_entry *obj = &objects[nr_objects++];
+	void *v4_data = NULL;
 	unsigned char header[10];
+	unsigned long v4_size;
+	enum object_type real_type = type;
 	int n;
 
 	if (packv4) {
 		if (nr_objects > nr_objects_final)
 			die(_("too many objects"));
-		/* TODO: convert OBJ_TREE to OBJ_PV4_TREE using pv4_encode_tree */
+
+		if (type == OBJ_TREE) {
+			initialize_packv4_tables();
+			v4_size = size;
+			v4_data = pv4_encode_tree(&pv4_tables, buf, &v4_size,
+						  NULL, 0, NULL);
+			if (v4_data)
+				type = OBJ_PV4_TREE;
+		}
+
+		/* TODO: convert OBJ_COMMIT to OBJ_PV4_COMMIT using pv4_encode_commit */
+
 		n = pv4_encode_object_header(type, size, header);
 	} else
 		n = encode_in_pack_object_header(type, size, header);
@@ -1737,12 +1769,17 @@ static struct object_entry *append_obj_to_pack(struct sha1file *f,
 	obj[0].size = size;
 	obj[0].hdr_size = n;
 	obj[0].type = type;
-	obj[0].real_type = type;
+	obj[0].real_type = real_type;
 	obj[1].idx.offset = obj[0].idx.offset + n;
-	obj[1].idx.offset += write_compressed(f, buf, size);
+	if (type != real_type) { /* must be v4 representation */
+		sha1write(f, v4_data, v4_size);
+		obj[1].idx.offset += v4_size;
+	} else
+		obj[1].idx.offset += write_compressed(f, buf, size);
 	obj[0].idx.crc32 = crc32_end(f);
 	sha1flush(f);
 	hashcpy(obj->idx.sha1, sha1);
+	free(v4_data);
 	return obj;
 }
 
diff --git a/packv4-create.c b/packv4-create.c
index 3acd10f..14be867 100644
--- a/packv4-create.c
+++ b/packv4-create.c
@@ -15,6 +15,7 @@
 #include "pack-revindex.h"
 #include "progress.h"
 #include "varint.h"
+#include "packv4-parse.h"
 #include "packv4-create.h"
 
 
@@ -144,6 +145,25 @@ void sort_dict_entries_by_hits(struct dict_table *t)
 	rehash_entries(t);
 }
 
+struct dict_table *pv4_dict_to_dict_table(struct packv4_dict *pv4dict)
+{
+	struct dict_table *dict;
+	int i;
+
+	dict = create_dict_table();
+	for (i = 0; i < pv4dict->nb_entries; i++) {
+		const unsigned char *mode_bytes;
+		const char *str;
+		int mode, str_len;
+		mode_bytes = pv4dict->data + pv4dict->offsets[i];
+		mode = (mode_bytes[0] << 8) | mode_bytes[1];
+		str = (const char *)mode_bytes + 2;
+		str_len = pv4dict->offsets[i+1] - pv4dict->offsets[i] - 2;
+		str_len--;  /* for NUL, dict_add_entry will add one back */
+		dict_add_entry(dict, mode, str, str_len);
+	}
+	return dict;
+}
 /*
  * Parse the author/committer line from a canonical commit object.
  * The 'from' argument points right after the "author " or "committer "
diff --git a/packv4-create.h b/packv4-create.h
index 4ac4d71..e0d4b1f 100644
--- a/packv4-create.h
+++ b/packv4-create.h
@@ -8,11 +8,13 @@ struct packv4_tables {
 	struct dict_table *tree_path_table;
 };
 
+struct packv4_dict;
 struct dict_table;
 struct sha1file;
 
 struct dict_table *create_dict_table(void);
 int dict_add_entry(struct dict_table *t, int val, const char *str, int str_len);
+struct dict_table *pv4_dict_to_dict_table(struct packv4_dict *dict);
 void destroy_dict_table(struct dict_table *t);
 void dict_dump(struct packv4_tables *v4);
 
-- 
1.8.2.83.gc99314b

[PATCH 14/17] t5302: disable sealth corruption tests when run with --packv4

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

These tests assume .pack v2 format and won't work with v4. New tests
may be needed to do the same thing with v4 format.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t5302-pack-index.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index fe82025..2f99cd1 100755
--- a/t/t5302-pack-index.sh
+++ b/t/t5302-pack-index.sh
@@ -140,7 +140,7 @@ index_obj_offset()
     ( read offs extra && echo "$offs" )
 }
 
-test_expect_success \
+test_expect_success !PACKV4 \
     '[index v1] 1) stream pack to repository' \
     'git index-pack --index-version=1 --stdin < "test-1-${pack1}.pack" &&
      git prune-packed &&
@@ -148,7 +148,7 @@ test_expect_success \
      cmp "test-1-${pack1}.pack" ".git/objects/pack/pack-${pack1}.pack" &&
      cmp "test-1-${pack1}.idx"  ".git/objects/pack/pack-${pack1}.idx"'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     '[index v1] 2) create a stealth corruption in a delta base reference' \
     '# This test assumes file_101 is a delta smaller than 16 bytes.
      # It should be against file_100 but we substitute its base for file_099
@@ -163,24 +163,24 @@ test_expect_success \
         bs=1 count=20 conv=notrunc &&
      git cat-file blob $sha1_101 > file_101_foo1'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     '[index v1] 3) corrupted delta happily returned wrong data' \
     'test -f file_101_foo1 && ! cmp file_101 file_101_foo1'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     '[index v1] 4) confirm that the pack is actually corrupted' \
     'test_must_fail git fsck --full $commit'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     '[index v1] 5) pack-objects happily reuses corrupted data' \
     'pack4=$(git pack-objects test-4 <obj-list) &&
      test -f "test-4-${pack1}.pack"'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     '[index v1] 6) newly created pack is BAD !' \
     'test_must_fail git verify-pack -v "test-4-${pack1}.pack"'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     '[index v2] 1) stream pack to repository' \
     'rm -f .git/objects/pack/* &&
      git index-pack --index-version=2 --stdin < "test-1-${pack1}.pack" &&
@@ -189,7 +189,7 @@ test_expect_success \
      cmp "test-1-${pack1}.pack" ".git/objects/pack/pack-${pack1}.pack" &&
      cmp "test-2-${pack1}.idx"  ".git/objects/pack/pack-${pack1}.idx"'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     '[index v2] 2) create a stealth corruption in a delta base reference' \
     '# This test assumes file_101 is a delta smaller than 16 bytes.
      # It should be against file_100 but we substitute its base for file_099
@@ -204,20 +204,20 @@ test_expect_success \
         bs=1 count=20 conv=notrunc &&
      git cat-file blob $sha1_101 > file_101_foo2'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     '[index v2] 3) corrupted delta happily returned wrong data' \
     'test -f file_101_foo2 && ! cmp file_101 file_101_foo2'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     '[index v2] 4) confirm that the pack is actually corrupted' \
     'test_must_fail git fsck --full $commit'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     '[index v2] 5) pack-objects refuses to reuse corrupted data' \
     'test_must_fail git pack-objects test-5 <obj-list &&
      test_must_fail git pack-objects --no-reuse-object test-6 <obj-list'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     '[index v2] 6) verify-pack detects CRC mismatch' \
     'rm -f .git/objects/pack/* &&
      git index-pack --index-version=2 --stdin < "test-1-${pack1}.pack" &&
-- 
1.8.2.83.gc99314b

Re: [PATCH 14/17] t5302: disable sealth corruption tests when run with --packv4

[Eric Sunshine]

On Sat, Sep 21, 2013 at 9:58 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> [PATCH 14/17] t5302: disable sealth corruption tests when run with --packv4

s/sealth/stealth/

> These tests assume .pack v2 format and won't work with v4. New tests
> may be needed to do the same thing with v4 format.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

[PATCH 15/17] t5300: avoid testing ofs-delta with --packv4

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t5300-pack-object.sh | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index a07c871..62fc997 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -98,7 +98,7 @@ test_expect_success \
      done'
 cd "$TRASH"
 
-test_expect_success \
+test_expect_success !PACKV4 \
     'pack with OFS_DELTA' \
     'pwd &&
      packname_3=$(git pack-objects --delta-base-offset test-3 <obj-list)'
@@ -106,7 +106,7 @@ test_expect_success \
 rm -fr .git2
 mkdir .git2
 
-test_expect_success \
+test_expect_success !PACKV4 \
     'unpack with OFS_DELTA' \
     'GIT_OBJECT_DIRECTORY=.git2/objects &&
      export GIT_OBJECT_DIRECTORY &&
@@ -116,7 +116,7 @@ test_expect_success \
 
 unset GIT_OBJECT_DIRECTORY
 cd "$TRASH/.git2"
-test_expect_success \
+test_expect_success !PACKV4 \
     'check unpack with OFS_DELTA' \
     '(cd ../.git && find objects -type f -print) |
      while read path
@@ -128,7 +128,7 @@ test_expect_success \
      done'
 cd "$TRASH"
 
-test_expect_success 'compare delta flavors' '
+test_expect_success !PACKV4 'compare delta flavors' '
 	"$PERL_PATH" -e '\''
 		defined($_ = -s $_) or die for @ARGV;
 		exit 1 if $ARGV[0] <= $ARGV[1];
@@ -168,7 +168,7 @@ test_expect_success \
     } >current &&
     test_cmp expect current'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     'use packed deltified (OFS_DELTA) objects' \
     'GIT_OBJECT_DIRECTORY=.git2/objects &&
      export GIT_OBJECT_DIRECTORY &&
@@ -185,7 +185,7 @@ test_expect_success \
 
 unset GIT_OBJECT_DIRECTORY
 
-test_expect_success 'survive missing objects/pack directory' '
+test_expect_success !PACKV4 'survive missing objects/pack directory' '
 	(
 		rm -fr missing-pack &&
 		mkdir missing-pack &&
@@ -205,14 +205,17 @@ test_expect_success 'survive missing objects/pack directory' '
 test_expect_success \
     'verify pack' \
     'git verify-pack	test-1-${packname_1}.idx \
-			test-2-${packname_2}.idx \
-			test-3-${packname_3}.idx'
+			test-2-${packname_2}.idx'
 
 test_expect_success \
     'verify pack -v' \
     'git verify-pack -v	test-1-${packname_1}.idx \
-			test-2-${packname_2}.idx \
-			test-3-${packname_3}.idx'
+			test-2-${packname_2}.idx'
+
+test_expect_success !PACKV4 \
+    'verify ofs pack' \
+    'git verify-pack	test-3-${packname_3}.idx
+     git verify-pack -v test-3-${packname_3}.idx'
 
 test_expect_success \
     'verify-pack catches mismatched .idx and .pack files' \
@@ -277,14 +280,16 @@ test_expect_success \
      git index-pack test-3.pack &&
      cmp test-3.idx test-2-${packname_2}.idx &&
 
-     cat test-3-${packname_3}.pack >test-3.pack &&
-     git index-pack -o tmp.idx test-3-${packname_3}.pack &&
-     cmp tmp.idx test-3-${packname_3}.idx &&
-
-     git index-pack test-3.pack &&
-     cmp test-3.idx test-3-${packname_3}.idx &&
+     if ! test_have_prereq PACKV4
+     then
+	cat test-3-${packname_3}.pack >test-3.pack &&
+	git index-pack -o tmp.idx test-3-${packname_3}.pack &&
+	cmp tmp.idx test-3-${packname_3}.idx &&
 
-     :'
+	git index-pack test-3.pack &&
+	cmp test-3.idx test-3-${packname_3}.idx
+     fi
+     '
 
 test_expect_success 'unpacking with --strict' '
 
-- 
1.8.2.83.gc99314b

Re: [PATCH 15/17] t5300: avoid testing ofs-delta with --packv4

[Nicolas Pitre]

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

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

> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  t/t5300-pack-object.sh | 39 ++++++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
[...]

This, in combination with patch 10/17, is making the test suite to test 
either packv4 or non packv4, and never both.  I think this is not a good 
approach.

Instead we should have packv2 specific tests to enforce --pack-version=2 
when using pack-objects and create a duplicate of those tests for 
--pack-version=4 when that makes sense.  For tests that are mostly 
common, the test could be factored out into a function with a pack 
version argument.  Then, most tests could be always run twice: once for 
packv2 and again for packv4.  Not doing so makes it more risky to 
regress packv2 when testing improvements to packv4 support.


Nicolas

Re: [PATCH 15/17] t5300: avoid testing ofs-delta with --packv4

[Duy Nguyen]

On Sat, Sep 21, 2013 at 11:46 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Sat, 21 Sep 2013, Nguy­n Thái Ng÷c Duy wrote:
>
>>
>> Signed-off-by: Nguy­n Thái Ng÷c Duy <pclouds@gmail.com>
>> ---
>>  t/t5300-pack-object.sh | 39 ++++++++++++++++++++++-----------------
>>  1 file changed, 22 insertions(+), 17 deletions(-)
> [...]
>
> This, in combination with patch 10/17, is making the test suite to test
> either packv4 or non packv4, and never both.  I think this is not a good
> approach.
>
> Instead we should have packv2 specific tests to enforce --pack-version=2
> when using pack-objects and create a duplicate of those tests for
> --pack-version=4 when that makes sense.  For tests that are mostly
> common, the test could be factored out into a function with a pack
> version argument.  Then, most tests could be always run twice: once for
> packv2 and again for packv4.  Not doing so makes it more risky to
> regress packv2 when testing improvements to packv4 support.

I agree. I wanted to split this (and maybe other t53xx) for v4-only
tests and update the existing t53xx to test on v2 only. For now I
think this will do as it will allow us to verify that v4 code works
(just need to run the test suite twice, with and without --packv4).
I'll add more v4 tests that run without --packv4. 10/17 should remain
in the end though to exercise v4 a lot more (some v4 bugs were found
not by t53xx), until we finally switch the default to v4.
-- 
Duy

Re: [PATCH 15/17] t5300: avoid testing ofs-delta with --packv4

[Nicolas Pitre]

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

On Sun, 22 Sep 2013, Duy Nguyen wrote:

> On Sat, Sep 21, 2013 at 11:46 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> > On Sat, 21 Sep 2013, Nguy­n Thái Ng÷c Duy wrote:
> >
> >>
> >> Signed-off-by: Nguy­n Thái Ng÷c Duy <pclouds@gmail.com>
> >> ---
> >>  t/t5300-pack-object.sh | 39 ++++++++++++++++++++++-----------------
> >>  1 file changed, 22 insertions(+), 17 deletions(-)
> > [...]
> >
> > This, in combination with patch 10/17, is making the test suite to test
> > either packv4 or non packv4, and never both.  I think this is not a good
> > approach.
> >
> > Instead we should have packv2 specific tests to enforce --pack-version=2
> > when using pack-objects and create a duplicate of those tests for
> > --pack-version=4 when that makes sense.  For tests that are mostly
> > common, the test could be factored out into a function with a pack
> > version argument.  Then, most tests could be always run twice: once for
> > packv2 and again for packv4.  Not doing so makes it more risky to
> > regress packv2 when testing improvements to packv4 support.
> 
> I agree. I wanted to split this (and maybe other t53xx) for v4-only
> tests and update the existing t53xx to test on v2 only. For now I
> think this will do as it will allow us to verify that v4 code works
> (just need to run the test suite twice, with and without --packv4).

OK.

I've queued those patches at the end of the series so they're easily 
replaceable.


Nicolas

[PATCH 16/17] pack-objects: disable pack size limit feature on pack v4

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

pack v4 format does not go along well with pack size limit feature. v4
requires to know the number of objects in the pack in advance in order
to contruct various tables after pack header. When pack size is
limited, we don't know this number until we write objects out and hit
it. By then the only option we have is rewrite the pack and update the
tables.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/pack-objects.c | 2 ++
 t/t5300-pack-object.sh | 8 ++++----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 01954cb..3fd761a 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2804,6 +2804,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		warning("minimum pack size limit is 1 MiB");
 		pack_size_limit = 1024*1024;
 	}
+	if (pack_size_limit && pack_version >= 4)
+		die("pack size limiting is not supported with pack version 4");
 
 	if (!pack_to_stdout && thin)
 		die("--thin cannot be used to build an indexable pack.");
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 62fc997..831ab79 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -385,9 +385,9 @@ test_expect_success 'index-pack with --strict' '
 	)
 '
 
-test_expect_success 'honor pack.packSizeLimit' '
+test_expect_success 'honor pack.packSizeLimit (pack v2)' '
 	git config pack.packSizeLimit 3m &&
-	packname_10=$(git pack-objects test-10 <obj-list) &&
+	packname_10=$(git pack-objects --version=2 test-10 <obj-list) &&
 	test 2 = $(ls test-10-*.pack | wc -l)
 '
 
@@ -395,9 +395,9 @@ test_expect_success 'verify resulting packs' '
 	git verify-pack test-10-*.pack
 '
 
-test_expect_success 'tolerate packsizelimit smaller than biggest object' '
+test_expect_success 'tolerate packsizelimit smaller than biggest object (pack v2)' '
 	git config pack.packSizeLimit 1 &&
-	packname_11=$(git pack-objects test-11 <obj-list) &&
+	packname_11=$(git pack-objects --version=2 test-11 <obj-list) &&
 	test 5 = $(ls test-11-*.pack | wc -l)
 '
 
-- 
1.8.2.83.gc99314b

[PATCH 17/17] t5303: adapt the tests to run with --packv4

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

"git show-index" does not work with .idx v3 which is the default for
.pack v4. Force .idx back to v2. Disable tests about OBJ_OFS_DELTA on
v4 because that is not supported.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t5303-pack-corruption-resilience.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
index 35926de..21fd54a 100755
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -36,7 +36,7 @@ create_new_pack() {
     blob_2=`git hash-object -t blob -w file_2` &&
     blob_3=`git hash-object -t blob -w file_3` &&
     pack=`printf "$blob_1\n$blob_2\n$blob_3\n" |
-          git pack-objects $@ .git/objects/pack/pack` &&
+	  git pack-objects --index-version=2 $@ .git/objects/pack/pack` &&
     pack=".git/objects/pack/pack-${pack}" &&
     git verify-pack -v ${pack}.pack
 }
@@ -205,7 +205,7 @@ test_expect_success \
      git cat-file blob $blob_2 > /dev/null &&
      git cat-file blob $blob_3 > /dev/null'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     'corruption #0 in delta base reference of first delta (OBJ_OFS_DELTA)' \
     'create_new_pack --delta-base-offset &&
      git prune-packed &&
@@ -214,7 +214,7 @@ test_expect_success \
      test_must_fail git cat-file blob $blob_2 > /dev/null &&
      test_must_fail git cat-file blob $blob_3 > /dev/null'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     '... but having a loose copy allows for full recovery' \
     'mv ${pack}.idx tmp &&
      git hash-object -t blob -w file_2 &&
@@ -223,7 +223,7 @@ test_expect_success \
      git cat-file blob $blob_2 > /dev/null &&
      git cat-file blob $blob_3 > /dev/null'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     '... and then a repack "clears" the corruption' \
     'do_repack --delta-base-offset &&
      git prune-packed &&
@@ -232,7 +232,7 @@ test_expect_success \
      git cat-file blob $blob_2 > /dev/null &&
      git cat-file blob $blob_3 > /dev/null'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     'corruption #1 in delta base reference of first delta (OBJ_OFS_DELTA)' \
     'create_new_pack --delta-base-offset &&
      git prune-packed &&
@@ -241,7 +241,7 @@ test_expect_success \
      test_must_fail git cat-file blob $blob_2 > /dev/null &&
      test_must_fail git cat-file blob $blob_3 > /dev/null'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     '... but having a loose copy allows for full recovery' \
     'mv ${pack}.idx tmp &&
      git hash-object -t blob -w file_2 &&
@@ -250,7 +250,7 @@ test_expect_success \
      git cat-file blob $blob_2 > /dev/null &&
      git cat-file blob $blob_3 > /dev/null'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     '... and then a repack "clears" the corruption' \
     'do_repack --delta-base-offset &&
      git prune-packed &&
@@ -259,7 +259,7 @@ test_expect_success \
      git cat-file blob $blob_2 > /dev/null &&
      git cat-file blob $blob_3 > /dev/null'
 
-test_expect_success \
+test_expect_success !PACKV4 \
     '... and a redundant pack allows for full recovery too' \
     'do_corrupt_object $blob_2 2 < zero &&
      git cat-file blob $blob_1 > /dev/null &&
-- 
1.8.2.83.gc99314b

Re: [PATCH 00/17] np/pack-v4 updates

[Nicolas Pitre]

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

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

> This contains many bug fixes or cleanups. Also you can now run the
> test suite with v4 by setting GIT_TEST_OPTS=--packv4. The test suite
> passes now. pack size limit is not officially not supported with v4.
> index-pack also learns to convert appended trees to v4 for completing
> thin packs (still need to convert commits though)
> 
> PS. Nico do you still take patches and then send pull requests to
> Junio occasionally, or should I start to CC Junio?

I'm still willing to act as the middle man if that suits everybody.  
That gives me the opportunity to review those patches and stay minimally 
involved.


Nicolas

Re: [PATCH 00/17] np/pack-v4 updates

[Junio C Hamano]

Nicolas Pitre <nico@fluxnic.net> writes:

> On Sat, 21 Sep 2013, Nguyễn Thái Ngọc Duy wrote:
>
>> This contains many bug fixes or cleanups. Also you can now run the
>> test suite with v4 by setting GIT_TEST_OPTS=--packv4. The test suite
>> passes now. pack size limit is not officially not supported with v4.
>> index-pack also learns to convert appended trees to v4 for completing
>> thin packs (still need to convert commits though)
>> 
>> PS. Nico do you still take patches and then send pull requests to
>> Junio occasionally, or should I start to CC Junio?
>
> I'm still willing to act as the middle man if that suits everybody.  
> That gives me the opportunity to review those patches and stay minimally 
> involved.

Your reviews in this area are greatly appreciated.