{"thread":{"id":"41338","subject":"[PATCH 0/8] Resumable clone revisited, proof of concept","startedAt":"2016-02-05T08:57:49Z","lastAt":"2016-02-08T05:23:34Z","messageCount":22,"participants":["Nguyễn Thái Ngọc Duy","Johannes Schindelin","Elia Pinto","Duy Nguyen","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"277520","messageId":"1454662677-15137-1-git-send-email-pclouds@gmail.com","threadId":"41338","inReplyTo":null,"subject":"[PATCH 0/8] Resumable clone revisited, proof of concept","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-05T08:57:49Z","receivedAt":"2016-02-05T08:57:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"I was reminded by LWN [1] about this. Annoyed in fact because it's\ncalled a bug while it looks more like an elephant. So let's see if we\ncan agree this is a good way to go.\n\nThis is the stable pack approach. The client initiates \"resumable\nmode\" and forces the server to generate a resumable, even though less\nefficient, pack. Compared to the \"download from a cached bundle first\"\napproach, this requires no extra setup for caching. But it wastes more\ncpu.\n\nThe UI is not there yet, and you can only resume a fetch, not a clone.\nWhat remains to do are:\n\n - Add UI to \"git fetch\" to retry\n - Make \"git clone\" use \"git fetch --resume\" to get the pack\n\n[1] Quote from https://lwn.net/Articles/674392/ \"she noted that Git\n still does not support interrupting and resuming download operations,\n which is an important bug to fix.\"\n\nNguyễn Thái Ngọc Duy (8):\n pack-objects: add --skip and --skip-hash\n pack-objects: produce a stable pack when --skip is given\n index-pack: add --append-pack=\n upload-pack: new capability to pass --skip* to pack-objects\n fetch-pack.c: send \"skip\" line to pack-objects\n fetch: add --resume-pack=\n index-pack: --append-pack implies --strict\n one ugly test to verify basic functionality\n\n Documentation/technical/pack-protocol.txt | 2 +\n Documentation/technical/protocol-capabilities.txt | 9 ++++\n builtin/fetch-pack.c | 4 ++\n builtin/fetch.c | 5 +++\n builtin/index-pack.c | 52 ++++++++++++++++++++++-\n builtin/pack-objects.c | 26 ++++++++++++\n csum-file.c | 52 +++++++++++++++++++++--\n csum-file.h | 3 ++\n fetch-pack.c | 40 +++++++++++++++++\n fetch-pack.h | 3 ++\n remote-curl.c | 8 +++-\n t/t5544-fetch-resume.sh (new +x) | 42 ++++++++++++++++++\n transport.c | 4 ++\n transport.h | 4 ++\n upload-pack.c | 30 ++++++++++++-\n 15 files changed, 275 insertions(+), 9 deletions(-)\n create mode 100755 t/t5544-fetch-resume.sh\n\n-- \n2.7.0.377.g4cd97dd\n"},{"id":"277524","messageId":"1454662677-15137-2-git-send-email-pclouds@gmail.com","threadId":"41338","inReplyTo":"1454662677-15137-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 1/8] pack-objects: add --skip and --skip-hash","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-05T08:57:50Z","receivedAt":"2016-02-05T08:57:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The idea is, a pack is requested the first time with --skip=0. If pack\ntransfer is interrupted, the client will ask for the same pack again,\nbut this time it asks the server not to send what it already has. The\nclient hashes what it has and sends the SHA-1 to the server. If the\nserver finds out the skipped part does not match, it can abort early.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/pack-objects.c | 21 ++++++++++++++++++++\n csum-file.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++----\n csum-file.h | 3 +++\n 3 files changed, 72 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 4dae5b1..417c830 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -75,6 +75,9 @@ static unsigned long cache_max_small_delta_size = 1000;\n \n static unsigned long window_memory_limit = 0;\n \n+static struct object_id skip_hash;\n+static int skip_opt = -1;\n+\n /*\n * stats\n */\n@@ -781,6 +784,11 @@ static void write_pack_file(void)\n \t\telse\n \t\t\tf = create_tmp_packfile(&pack_tmp_name);\n \n+\t\tif (skip_opt > 0) {\n+\t\t\tf->skip = skip_opt;\n+\t\t\thashcpy(f->skip_hash, skip_hash.hash);\n+\t\t}\n+\n \t\toffset = write_pack_header(f, nr_remaining);\n \n \t\tif (reuse_packfile) {\n@@ -2597,6 +2605,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tstruct argv_array rp = ARGV_ARRAY_INIT;\n \tint rev_list_unpacked = 0, rev_list_all = 0, rev_list_reflog = 0;\n \tint rev_list_index = 0;\n+\tconst char *skip_hash_hex = NULL;\n \tstruct option pack_objects_options[] = {\n \t\tOPT_SET_INT('q', \"quiet\", &progress,\n \t\t\t N_(\"do not show progress meter\"), 0),\n@@ -2669,6 +2678,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"use a bitmap index if available to speed up counting objects\")),\n \t\tOPT_BOOL(0, \"write-bitmap-index\", &write_bitmap_index,\n \t\t\t N_(\"write a bitmap index together with the pack index\")),\n+\t\tOPT_STRING(0, \"skip-hash\", &skip_hash_hex, \"hash\",\n+\t\t\t N_(\"hash of the skipped part of the pack\")),\n+\t\tOPT_INTEGER(0, \"skip\", &skip_opt,\n+\t\t\t N_(\"do not produce the first bytes of the pack\")),\n \t\tOPT_END(),\n \t};\n \n@@ -2689,6 +2702,14 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t}\n \tif (pack_to_stdout != !base_name || argc)\n \t\tusage_with_options(pack_usage, pack_objects_options);\n+\tif (skip_opt >= 0) {\n+\t\tif (skip_opt > 0) {\n+\t\t\tif (!skip_hash_hex)\n+\t\t\t\tdie(_(\"--skip-hash is required if --skip is non-zero\"));\n+\t\t\tif (get_oid_hex(skip_hash_hex, &skip_hash))\n+\t\t\t\tdie(_(\"%s is not SHA-1\"), skip_hash_hex);\n+\t\t}\n+\t}\n \n \targv_array_push(&rp, \"pack-objects\");\n \tif (thin) {\ndiff --git a/csum-file.c b/csum-file.c\nindex a172199..284847f 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -11,8 +11,27 @@\n #include \"progress.h\"\n #include \"csum-file.h\"\n \n+static void check_skip_hash(struct sha1file *f, off_t old_total)\n+{\n+\tif (old_total < f->skip && f->total > f->skip)\n+\t\tdie(\"BUG: flush() must stop at skip boundary\");\n+\n+\tif (f->total == f->skip) {\n+\t\tgit_SHA_CTX ctx;\n+\t\tunsigned char hash[20];\n+\n+\t\tctx = f->ctx;\n+\t\tgit_SHA1_Final(hash, &ctx);\n+\t\tif (hashcmp(hash, f->skip_hash))\n+\t\t\tdie(\"skip hash does not match, expected %s got %s\",\n+\t\t\t sha1_to_hex(f->skip_hash), sha1_to_hex(hash));\n+\t}\n+}\n+\n static void flush(struct sha1file *f, const void *buf, unsigned int count)\n {\n+\toff_t old_total = f->total;\n+\n \tif (0 <= f->check_fd && count) {\n \t\tunsigned char check_buffer[8192];\n \t\tssize_t ret = read_in_full(f->check_fd, check_buffer, count);\n@@ -26,7 +45,11 @@ static void flush(struct sha1file *f, const void *buf, unsigned int count)\n \t}\n \n \tfor (;;) {\n-\t\tint ret = xwrite(f->fd, buf, count);\n+\t\tint ret;\n+\t\tif (f->total + count <= f->skip)\n+\t\t\tret = count;\n+\t\telse\n+\t\t\tret = xwrite(f->fd, buf, count);\n \t\tif (ret > 0) {\n \t\t\tf->total += ret;\n \t\t\tdisplay_throughput(f->tp, f->total);\n@@ -34,6 +57,8 @@ static void flush(struct sha1file *f, const void *buf, unsigned int count)\n \t\t\tcount -= ret;\n \t\t\tif (count)\n \t\t\t\tcontinue;\n+\t\t\tif (f->skip > 0)\n+\t\t\t\tcheck_skip_hash(f, old_total);\n \t\t\treturn;\n \t\t}\n \t\tif (!ret)\n@@ -45,12 +70,25 @@ static void flush(struct sha1file *f, const void *buf, unsigned int count)\n void sha1flush(struct sha1file *f)\n {\n \tunsigned offset = f->offset;\n+\tconst unsigned char *buffer = f->buffer;\n+\n+\tif (!offset)\n+\t\treturn;\n+\n+\tif (f->total < f->skip && f->skip < f->total + offset) {\n+\t\tunsigned size = f->skip - f->total;\n+\t\tgit_SHA1_Update(&f->ctx, buffer, size);\n+\t\tflush(f, buffer, size);\n+\t\tbuffer += size;\n+\t\toffset -= size;\n+\t}\n \n \tif (offset) {\n-\t\tgit_SHA1_Update(&f->ctx, f->buffer, offset);\n-\t\tflush(f, f->buffer, offset);\n-\t\tf->offset = 0;\n+\t\tgit_SHA1_Update(&f->ctx, buffer, offset);\n+\t\tflush(f, buffer, offset);\n \t}\n+\n+\tf->offset = 0;\n }\n \n int sha1close(struct sha1file *f, unsigned char *result, unsigned int flags)\n@@ -62,6 +100,8 @@ int sha1close(struct sha1file *f, unsigned char *result, unsigned int flags)\n \tif (result)\n \t\thashcpy(result, f->buffer);\n \tif (flags & (CSUM_CLOSE | CSUM_FSYNC)) {\n+\t\tif (f->skip > f->total)\n+\t\t\tdie(_(\"can't skip in the middle of, or beyond the trailing SHA-1\"));\n \t\t/* write checksum and close fd */\n \t\tflush(f, f->buffer, 20);\n \t\tif (flags & CSUM_FSYNC)\n@@ -94,6 +134,9 @@ void sha1write(struct sha1file *f, const void *buf, unsigned int count)\n \t\tunsigned nr = count > left ? left : count;\n \t\tconst void *data;\n \n+\t\tif (f->total < f->skip && f->skip - f->total < nr)\n+\t\t\tnr = f->skip - f->total;\n+\n \t\tif (f->do_crc)\n \t\t\tf->crc32 = crc32(f->crc32, buf, nr);\n \n@@ -149,6 +192,7 @@ struct sha1file *sha1fd_throughput(int fd, const char *name, struct progress *tp\n \tf->tp = tp;\n \tf->name = name;\n \tf->do_crc = 0;\n+\tf->skip = 0;\n \tgit_SHA1_Init(&f->ctx);\n \treturn f;\n }\ndiff --git a/csum-file.h b/csum-file.h\nindex 7530927..5a9475f 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -15,6 +15,9 @@ struct sha1file {\n \tint do_crc;\n \tuint32_t crc32;\n \tunsigned char buffer[8192];\n+\n+\toff_t skip;\n+\tunsigned char skip_hash[20];\n };\n \n /* Checkpoint */\n-- \n2.7.0.377.g4cd97dd\n"},{"id":"277521","messageId":"1454662677-15137-3-git-send-email-pclouds@gmail.com","threadId":"41338","inReplyTo":"1454662677-15137-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/8] pack-objects: produce a stable pack when --skip is given","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-05T08:57:51Z","receivedAt":"2016-02-05T08:57:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Parallel delta search does not produce a stable pack so it's disabled\nwhen --skip is used. zlib compression algorithm is stable. Ref\nnegotiation should be stable (at least on smart http). Ref changes\nwill be addressed separately.\n\nSo unless configuration files or git binary is changed, we should be\nable to produce a stable pack. And if config or binaries are changed,\nit's ok to abort and fetch a new (resumable) pack again.\n\nAlternatively (and not implemented), pack-objects can be taught to\nsave the output pack when --skip=0 is passed in, then somehow find the\ncached version and send the remaining when --skip= is\ngiven. This saves processing power at the cost of disk and cache\nmanagement.\n\nThe key thing for caching though, is \"find the cached version\". We do\nnot provide any way for pack-objects to send a \"key\", like http\ncookies, to the client, so that the client can pass it back on the\nnext fetch. Regardless, the input from client must be identical\nbetween fetches, the server should be able to extract a signature from\nthat and use it to associate with a cached pack. So no need for\nsending keys from the server side.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/pack-objects.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 417c830..c58a9cb 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2709,6 +2709,11 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\tif (get_oid_hex(skip_hash_hex, &skip_hash))\n \t\t\t\tdie(_(\"%s is not SHA-1\"), skip_hash_hex);\n \t\t}\n+\n+\t\t/*\n+\t\t * Parallel delta search can't produce stable packs.\n+\t\t */\n+\t\tdelta_search_threads = 1;\n \t}\n \n \targv_array_push(&rp, \"pack-objects\");\n-- \n2.7.0.377.g4cd97dd\n"},{"id":"277522","messageId":"1454662677-15137-4-git-send-email-pclouds@gmail.com","threadId":"41338","inReplyTo":"1454662677-15137-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/8] index-pack: add --append-pack=","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-05T08:57:52Z","receivedAt":"2016-02-05T08:57:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"In this mode (--append-pack --stdin), index-pack consumes current\ncontent in first without producing anything else, then\ncontinues to read from stdin and append to . This is the\nconsumer end of \"pack-objects --skip\".\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/index-pack.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 48 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 6a01509..f099ac2 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -73,6 +73,7 @@ static int nr_resolved_deltas;\n static int nr_threads;\n \n static int from_stdin;\n+static int skip_mode;\n static int strict;\n static int do_fsck_object;\n static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT;\n@@ -272,7 +273,29 @@ static void *fill(int min)\n \tdo {\n \t\tssize_t ret = xread(input_fd, input_buffer + input_len,\n \t\t\t\tsizeof(input_buffer) - input_len);\n-\t\tif (ret <= 0) {\n+\n+\t\tif (ret == 0 && skip_mode) {\n+\t\t\toutput_fd = input_fd;\n+\t\t\tinput_fd = 0;\n+\t\t\tskip_mode = 0;\n+\t\t\t/*\n+\t\t\t * At this point we have 'input_len' bytes in\n+\t\t\t * input_buffer, which is from the existing pack.\n+\t\t\t * Assuming that we still need to read more, the\n+\t\t\t * next loop will read from stdin instead.\n+\t\t\t *\n+\t\t\t * What's read from stdin must be written down. The\n+\t\t\t * next flush() will write from &input_buffer[0],\n+\t\t\t * which appends the 'input_len' bytes from existing\n+\t\t\t * pack to the pack again.\n+\t\t\t *\n+\t\t\t * Seek back to make this an overwrite (of same\n+\t\t\t * content) instead of an append.\n+\t\t\t */\n+\t\t\tif (input_len && lseek(output_fd, -input_len, SEEK_CUR))\n+\t\t\t\tdie_errno(_(\"cannot seek back %d bytes\"), input_len);\n+\t\t}\n+\t\telse if (ret <= 0) {\n \t\t\tif (!ret)\n \t\t\t\tdie(_(\"early EOF\"));\n \t\t\tdie_errno(_(\"read error on input\"));\n@@ -323,6 +346,26 @@ static const char *open_pack_file(const char *pack_name)\n \treturn pack_name;\n }\n \n+static const char *open_pack_for_append(const char *path)\n+{\n+\tif (!from_stdin)\n+\t\tdie(_(\"--append-pack must be used with --stdin\"));\n+\n+\tinput_fd = open(path, O_CREAT|O_RDWR, 0600);\n+\tif (input_fd < 0)\n+\t\tdie_errno(_(\"cannot open packfile '%s'\"), path);\n+\toutput_fd = -1;\n+\tnothread_data.pack_fd = input_fd;\n+\tgit_SHA1_Init(&input_ctx);\n+\n+\t/*\n+\t * fill() will eventually turn this flag off and set output_fd\n+\t * after reading everything\n+\t */\n+\tskip_mode = 1;\n+\treturn xstrdup(path);\n+}\n+\n static void parse_pack_header(void)\n {\n \tstruct pack_header *hdr = fill(sizeof(struct pack_header));\n@@ -1692,6 +1735,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\t\topts.off32_limit = strtoul(c+1, &c, 0);\n \t\t\t\tif (*c || opts.off32_limit & 0x80000000)\n \t\t\t\t\tdie(_(\"bad %s\"), arg);\n+\t\t\t} else if (skip_prefix(arg, \"--append-pack=\", &arg)) {\n+\t\t\t\tcurr_pack = open_pack_for_append(arg);\n \t\t\t} else\n \t\t\t\tusage(index_pack_usage);\n \t\t\tcontinue;\n@@ -1742,7 +1787,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t}\n #endif\n \n-\tcurr_pack = open_pack_file(pack_name);\n+\tif (!curr_pack)\n+\t\tcurr_pack = open_pack_file(pack_name);\n \tparse_pack_header();\n \tobjects = xcalloc(nr_objects + 1, sizeof(struct object_entry));\n \tif (show_stat)\n-- \n2.7.0.377.g4cd97dd\n"},{"id":"277523","messageId":"1454662677-15137-5-git-send-email-pclouds@gmail.com","threadId":"41338","inReplyTo":"1454662677-15137-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 4/8] upload-pack: new capability to pass --skip* to pack-objects","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-05T08:57:53Z","receivedAt":"2016-02-05T08:57:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/technical/pack-protocol.txt | 2 ++\n Documentation/technical/protocol-capabilities.txt | 9 +++++++\n upload-pack.c | 30 +++++++++++++++++++++--\n 3 files changed, 39 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex c6977bb..5207d08 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -167,6 +167,7 @@ MUST peel the ref if it's an annotated tag.\n ----\n advertised-refs = (no-refs / list-of-refs)\n \t\t *shallow\n+\t\t skip\n \t\t flush-pkt\n \n no-refs = PKT-LINE(zero-id SP \"capabilities^{}\"\n@@ -181,6 +182,7 @@ MUST peel the ref if it's an annotated tag.\n other-peeled = obj-id SP refname \"^{}\"\n \n shallow = PKT-LINE(\"shallow\" SP obj-id)\n+ skip = PKT-LINE(\"skip\" SP obj-id SP 1*DIGIT)\n \n capability-list = capability *(SP capability)\n capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\")\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex eaab6b4..0567970 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -275,3 +275,12 @@ to accept a signed push certificate, and asks the to be\n included in the push certificate. A send-pack client MUST NOT\n send a push-cert packet unless the receive-pack server advertises\n this capability.\n+\n+partial\n+------\n+\n+This capability adds \"skip\" line to the protocol, which passes --skip\n+and --skip-hash to pack-objects. When \"skip\" line is present, given\n+the same set of input from the client (e.g. have, want and shallow\n+lines, \"skip\" line excluded), the exact same pack must be produced.\n+\ndiff --git a/upload-pack.c b/upload-pack.c\nindex b3f6653..5565afe 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -53,6 +53,9 @@ static int use_sideband;\n static int advertise_refs;\n static int stateless_rpc;\n \n+static struct object_id skip_hash;\n+static int skip_opt = -1;\n+\n static void reset_timeout(void)\n {\n \talarm(timeout);\n@@ -90,7 +93,7 @@ static void create_pack_file(void)\n \t\t\"corruption on the remote side.\";\n \tint buffered = -1;\n \tssize_t sz;\n-\tconst char *argv[13];\n+\tconst char *argv[17];\n \tint i, arg = 0;\n \tFILE *pipe_fd;\n \n@@ -112,6 +115,14 @@ static void create_pack_file(void)\n \t\targv[arg++] = \"--delta-base-offset\";\n \tif (use_include_tag)\n \t\targv[arg++] = \"--include-tag\";\n+\tif (skip_opt >= 0) {\n+\t\targv[arg++] = \"--skip\";\n+\t\targv[arg++] = xstrfmt(\"%d\", skip_opt);\n+\t\tif (skip_opt > 0) {\n+\t\t\targv[arg++] = \"--skip-hash\";\n+\t\t\targv[arg++] = xstrdup(oid_to_hex(&skip_hash));\n+\t\t}\n+\t}\n \targv[arg++] = NULL;\n \n \tpack_objects.in = -1;\n@@ -550,6 +561,8 @@ static void receive_needs(void)\n \t\tconst char *features;\n \t\tunsigned char sha1_buf[20];\n \t\tchar *line = packet_read_line(0, NULL);\n+\t\tconst char *arg;\n+\n \t\treset_timeout();\n \t\tif (!line)\n \t\t\tbreak;\n@@ -577,6 +590,19 @@ static void receive_needs(void)\n \t\t\t\tdie(\"Invalid deepen: %s\", line);\n \t\t\tcontinue;\n \t\t}\n+\t\tif (skip_prefix(line, \"skip \", &arg)) {\n+\t\t\tchar *end = NULL;\n+\n+\t\t\tif (get_oid_hex(arg, &skip_hash))\n+\t\t\t\tdie(\"invalid skip line: %s\", line);\n+\t\t\targ += 40;\n+\t\t\tif (*arg++ != ' ')\n+\t\t\t\tdie(\"invalid skip line: %s\", line);\n+\t\t\tskip_opt = strtol(arg, &end, 0);\n+\t\t\tif (!end || *end || skip_opt < 0)\n+\t\t\t\tdie(\"Invalid skip line: %s\", line);\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!starts_with(line, \"want \") ||\n \t\t get_sha1_hex(line+5, sha1_buf))\n \t\t\tdie(\"git upload-pack: protocol error, \"\n@@ -725,7 +751,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n {\n \tstatic const char *capabilities = \"multi_ack thin-pack side-band\"\n \t\t\" side-band-64k ofs-delta shallow no-progress\"\n-\t\t\" include-tag multi_ack_detailed\";\n+\t\t\" include-tag multi_ack_detailed partial\";\n \tconst char *refname_nons = strip_namespace(refname);\n \tstruct object_id peeled;\n \n-- \n2.7.0.377.g4cd97dd\n"},{"id":"277525","messageId":"1454662677-15137-6-git-send-email-pclouds@gmail.com","threadId":"41338","inReplyTo":"1454662677-15137-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 5/8] fetch-pack.c: send \"skip\" line to pack-objects","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-05T08:57:54Z","receivedAt":"2016-02-05T08:57:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n fetch-pack.c | 40 ++++++++++++++++++++++++++++++++++++++++\n fetch-pack.h | 3 +++\n 2 files changed, 43 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 01e34b6..ffb5254 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -15,6 +15,7 @@\n #include \"version.h\"\n #include \"prio-queue.h\"\n #include \"sha1-array.h\"\n+#include \"dir.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -330,6 +331,10 @@ static int find_common(struct fetch_pack_args *args,\n \t\twrite_shallow_commits(&req_buf, 1, NULL);\n \tif (args->depth > 0)\n \t\tpacket_buf_write(&req_buf, \"deepen %d\", args->depth);\n+\tif (args->resume_path)\n+\t\tpacket_buf_write(&req_buf, \"skip %s %d\",\n+\t\t\t\t sha1_to_hex(args->skip_hash),\n+\t\t\t\t args->skip_offset);\n \tpacket_buf_flush(&req_buf);\n \tstate_len = req_buf.len;\n \n@@ -730,6 +735,9 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t\targv_array_push(&cmd.args, \"-v\");\n \t\tif (args->use_thin_pack)\n \t\t\targv_array_push(&cmd.args, \"--fix-thin\");\n+\t\tif (args->resume_path && args->skip_offset)\n+\t\t\targv_array_pushf(&cmd.args, \"--append-pack=%s\",\n+\t\t\t\t\t args->resume_path);\n \t\tif (args->lock_pack || unpack_limit) {\n \t\t\tchar hostname[256];\n \t\t\tif (gethostname(hostname, sizeof(hostname)))\n@@ -856,6 +864,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\t\tfprintf(stderr, \"Server supports ofs-delta\\n\");\n \t} else\n \t\tprefer_ofs_delta = 0;\n+\tif (args->resume_path && !server_supports(\"partial\"))\n+\t\tdie(_(\"Server does not support resume capability\\n\"));\n \n \tif ((agent_feature = server_feature_value(\"agent\", &agent_len))) {\n \t\tagent_supported = 1;\n@@ -1030,6 +1040,35 @@ static void update_shallow(struct fetch_pack_args *args,\n \tsha1_array_clear(&ref);\n }\n \n+static void prepare_resume_fetch(struct fetch_pack_args *args)\n+{\n+\tgit_SHA_CTX c;\n+\tchar buf[8192];\n+\tint fd, len;\n+\n+\tif (!args->resume_path)\n+\t\treturn;\n+\n+\targs->skip_offset = 0;\n+\targs->keep_pack = 1;\n+\tif (!file_exists(args->resume_path))\n+\t\treturn;\n+\n+\tfd = open(args->resume_path, O_RDONLY);\n+\tif (fd == -1)\n+\t\tdie_errno(_(\"failed to open '%s'\"), args->resume_path);\n+\n+\tgit_SHA1_Init(&c);\n+\twhile ((len = xread(fd, buf, sizeof(buf))) > 0) {\n+\t\tgit_SHA1_Update(&c, buf, len);\n+\t\targs->skip_offset += len;\n+\t}\n+\tif (len < 0)\n+\t\tdie_errno(_(\"failed to read '%s'\"), args->resume_path);\n+\tgit_SHA1_Final(args->skip_hash, &c);\n+\tclose(fd);\n+}\n+\n struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t int fd[], struct child_process *conn,\n \t\t const struct ref *ref,\n@@ -1050,6 +1089,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\tdie(\"no matching remote head\");\n \t}\n \tprepare_shallow_info(&si, shallow);\n+\tprepare_resume_fetch(args);\n \tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,\n \t\t\t\t&si, pack_lockfile);\n \treprepare_packed_git();\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex bb7fd76..46f0997 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -10,6 +10,9 @@ struct fetch_pack_args {\n \tconst char *uploadpack;\n \tint unpacklimit;\n \tint depth;\n+\tconst char *resume_path;\n+\tint skip_offset;\n+\tunsigned char skip_hash[20];\n \tunsigned quiet:1;\n \tunsigned keep_pack:1;\n \tunsigned lock_pack:1;\n-- \n2.7.0.377.g4cd97dd\n"},{"id":"277526","messageId":"1454662677-15137-7-git-send-email-pclouds@gmail.com","threadId":"41338","inReplyTo":"1454662677-15137-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 6/8] fetch: add --resume-pack=","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-05T08:57:55Z","receivedAt":"2016-02-05T08:57:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This is a low-level option for resumable fetch. You start a resumable\nfetch with\n\n git fetch --resume-pack=blah \n\nwhere \"blah\" file does not exist. If the fetch is interrupted, \"blah\"\nwill contain what's been fetched so far. Run the same command again.\n\nOn the server side, pack-objects performs the exact same operation to\nproduce full pack again, but it will not send what is already in\n\"blah\". pack-objects does check if the skipped part is the same between\ntwo sides, in case of configuration change or whatever, and abort early.\n\nOn the client side, index-pack feeds itself with what's in \"blah\",\nthen the input stream from pack-objects. index-pack does strict\nverification as usual. Even if pack-objects fails to produce a stable\npack, index-pack should catch it and complain loudly. If everything\ngoes well, \"blah\" is removed and a new good pack is put in $GIT_DIR.\n\nImprovement point. We should be able to perform some heavy operations\nlocally before connecting to the server (e.g. produce the skip hash\nfrom \"blah\", or index-pack consuming \"blah\").\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/fetch-pack.c | 4 ++++\n builtin/fetch.c | 5 +++++\n remote-curl.c | 8 +++++++-\n transport.c | 4 ++++\n transport.h | 4 ++++\n 5 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 9b2a514..996ad30 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -129,6 +129,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\targs.update_shallow = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (skip_prefix(arg, \"--resume-path=\", &arg)) {\n+\t\t\targs.resume_path = arg;\n+\t\t\tcontinue;\n+\t\t}\n \t\tusage(fetch_pack_usage);\n \t}\n \ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 8e74213..34f32c6 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -48,6 +48,7 @@ static const char *recurse_submodules_default;\n static int shown_url = 0;\n static int refmap_alloc, refmap_nr;\n static const char **refmap_array;\n+static const char *resume_path;\n \n static int option_parse_recurse_submodules(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n@@ -115,6 +116,8 @@ static struct option builtin_fetch_options[] = {\n \tOPT_BOOL(0, \"progress\", &progress, N_(\"force progress reporting\")),\n \tOPT_STRING(0, \"depth\", &depth, N_(\"depth\"),\n \t\t N_(\"deepen history of shallow clone\")),\n+\tOPT_FILENAME(0, \"resume-pack\", &resume_path,\n+\t\t N_(\"perform resumable fetch on the given pack\")),\n \t{ OPTION_SET_INT, 0, \"unshallow\", &unshallow, NULL,\n \t\t N_(\"convert to a complete repository\"),\n \t\t PARSE_OPT_NONEG | PARSE_OPT_NOARG, NULL, 1 },\n@@ -872,6 +875,8 @@ static struct transport *prepare_transport(struct remote *remote)\n \t\tset_option(transport, TRANS_OPT_DEPTH, depth);\n \tif (update_shallow)\n \t\tset_option(transport, TRANS_OPT_UPDATE_SHALLOW, \"yes\");\n+\tif (resume_path)\n+\t\tset_option(transport, TRANS_OPT_RESUME_PATH, resume_path);\n \treturn transport;\n }\n \ndiff --git a/remote-curl.c b/remote-curl.c\nindex c704857..36835fb 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -20,6 +20,7 @@ static struct strbuf url = STRBUF_INIT;\n struct options {\n \tint verbosity;\n \tunsigned long depth;\n+\tconst char *resume_path;\n \tunsigned progress : 1,\n \t\tcheck_self_contained_and_connected : 1,\n \t\tcloning : 1,\n@@ -119,6 +120,9 @@ static int set_option(const char *name, const char *value)\n \t\telse\n \t\t\treturn -1;\n \t\treturn 0;\n+\t} else if (!strcmp(name, \"resume-path\")) {\n+\t\toptions.resume_path = xstrdup(value);\n+\t\treturn 0;\n \t} else {\n \t\treturn 1 /* unsupported */;\n \t}\n@@ -727,7 +731,7 @@ static int fetch_git(struct discovery *heads,\n \tstruct strbuf preamble = STRBUF_INIT;\n \tchar *depth_arg = NULL;\n \tint argc = 0, i, err;\n-\tconst char *argv[17];\n+\tconst char *argv[18];\n \n \targv[argc++] = \"fetch-pack\";\n \targv[argc++] = \"--stateless-rpc\";\n@@ -755,6 +759,8 @@ static int fetch_git(struct discovery *heads,\n \t\tdepth_arg = strbuf_detach(&buf, NULL);\n \t\targv[argc++] = depth_arg;\n \t}\n+\tif (options.resume_path)\n+\t\targv[argc++] = xstrfmt(\"--resume-path=%s\", options.resume_path);\n \targv[argc++] = url.buf;\n \targv[argc++] = NULL;\n \ndiff --git a/transport.c b/transport.c\nindex 67f3666..6378bed 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -467,6 +467,9 @@ static int set_git_option(struct git_transport_options *opts,\n \t} else if (!strcmp(name, TRANS_OPT_UPDATE_SHALLOW)) {\n \t\topts->update_shallow = !!value;\n \t\treturn 0;\n+\t} else if (!strcmp(name, TRANS_OPT_RESUME_PATH)) {\n+\t\topts->resume_path = value;\n+\t\treturn 0;\n \t} else if (!strcmp(name, TRANS_OPT_DEPTH)) {\n \t\tif (!value)\n \t\t\topts->depth = 0;\n@@ -534,6 +537,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \t\tdata->options.check_self_contained_and_connected;\n \targs.cloning = transport->cloning;\n \targs.update_shallow = data->options.update_shallow;\n+\targs.resume_path = data->options.resume_path;\n \n \tif (!data->got_remote_heads) {\n \t\tconnect_setup(transport, 0, 0);\ndiff --git a/transport.h b/transport.h\nindex 8ebaaf2..765e4e5 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -16,6 +16,7 @@ struct git_transport_options {\n \tconst char *uploadpack;\n \tconst char *receivepack;\n \tstruct push_cas_option *cas;\n+\tconst char *resume_path;\n };\n \n struct transport {\n@@ -180,6 +181,9 @@ int transport_restrict_protocols(void);\n /* Send push certificates */\n #define TRANS_OPT_PUSH_CERT \"pushcert\"\n \n+/* Resumable fetch */\n+#define TRANS_OPT_RESUME_PATH \"resume-path\"\n+\n /**\n * Returns 0 if the option was used, non-zero otherwise. Prints a\n * message to stderr if the option is not used.\n-- \n2.7.0.377.g4cd97dd\n"},{"id":"277527","messageId":"1454662677-15137-8-git-send-email-pclouds@gmail.com","threadId":"41338","inReplyTo":"1454662677-15137-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 7/8] index-pack: --append-pack implies --strict","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-05T08:57:56Z","receivedAt":"2016-02-05T08:57:56Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"A frankenstein pack, generated by multiple pack-objects runs,\ncertainly has higher risk of broken, especially when the server side\ncould be some other implementation than pack-objects. Be safe and\nstrict.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/index-pack.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex f099ac2..cc60aee 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1736,6 +1736,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tif (*c || opts.off32_limit & 0x80000000)\n \t\t\t\t\tdie(_(\"bad %s\"), arg);\n \t\t\t} else if (skip_prefix(arg, \"--append-pack=\", &arg)) {\n+\t\t\t\tstrict = 1;\n+\t\t\t\tdo_fsck_object = 1;\n \t\t\t\tcurr_pack = open_pack_for_append(arg);\n \t\t\t} else\n \t\t\t\tusage(index_pack_usage);\n-- \n2.7.0.377.g4cd97dd\n"},{"id":"277528","messageId":"1454662677-15137-9-git-send-email-pclouds@gmail.com","threadId":"41338","inReplyTo":"1454662677-15137-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 8/8] one ugly test to verify basic functionality","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-05T08:57:57Z","receivedAt":"2016-02-05T08:57:57Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n t/t5544-fetch-resume.sh (new +x) | 42 ++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 42 insertions(+)\n create mode 100755 t/t5544-fetch-resume.sh\n\ndiff --git a/t/t5544-fetch-resume.sh b/t/t5544-fetch-resume.sh\nnew file mode 100755\nindex 0000000..dfa033d\n--- /dev/null\n+++ b/t/t5544-fetch-resume.sh\n@@ -0,0 +1,42 @@\n+#!/bin/sh\n+\n+test_description='what'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'what' '\n+\ttest_commit one &&\n+\tgit clone --no-local .git abc &&\n+\t(\n+\tcd abc &&\n+\tmv `ls .git/objects/pack/*.pack` pack &&\n+\tgit unpack-objects < pack &&\n+\trm pack &&\n+\tgit fsck\n+\t) &&\n+\ttest_commit two &&\n+\ttest_commit three &&\n+\t(\n+\tcd abc &&\n+\tgit fetch --resume-pack=foo origin HEAD &&\n+\tgit log --format=%s origin/master >actual &&\n+\techo one >expected &&\n+\ttest_cmp expected actual &&\n+\trm .git/FETCH_HEAD &&\n+\tmv `ls .git/objects/pack/*.pack` pack &&\n+\thead -c 123 pack >tmp &&\n+\tgit fetch --resume-pack=tmp origin &&\n+\ttest_path_is_missing tmp &&\n+\tcmp pack .git/objects/pack/*.pack &&\n+\tgit fsck &&\n+\tgit log --format=%s origin/master >actual &&\n+\tcat >expected < [... empty commit message body...]\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n>\n> [...]\n> diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\n> index eaab6b4..0567970 100644\n> --- a/Documentation/technical/protocol-capabilities.txt\n> +++ b/Documentation/technical/protocol-capabilities.txt\n> @@ -275,3 +275,12 @@ to accept a signed push certificate, and asks the to be\n> included in the push certificate. A send-pack client MUST NOT\n> send a push-cert packet unless the receive-pack server advertises\n> this capability.\n> +\n> +partial\n> +------\n> +\n> +This capability adds \"skip\" line to the protocol, which passes --skip\n> +and --skip-hash to pack-objects. When \"skip\" line is present, given\n> +the same set of input from the client (e.g. have, want and shallow\n> +lines, \"skip\" line excluded), the exact same pack must be produced.\n> +\n\nWould you care to elaborate on this idea a bit more? The commit message\nwould make an excellent place for describing the underlying idea, as well\nas comparing it to the alternatives.\n\nCiao,\nDscho"},{"id":"277531","messageId":"alpine.DEB.2.20.1602051019580.2964@virtualbox","threadId":"41338","inReplyTo":"1454662677-15137-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/8] pack-objects: add --skip and --skip-hash","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-05T09:20:47Z","receivedAt":"2016-02-05T09:20:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Fri, 5 Feb 2016, Nguyễn Thái Ngọc Duy wrote:\n\n> The idea is, a pack is requested the first time with --skip=0. If pack\n> transfer is interrupted, the client will ask for the same pack again,\n> but this time it asks the server not to send what it already has. The\n> client hashes what it has and sends the SHA-1 to the server. If the\n> server finds out the skipped part does not match, it can abort early.\n\nAh, here it is. This description should definitely go into Documentation/,\nmethinks. Maybe elaborate a little bit more on the \"what it has\" part?\n\nCiao,\nDscho"},{"id":"277537","messageId":"CA+EOSBnZVuHThXLg9+VftPVaKy5wqqxp=iQtP1ZUtxkKq32HMQ@mail.gmail.com","threadId":"41338","inReplyTo":"1454662677-15137-9-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 8/8] one ugly test to verify basic functionality","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-02-05T11:57:53Z","receivedAt":"2016-02-05T11:57:53Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"2016-02-05 9:57 GMT+01:00 Nguyễn Thái Ngọc Duy :\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> t/t5544-fetch-resume.sh (new +x) | 42 ++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 42 insertions(+)\n> create mode 100755 t/t5544-fetch-resume.sh\n>\n> diff --git a/t/t5544-fetch-resume.sh b/t/t5544-fetch-resume.sh\n> new file mode 100755\n> index 0000000..dfa033d\n> --- /dev/null\n> +++ b/t/t5544-fetch-resume.sh\n> @@ -0,0 +1,42 @@\n> +#!/bin/sh\n> +\n> +test_description='what'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'what' '\n> + test_commit one &&\n> + git clone --no-local .git abc &&\n> + (\n> + cd abc &&\n> + mv `ls .git/objects/pack/*.pack` pack &&\n\nNo, please. From the git coding guideline : \"We prefer $( ... ) for\ncommand substitution; unlike ``, it properly nests.\nIt should have been the way Bourne spelled it from day one, but\nunfortunately isn't.\"\n\nhttp://stackoverflow.com/questions/4708549/whats-the-difference-between-command-and-command-in-shell-programming\n\nThank you\n> + git unpack-objects < pack &&\n> + rm pack &&\n> + git fsck\n> + ) &&\n> + test_commit two &&\n> + test_commit three &&\n> + (\n> + cd abc &&\n> + git fetch --resume-pack=foo origin HEAD &&\n> + git log --format=%s origin/master >actual &&\n> + echo one >expected &&\n> + test_cmp expected actual &&\n> + rm .git/FETCH_HEAD &&\n> + mv `ls .git/objects/pack/*.pack` pack &&\n> + head -c 123 pack >tmp &&\n> + git fetch --resume-pack=tmp origin &&\n> + test_path_is_missing tmp &&\n> + cmp pack .git/objects/pack/*.pack &&\n> + git fsck &&\n> + git log --format=%s origin/master >actual &&\n> + cat >expected < +three\n> +two\n> +one\n> +EOF\n> + test_cmp expected actual\n> + )\n> +'\n> +\n> +test_done\n> --\n> 2.7.0.377.g4cd97dd\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"277539","messageId":"CACsJy8C2+kaPLALojJoFe+T=WoeyQ8JsB0zenDzQt3wpi9D=tw@mail.gmail.com","threadId":"41338","inReplyTo":"CA+EOSBnZVuHThXLg9+VftPVaKy5wqqxp=iQtP1ZUtxkKq32HMQ@mail.gmail.com","subject":"Re: [PATCH 8/8] one ugly test to verify basic functionality","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-05T13:02:18Z","receivedAt":"2016-02-05T13:02:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 5, 2016 at 6:57 PM, Elia Pinto wrote:\n>> + mv `ls .git/objects/pack/*.pack` pack &&\n>\n> No, please. From the git coding guideline : \"We prefer $( ... ) for\n> command substitution; unlike ``, it properly nests.\n\nI was being too subtle with the word \"ugly\". I assure you this test\ncannot be merged in its current form.\n\nBut that's not important. It makes me think, can we catch `..`\nautomatically? I know the test suite can catch broken && command\nsequence. Maybe we can do the same for`..`?\n\n> It should have been the way Bourne spelled it from day one, but\n> unfortunately isn't.\"\n>\n> http://stackoverflow.com/questions/4708549/whats-the-difference-between-command-and-command-in-shell-programming\n-- \nDuy\n"},{"id":"277540","messageId":"alpine.DEB.2.20.1602051417490.2964@virtualbox","threadId":"41338","inReplyTo":"CA+EOSBnZVuHThXLg9+VftPVaKy5wqqxp=iQtP1ZUtxkKq32HMQ@mail.gmail.com","subject":"Re: [PATCH 8/8] one ugly test to verify basic functionality","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-05T13:20:38Z","receivedAt":"2016-02-05T13:20:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elia,\n\nOn Fri, 5 Feb 2016, Elia Pinto wrote:\n\n> From the git coding guideline : \"We prefer $( ... ) for command\n> substitution; unlike ``, it properly nests. It should have been the way\n> Bourne spelled it from day one, but unfortunately isn't.\"\n\nThere was only one time in my life as a developer when I had to change a\n$(...) to a `...` construct. I do not really remember all the details, but\nI think it was on MacOSX, and it was inside a case ... esac and the\nclosing parentheses was mistaken for a case arm (the $(...) construct\nmight have been multi-line, is the only thing that would make sense in my\nmind.\n\nDid you hear of similar problems?\n\nCiao,\nDscho\n"},{"id":"277541","messageId":"CA+EOSB=UoMves8hdParcdtMGgzmx5p4RyKux_kB3SNK-Par-dw@mail.gmail.com","threadId":"41338","inReplyTo":"CACsJy8C2+kaPLALojJoFe+T=WoeyQ8JsB0zenDzQt3wpi9D=tw@mail.gmail.com","subject":"Re: [PATCH 8/8] one ugly test to verify basic functionality","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-02-05T13:33:29Z","receivedAt":"2016-02-05T13:33:29Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"2016-02-05 14:02 GMT+01:00 Duy Nguyen :\n> On Fri, Feb 5, 2016 at 6:57 PM, Elia Pinto wrote:\n>>> + mv `ls .git/objects/pack/*.pack` pack &&\n>>\n>> No, please. From the git coding guideline : \"We prefer $( ... ) for\n>> command substitution; unlike ``, it properly nests.\n>\n> I was being too subtle with the word \"ugly\". I assure you this test\n> cannot be merged in its current form.\n>\n> But that's not important. It makes me think, can we catch `..`\n> automatically? I know the test suite can catch broken && command\n> sequence. Maybe we can do the same for`..`?\nI dunno. But it might be a good idea\n>\n>> It should have been the way Bourne spelled it from day one, but\n>> unfortunately isn't.\"\n>>\n>> http://stackoverflow.com/questions/4708549/whats-the-difference-between-command-and-command-in-shell-programming\n> --\n> Duy\n"},{"id":"277542","messageId":"CA+EOSBn_HB8LABaN497zkr6tvCdqoQXhK=ecsLMx2c5CaJRg1A@mail.gmail.com","threadId":"41338","inReplyTo":"alpine.DEB.2.20.1602051417490.2964@virtualbox","subject":"Re: [PATCH 8/8] one ugly test to verify basic functionality","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-02-05T13:38:14Z","receivedAt":"2016-02-05T13:38:14Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"2016-02-05 14:20 GMT+01:00 Johannes Schindelin :\n> Hi Elia,\n>\n> On Fri, 5 Feb 2016, Elia Pinto wrote:\n>\n>> From the git coding guideline : \"We prefer $( ... ) for command\n>> substitution; unlike ``, it properly nests. It should have been the way\n>> Bourne spelled it from day one, but unfortunately isn't.\"\n>\n> There was only one time in my life as a developer when I had to change a\n> $(...) to a `...` construct. I do not really remember all the details, but\n> I think it was on MacOSX, and it was inside a case ... esac and the\n> closing parentheses was mistaken for a case arm (the $(...) construct\n> might have been multi-line, is the only thing that would make sense in my\n> mind.\n>\n> Did you hear of similar problems?\n\nI not personally. The autoconf manual does not seem to me (for that i\ncan remember) to mention this possible incompatibility, for example.\n\nBest\n\n>\n> Ciao,\n> Dscho\n"},{"id":"277556","messageId":"xmqq1t8rdmky.fsf@gitster.mtv.corp.google.com","threadId":"41338","inReplyTo":"1454662677-15137-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/8] pack-objects: produce a stable pack when --skip is given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-05T18:43:57Z","receivedAt":"2016-02-05T18:43:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 417c830..c58a9cb 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -2709,6 +2709,11 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \t\t\tif (get_oid_hex(skip_hash_hex, &skip_hash))\n> \t\t\t\tdie(_(\"%s is not SHA-1\"), skip_hash_hex);\n> \t\t}\n> +\n> +\t\t/*\n> +\t\t * Parallel delta search can't produce stable packs.\n> +\t\t */\n> +\t\tdelta_search_threads = 1;\n> \t}\n> \n> \targv_array_push(&rp, \"pack-objects\");\n\nA multi-threaded packing is _one_ source of regenerating the same\npack for the same set of objects, but we shouldn't be tying our\nhands by promising it will forever be the _only_ source of it by\ndoing things like this. We may want to dynamically tweak the\npacking behaviour depending on the load of the minute and such for\nexample.\n\nThis is an indication that the approach this series takes is taking\nus in a wrong direction.\n\nI think a more sensible approach for \"resuming\" is to attack cloning\nfirst. Take a reasonable baseline snapshot periodically (depending\non the activity level of the project, the interval may span between\n12 hours to 2 weeks and you would want to make it configurable) to\ncreate a bundle, teach \"clone\" to check the bundle first and perform\na resumable and bulk transfer for the stable part of the history\n(e.g. up to the latest tag or a branch that does not rewind, the set\nof refs to use as the stable anchors you would want to make\nconfigurable), and then fill the gap between that baseline snapshot\nand up-to-date state by doing another round of \"git fetch\" and then\nyou'd have solved the most serious problem already.\n"},{"id":"277616","messageId":"CACsJy8CkQPX4zqW39gpODSkU+habM7TGTJ85MU-S0UuNPk9SCw@mail.gmail.com","threadId":"41338","inReplyTo":"xmqq1t8rdmky.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/8] pack-objects: produce a stable pack when --skip is given","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-05T23:25:56Z","receivedAt":"2016-02-05T23:25:56Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 6, 2016 at 1:43 AM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n>> index 417c830..c58a9cb 100644\n>> --- a/builtin/pack-objects.c\n>> +++ b/builtin/pack-objects.c\n>> @@ -2709,6 +2709,11 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n>> if (get_oid_hex(skip_hash_hex, &skip_hash))\n>> die(_(\"%s is not SHA-1\"), skip_hash_hex);\n>> }\n>> +\n>> + /*\n>> + * Parallel delta search can't produce stable packs.\n>> + */\n>> + delta_search_threads = 1;\n>> }\n>>\n>> argv_array_push(&rp, \"pack-objects\");\n>\n> A multi-threaded packing is _one_ source of regenerating the same\n> pack for the same set of objects, but we shouldn't be tying our\n> hands by promising it will forever be the _only_ source of it by\n> doing things like this. We may want to dynamically tweak the\n> packing behaviour depending on the load of the minute and such for\n> example.\n\nYou noticed that tying the behavior only happens when the user asks\nfor it, right? I don't expect people to do resumable fetch/clone by\ndefault. There are tradeoffs to make and they decide it, we offer\noptions. So, it does not really tie our hands in the normal case.\n\n> I think a more sensible approach for \"resuming\" is to attack cloning\n> first. Take a reasonable baseline snapshot periodically (depending\n> on the activity level of the project, the interval may span between\n> 12 hours to 2 weeks and you would want to make it configurable) to\n> create a bundle, teach \"clone\" to check the bundle first and perform\n> a resumable and bulk transfer for the stable part of the history\n> (e.g. up to the latest tag or a branch that does not rewind, the set\n> of refs to use as the stable anchors you would want to make\n> configurable), and then fill the gap between that baseline snapshot\n> and up-to-date state by doing another round of \"git fetch\" and then\n> you'd have solved the most serious problem already.\n\n_most_. On a troubled connection, fetch can fail as well, especially\nwhen the repo is not uptodate. What about shallow clone? I don't think\nyou want to prepare snapshots for all depths (or some \"popular\"\ndepths). Cloning full then cutting it shallow locally might work, but\nwe're wasting bandwidth and disk space.\n\n> This is an indication that the approach this series takes is taking\n> us in a wrong direction.\n\nThe way I see it, the two approaches complement each other. Snapshots,\nlike pack bitmaps, helps tremendously, but it has corner cases that\ncan be covered by this.\n-- \nDuy\n"},{"id":"277625","messageId":"CAPc5daXmci5TTc0zqKe+izbrQ5XmHd75V=ZKjoWN8hMPTJPSTQ@mail.gmail.com","threadId":"41338","inReplyTo":"CACsJy8CkQPX4zqW39gpODSkU+habM7TGTJ85MU-S0UuNPk9SCw@mail.gmail.com","subject":"Re: [PATCH 2/8] pack-objects: produce a stable pack when --skip is given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-06T00:48:13Z","receivedAt":"2016-02-06T00:48:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> You noticed that tying the behavior only happens when the user asks\n> for it, right? I don't expect people to do resumable fetch/clone by\n> default. There are tradeoffs to make and they decide it, we offer\n> options. So, it does not really tie our hands in the normal case.\n\nYou misread me. I do not want to see us having to add\n\"disable_this_feature = 1\" next to that \"delta_search_thread = 1\"\nin this block, and then supporting code to actually disable that\nfeature, only to support this block. You are declaring that \"to\nsupport this mode, we must always have a way to produce a\nbyte-for-byte identical output and keep supporting it forever\".\n"},{"id":"277684","messageId":"CAPig+cT8G3q4uug4TObyb2jAYvTPPLdhEV2+GpzVKmA6KxsJ_A@mail.gmail.com","threadId":"41338","inReplyTo":"1454662677-15137-6-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 5/8] fetch-pack.c: send \"skip\" line to pack-objects","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-07T08:57:28Z","receivedAt":"2016-02-07T08:57:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 5, 2016 at 3:57 AM, Nguyễn Thái Ngọc Duy wrote:\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> @@ -856,6 +864,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n> fprintf(stderr, \"Server supports ofs-delta\\n\");\n> } else\n> prefer_ofs_delta = 0;\n> + if (args->resume_path && !server_supports(\"partial\"))\n> + die(_(\"Server does not support resume capability\\n\"));\n\nDrop \"\\n\" from die() message.\n\n> if ((agent_feature = server_feature_value(\"agent\", &agent_len))) {\n> agent_supported = 1;\n"},{"id":"277685","messageId":"CAPig+cQ6=nh054ta52NiNq_OmqPT48d+ekOV1epCLwmz55QuJw@mail.gmail.com","threadId":"41338","inReplyTo":"1454662677-15137-8-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 7/8] index-pack: --append-pack implies --strict","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-07T09:02:09Z","receivedAt":"2016-02-07T09:02:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 5, 2016 at 3:57 AM, Nguyễn Thái Ngọc Duy wrote:\n> A frankenstein pack, generated by multiple pack-objects runs,\n> certainly has higher risk of broken, especially when the server side\n\ns/of/& being/\n\n> could be some other implementation than pack-objects. Be safe and\n> strict.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n"},{"id":"277713","messageId":"CACsJy8CyFnUGdBQ4XQ5jvfc7GY08jQkB-z70bDSdSNAPigYDvQ@mail.gmail.com","threadId":"41338","inReplyTo":"CAPc5daXmci5TTc0zqKe+izbrQ5XmHd75V=ZKjoWN8hMPTJPSTQ@mail.gmail.com","subject":"Re: [PATCH 2/8] pack-objects: produce a stable pack when --skip is given","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-08T05:23:34Z","receivedAt":"2016-02-08T05:23:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 6, 2016 at 7:48 AM, Junio C Hamano wrote:\n>> You noticed that tying the behavior only happens when the user asks\n>> for it, right? I don't expect people to do resumable fetch/clone by\n>> default. There are tradeoffs to make and they decide it, we offer\n>> options. So, it does not really tie our hands in the normal case.\n>\n> You misread me. I do not want to see us having to add\n> \"disable_this_feature = 1\" next to that \"delta_search_thread = 1\"\n> in this block, and then supporting code to actually disable that\n> feature, only to support this block. You are declaring that \"to\n> support this mode, we must always have a way to produce a\n> byte-for-byte identical output and keep supporting it forever\".\n\nMy last defense is, this is an extension, not part of the core\nprotocol. If the burden becomes real we can always remove it.\nModem-quality connection users may yell a bit, but the majority of\nbroadband users won't notice a thing. But I understand if the answer\nis still 'no'.\n\nI find it unlikely that this cause much maintenance burden though, the\npacking algorithm has not changed for a very long time (most related\nchange is pack bitmaps, which technically happen before pack-objects).\nThe next big change (at least in public) may be pack v4. We haven't\nfound a good algorithm for pack-objects on v4 yet, so there's a chance\nof problems there.\n-- \nDuy\n"}]}