{"thread":{"id":"33712","subject":"[PATCH 0/4] fast-export: speed improvements","startedAt":"2013-05-03T04:31:38Z","lastAt":"2013-05-07T07:34:07Z","messageCount":38,"participants":["Felipe Contreras","Thomas Rast","Junio C Hamano","Michael Haggerty","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"216327","messageId":"1367555502-4706-1-git-send-email-felipe.contreras@gmail.com","threadId":"33712","inReplyTo":null,"subject":"[PATCH 0/4] fast-export: speed improvements","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-03T04:31:38Z","receivedAt":"2013-05-03T04:31:38Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nParsing the marks of an import of the emacs repository moves fast-export to a\ncrawl. It takes 14 minutes in my setup, after these patches, it takes 1 second.\n\nThe important patches are #2 and #3, the rest are niceities.\n\nFelipe Contreras (4):\n fast-{import,export}: use get_sha1_hex() directly\n fast-export: improve speed by skipping blobs\n fast-export: don't parse all the commits\n fast-import: only store commit objects\n\n builtin/fast-export.c | 17 ++++++++++-------\n fast-import.c | 19 ++++++++++++-------\n 2 files changed, 22 insertions(+), 14 deletions(-)\n\n-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216328","messageId":"1367555502-4706-2-git-send-email-felipe.contreras@gmail.com","threadId":"33712","inReplyTo":"1367555502-4706-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 1/4] fast-{import,export}: use get_sha1_hex() directly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-03T04:31:39Z","receivedAt":"2013-05-03T04:31:39Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"There's no point in calling get_sha1() if we know they are SHA-1s.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/fast-export.c | 2 +-\n fast-import.c | 10 +++++-----\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex d60d675..a4dee14 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -621,7 +621,7 @@ static void import_marks(char *input_file)\n \n \t\tmark = strtoumax(line + 1, &mark_end, 10);\n \t\tif (!mark || mark_end == line + 1\n-\t\t\t|| *mark_end != ' ' || get_sha1(mark_end + 1, sha1))\n+\t\t\t|| *mark_end != ' ' || get_sha1_hex(mark_end + 1, sha1))\n \t\t\tdie(\"corrupt mark line: %s\", line);\n \n \t\tif (last_idnum < mark)\ndiff --git a/fast-import.c b/fast-import.c\nindex 5f539d7..e02f212 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1822,7 +1822,7 @@ static void read_marks(void)\n \t\t*end = 0;\n \t\tmark = strtoumax(line + 1, &end, 10);\n \t\tif (!mark || end == line + 1\n-\t\t\t|| *end != ' ' || get_sha1(end + 1, sha1))\n+\t\t\t|| *end != ' ' || get_sha1_hex(end + 1, sha1))\n \t\t\tdie(\"corrupt mark line: %s\", line);\n \t\te = find_object(sha1);\n \t\tif (!e) {\n@@ -2490,7 +2490,7 @@ static void note_change_n(struct branch *b, unsigned char *old_fanout)\n \t\tif (commit_oe->type != OBJ_COMMIT)\n \t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", commit_mark);\n \t\thashcpy(commit_sha1, commit_oe->idx.sha1);\n-\t} else if (!get_sha1(p, commit_sha1)) {\n+\t} else if (!get_sha1_hex(p, commit_sha1)) {\n \t\tunsigned long size;\n \t\tchar *buf = read_object_with_reference(commit_sha1,\n \t\t\tcommit_type, &size, commit_sha1);\n@@ -2604,7 +2604,7 @@ static int parse_from(struct branch *b)\n \t\t\tfree(buf);\n \t\t} else\n \t\t\tparse_from_existing(b);\n-\t} else if (!get_sha1(from, b->sha1))\n+\t} else if (!get_sha1_hex(from, b->sha1))\n \t\tparse_from_existing(b);\n \telse\n \t\tdie(\"Invalid ref name or SHA1 expression: %s\", from);\n@@ -2632,7 +2632,7 @@ static struct hash_list *parse_merge(unsigned int *count)\n \t\t\tif (oe->type != OBJ_COMMIT)\n \t\t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", idnum);\n \t\t\thashcpy(n->sha1, oe->idx.sha1);\n-\t\t} else if (!get_sha1(from, n->sha1)) {\n+\t\t} else if (!get_sha1_hex(from, n->sha1)) {\n \t\t\tunsigned long size;\n \t\t\tchar *buf = read_object_with_reference(n->sha1,\n \t\t\t\tcommit_type, &size, n->sha1);\n@@ -2792,7 +2792,7 @@ static void parse_new_tag(void)\n \t\toe = find_mark(from_mark);\n \t\ttype = oe->type;\n \t\thashcpy(sha1, oe->idx.sha1);\n-\t} else if (!get_sha1(from, sha1)) {\n+\t} else if (!get_sha1_hex(from, sha1)) {\n \t\tstruct object_entry *oe = find_object(sha1);\n \t\tif (!oe) {\n \t\t\ttype = sha1_object_info(sha1, NULL);\n-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216329","messageId":"1367555502-4706-3-git-send-email-felipe.contreras@gmail.com","threadId":"33712","inReplyTo":"1367555502-4706-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 2/4] fast-export: improve speed by skipping blobs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-03T04:31:40Z","receivedAt":"2013-05-03T04:31:40Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"We don't care about blobs, or any object other than commits, but in\norder to find the type of object, we are parsing the whole thing, which\nis slow, specially in big repositories with lots of big files.\n\nThere's no need for that, we can query the object information with\nsha1_object_info();\n\nBefore this, loading the objects of a fresh emacs import, with 260598\nblobs took 14 minutes, after this patch, it takes 3 seconds.\n\nThis is the way fast-import does it. Also die if the object is not\nfound (like fast-import).\n\nSigned-off-by: Felipe Contreras \n---\n builtin/fast-export.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex a4dee14..a5b8da8 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -613,6 +613,7 @@ static void import_marks(char *input_file)\n \t\tchar *line_end, *mark_end;\n \t\tunsigned char sha1[20];\n \t\tstruct object *object;\n+\t\tenum object_type type;\n \n \t\tline_end = strchr(line, '\\n');\n \t\tif (line[0] != ':' || !line_end)\n@@ -627,17 +628,19 @@ static void import_marks(char *input_file)\n \t\tif (last_idnum < mark)\n \t\t\tlast_idnum = mark;\n \n-\t\tobject = parse_object(sha1);\n-\t\tif (!object)\n+\t\ttype = sha1_object_info(sha1, NULL);\n+\t\tif (type < 0)\n+\t\t\tdie(\"object not found: %s\", sha1_to_hex(sha1));\n+\n+\t\tif (type != OBJ_COMMIT)\n+\t\t\t/* only commits */\n \t\t\tcontinue;\n \n+\t\tobject = parse_object(sha1);\n+\n \t\tif (object->flags & SHOWN)\n \t\t\terror(\"Object %s already has a mark\", sha1_to_hex(sha1));\n \n-\t\tif (object->type != OBJ_COMMIT)\n-\t\t\t/* only commits */\n-\t\t\tcontinue;\n-\n \t\tmark_object(object, mark);\n \n \t\tobject->flags |= SHOWN;\n-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216330","messageId":"1367555502-4706-4-git-send-email-felipe.contreras@gmail.com","threadId":"33712","inReplyTo":"1367555502-4706-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 3/4] fast-export: don't parse all the commits","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-03T04:31:41Z","receivedAt":"2013-05-03T04:31:41Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"We don't need the parsed objects at this point, merely the information\nthat they have marks.\n\nSeems to be three times faster in my setup with lots of objects.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/fast-export.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex a5b8da8..3c5a701 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -636,7 +636,7 @@ static void import_marks(char *input_file)\n \t\t\t/* only commits */\n \t\t\tcontinue;\n \n-\t\tobject = parse_object(sha1);\n+\t\tobject = lookup_unknown_object(sha1);\n \n \t\tif (object->flags & SHOWN)\n \t\t\terror(\"Object %s already has a mark\", sha1_to_hex(sha1));\n-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216331","messageId":"1367555502-4706-5-git-send-email-felipe.contreras@gmail.com","threadId":"33712","inReplyTo":"1367555502-4706-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 4/4] fast-import: only store commit objects","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-03T04:31:42Z","receivedAt":"2013-05-03T04:31:42Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"There's no point in storing blob, they would increase the time of\nloading the marks, and the vast majority of them will never be used\nagain.\n\nThis also makes fast-export and fast-import marks compatible.\n\nSigned-off-by: Felipe Contreras \n---\n fast-import.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex e02f212..c583975 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1743,9 +1743,14 @@ static void dump_marks_helper(FILE *f,\n \t\t}\n \t} else {\n \t\tfor (k = 0; k < 1024; k++) {\n-\t\t\tif (m->data.marked[k])\n+\t\t\tif (m->data.marked[k]) {\n+\t\t\t\tstruct object_entry *e;\n+\t\t\t\te = m->data.marked[k];\n+\t\t\t\tif (e->type != OBJ_COMMIT)\n+\t\t\t\t\tcontinue;\n \t\t\t\tfprintf(f, \":%\" PRIuMAX \" %s\\n\", base + k,\n-\t\t\t\t\tsha1_to_hex(m->data.marked[k]->idx.sha1));\n+\t\t\t\t\tsha1_to_hex(e->idx.sha1));\n+\t\t\t}\n \t\t}\n \t}\n }\n-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216363","messageId":"87y5bw3q1s.fsf@hexa.v.cablecom.net","threadId":"33712","inReplyTo":"1367555502-4706-5-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-05-03T17:56:47Z","receivedAt":"2013-05-03T17:56:47Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Felipe Contreras writes:\n\n> There's no point in storing blob, they would increase the time of\n> loading the marks, and the vast majority of them will never be used\n> again.\n>\n> This also makes fast-export and fast-import marks compatible.\n[...]\n> -\t\t\tif (m->data.marked[k])\n> +\t\t\tif (m->data.marked[k]) {\n> +\t\t\t\tstruct object_entry *e;\n> +\t\t\t\te = m->data.marked[k];\n> +\t\t\t\tif (e->type != OBJ_COMMIT)\n> +\t\t\t\t\tcontinue;\n> \t\t\t\tfprintf(f, \":%\" PRIuMAX \" %s\\n\", base + k,\n> -\t\t\t\t\tsha1_to_hex(m->data.marked[k]->idx.sha1));\n> +\t\t\t\t\tsha1_to_hex(e->idx.sha1));\n> +\t\t\t}\n\nIIUC, you are unconditionally storing only marks to commit objects.\n\nAre you allowed to do that at this point? I notice that\ngit-fast-export(1) says\n\n --export-marks=\n Dumps the internal marks table to when complete. Marks are\n written one per line as :markid SHA-1. Only marks for revisions\n are dumped[...]\n\nBut git-fast-import(1) says nothing of the sort; I would even claim that\n\n --export-marks=\n Dumps the internal marks table to when complete.\n\nmeans that the *full* marks table is dumped.\n\nHow do we know that this doesn't break any users of fast-import? Your\ncomment isn't very reassuring:\n\n> the vast majority of them will never be used again\n\nSo what's with the minority?\n\nIn any case, if this does go in, please update the documentation to\nmatch, probably by copying the sentence from git-fast-export(1).\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"216364","messageId":"CAMP44s1R9hAMZ=DQoPiTVi3+40NpADjVFU7tYovZA8W-PWEhhg@mail.gmail.com","threadId":"33712","inReplyTo":"87y5bw3q1s.fsf@hexa.v.cablecom.net","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-03T18:23:32Z","receivedAt":"2013-05-03T18:23:32Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, May 3, 2013 at 12:56 PM, Thomas Rast wrote:\n> Felipe Contreras writes:\n\n> How do we know that this doesn't break any users of fast-import? Your\n> comment isn't very reassuring:\n>\n>> the vast majority of them will never be used again\n>\n> So what's with the minority?\n\nActually I don't think there's any minority. If the client program\ndoesn't store blobs, the blob marks are not used anyway. So there's no\nchange.\n\nHowever, there's a chance the client programs do rely on blob objects,\nin which case things would break. I would like to analyze some client\nprograms of fast-import out there, to see what would be the impact.\n\nBut I think this has to be done either way, the only question is how.\nHaving a warning would take a lot of effort, and it might be for\nnothing. I think it might make sense to knowingly make the potentially\ndangerous change, and see what breaks. Most likely nothing will, but\nif something does, we revert immediately.\n\nOtherwise we would be stuck in this non-ideal state forever.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"216371","messageId":"7vli7vybqg.fsf@alter.siamese.dyndns.org","threadId":"33712","inReplyTo":"1367555502-4706-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/4] fast-{import,export}: use get_sha1_hex() directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-03T21:50:15Z","receivedAt":"2013-05-03T21:50:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> There's no point in calling get_sha1() if we know they are SHA-1s.\n\nIf we know they _have to be_ 40-hex object names, calling get_sha1()\nis not just pointless but outright wrong and these calls have to be\nget_sha1_hex().\n\nLooks like a good change to me.\n\n>\n> Signed-off-by: Felipe Contreras \n> ---\n> builtin/fast-export.c | 2 +-\n> fast-import.c | 10 +++++-----\n> 2 files changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index d60d675..a4dee14 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -621,7 +621,7 @@ static void import_marks(char *input_file)\n> \n> \t\tmark = strtoumax(line + 1, &mark_end, 10);\n> \t\tif (!mark || mark_end == line + 1\n> -\t\t\t|| *mark_end != ' ' || get_sha1(mark_end + 1, sha1))\n> +\t\t\t|| *mark_end != ' ' || get_sha1_hex(mark_end + 1, sha1))\n> \t\t\tdie(\"corrupt mark line: %s\", line);\n> \n> \t\tif (last_idnum < mark)\n> diff --git a/fast-import.c b/fast-import.c\n> index 5f539d7..e02f212 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -1822,7 +1822,7 @@ static void read_marks(void)\n> \t\t*end = 0;\n> \t\tmark = strtoumax(line + 1, &end, 10);\n> \t\tif (!mark || end == line + 1\n> -\t\t\t|| *end != ' ' || get_sha1(end + 1, sha1))\n> +\t\t\t|| *end != ' ' || get_sha1_hex(end + 1, sha1))\n> \t\t\tdie(\"corrupt mark line: %s\", line);\n> \t\te = find_object(sha1);\n> \t\tif (!e) {\n> @@ -2490,7 +2490,7 @@ static void note_change_n(struct branch *b, unsigned char *old_fanout)\n> \t\tif (commit_oe->type != OBJ_COMMIT)\n> \t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", commit_mark);\n> \t\thashcpy(commit_sha1, commit_oe->idx.sha1);\n> -\t} else if (!get_sha1(p, commit_sha1)) {\n> +\t} else if (!get_sha1_hex(p, commit_sha1)) {\n> \t\tunsigned long size;\n> \t\tchar *buf = read_object_with_reference(commit_sha1,\n> \t\t\tcommit_type, &size, commit_sha1);\n> @@ -2604,7 +2604,7 @@ static int parse_from(struct branch *b)\n> \t\t\tfree(buf);\n> \t\t} else\n> \t\t\tparse_from_existing(b);\n> -\t} else if (!get_sha1(from, b->sha1))\n> +\t} else if (!get_sha1_hex(from, b->sha1))\n> \t\tparse_from_existing(b);\n> \telse\n> \t\tdie(\"Invalid ref name or SHA1 expression: %s\", from);\n> @@ -2632,7 +2632,7 @@ static struct hash_list *parse_merge(unsigned int *count)\n> \t\t\tif (oe->type != OBJ_COMMIT)\n> \t\t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", idnum);\n> \t\t\thashcpy(n->sha1, oe->idx.sha1);\n> -\t\t} else if (!get_sha1(from, n->sha1)) {\n> +\t\t} else if (!get_sha1_hex(from, n->sha1)) {\n> \t\t\tunsigned long size;\n> \t\t\tchar *buf = read_object_with_reference(n->sha1,\n> \t\t\t\tcommit_type, &size, n->sha1);\n> @@ -2792,7 +2792,7 @@ static void parse_new_tag(void)\n> \t\toe = find_mark(from_mark);\n> \t\ttype = oe->type;\n> \t\thashcpy(sha1, oe->idx.sha1);\n> -\t} else if (!get_sha1(from, sha1)) {\n> +\t} else if (!get_sha1_hex(from, sha1)) {\n> \t\tstruct object_entry *oe = find_object(sha1);\n> \t\tif (!oe) {\n> \t\t\ttype = sha1_object_info(sha1, NULL);\n"},{"id":"216372","messageId":"7vhaijybnr.fsf@alter.siamese.dyndns.org","threadId":"33712","inReplyTo":"1367555502-4706-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 2/4] fast-export: improve speed by skipping blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-03T21:51:52Z","receivedAt":"2013-05-03T21:51:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> We don't care about blobs, or any object other than commits, but in\n> order to find the type of object, we are parsing the whole thing, which\n> is slow, specially in big repositories with lots of big files.\n>\n> There's no need for that, we can query the object information with\n> sha1_object_info();\n>\n> Before this, loading the objects of a fresh emacs import, with 260598\n> blobs took 14 minutes, after this patch, it takes 3 seconds.\n\nOK.\n\n>\n> This is the way fast-import does it. Also die if the object is not\n> found (like fast-import).\n>\n> Signed-off-by: Felipe Contreras \n> ---\n\n\n> builtin/fast-export.c | 15 +++++++++------\n> 1 file changed, 9 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index a4dee14..a5b8da8 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -613,6 +613,7 @@ static void import_marks(char *input_file)\n> \t\tchar *line_end, *mark_end;\n> \t\tunsigned char sha1[20];\n> \t\tstruct object *object;\n> +\t\tenum object_type type;\n> \n> \t\tline_end = strchr(line, '\\n');\n> \t\tif (line[0] != ':' || !line_end)\n> @@ -627,17 +628,19 @@ static void import_marks(char *input_file)\n> \t\tif (last_idnum < mark)\n> \t\t\tlast_idnum = mark;\n> \n> -\t\tobject = parse_object(sha1);\n> -\t\tif (!object)\n> +\t\ttype = sha1_object_info(sha1, NULL);\n> +\t\tif (type < 0)\n> +\t\t\tdie(\"object not found: %s\", sha1_to_hex(sha1));\n> +\n> +\t\tif (type != OBJ_COMMIT)\n> +\t\t\t/* only commits */\n> \t\t\tcontinue;\n> \n> +\t\tobject = parse_object(sha1);\n> +\n> \t\tif (object->flags & SHOWN)\n> \t\t\terror(\"Object %s already has a mark\", sha1_to_hex(sha1));\n> \n> -\t\tif (object->type != OBJ_COMMIT)\n> -\t\t\t/* only commits */\n> -\t\t\tcontinue;\n> -\n> \t\tmark_object(object, mark);\n> \n> \t\tobject->flags |= SHOWN;\n"},{"id":"216373","messageId":"7vd2t7ybin.fsf@alter.siamese.dyndns.org","threadId":"33712","inReplyTo":"1367555502-4706-4-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 3/4] fast-export: don't parse all the commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-03T21:54:56Z","receivedAt":"2013-05-03T21:54:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> We don't need the parsed objects at this point, merely the information\n> that they have marks.\n>\n> Seems to be three times faster in my setup with lots of objects.\n>\n> Signed-off-by: Felipe Contreras \n> ---\n> builtin/fast-export.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index a5b8da8..3c5a701 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -636,7 +636,7 @@ static void import_marks(char *input_file)\n> \t\t\t/* only commits */\n> \t\t\tcontinue;\n> \n> -\t\tobject = parse_object(sha1);\n> +\t\tobject = lookup_unknown_object(sha1);\n\nThis updates the parse_object() moved by the previous patch. At this\npoint in the codeflow, unlike the original, we already _know_ the\nobject must be a commit; wouldn't an equivalent of:\n\n\tobject = &(lookup_commit(sha1)->object)\n\nbe more correct here?\n\n> \n> \t\tif (object->flags & SHOWN)\n> \t\t\terror(\"Object %s already has a mark\", sha1_to_hex(sha1));\n"},{"id":"216374","messageId":"7v61yzyavo.fsf@alter.siamese.dyndns.org","threadId":"33712","inReplyTo":"87y5bw3q1s.fsf@hexa.v.cablecom.net","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-03T22:08:43Z","receivedAt":"2013-05-03T22:08:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> IIUC, you are unconditionally storing only marks to commit objects.\n>\n> Are you allowed to do that at this point? I notice that\n> git-fast-export(1) says\n>\n> --export-marks=\n> Dumps the internal marks table to when complete. Marks are\n> written one per line as :markid SHA-1. Only marks for revisions\n> are dumped[...]\n>\n> But git-fast-import(1) says nothing of the sort; I would even claim that\n>\n> --export-marks=\n> Dumps the internal marks table to when complete.\n>\n> means that the *full* marks table is dumped.\n>\n> How do we know that this doesn't break any users of fast-import? Your\n> comment isn't very reassuring:\n>\n>> the vast majority of them will never be used again\n>\n> So what's with the minority?\n>\n> In any case, if this does go in, please update the documentation to\n> match, probably by copying the sentence from git-fast-export(1).\n\nA safe and sane approach may be to teach these an option to tell\nthem to omit non-commits or to emit all kinds, and make remote-bzr\nuse that to exclude non-commits. If the defaults is matched to the\ncurrent behaviour, nobody will get hurt, and Felipe's Emacs import,\nknowing that it does not need marks to blobs, can take advantage of\nthe new feature.\n"},{"id":"216376","messageId":"CAMP44s2o2EvmSwB0P_WkudxQWigUV20+ECio1rUGPt8qF5=0Ow@mail.gmail.com","threadId":"33712","inReplyTo":"7v61yzyavo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-03T22:19:44Z","receivedAt":"2013-05-03T22:19:44Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, May 3, 2013 at 5:08 PM, Junio C Hamano wrote:\n> Thomas Rast writes:\n>\n>> IIUC, you are unconditionally storing only marks to commit objects.\n>>\n>> Are you allowed to do that at this point? I notice that\n>> git-fast-export(1) says\n>>\n>> --export-marks=\n>> Dumps the internal marks table to when complete. Marks are\n>> written one per line as :markid SHA-1. Only marks for revisions\n>> are dumped[...]\n>>\n>> But git-fast-import(1) says nothing of the sort; I would even claim that\n>>\n>> --export-marks=\n>> Dumps the internal marks table to when complete.\n>>\n>> means that the *full* marks table is dumped.\n>>\n>> How do we know that this doesn't break any users of fast-import? Your\n>> comment isn't very reassuring:\n>>\n>>> the vast majority of them will never be used again\n>>\n>> So what's with the minority?\n>>\n>> In any case, if this does go in, please update the documentation to\n>> match, probably by copying the sentence from git-fast-export(1).\n>\n> A safe and sane approach may be to teach these an option to tell\n> them to omit non-commits or to emit all kinds, and make remote-bzr\n> use that to exclude non-commits.\n\nThis has nothing to do with remote-bzr, or any remote helper. These\nobjects are not useful, not even to 'git fast-export'.\n\n> If the defaults is matched to the\n> current behaviour, nobody will get hurt\n\nChanging nothing always ensures that nobody will get hurt, but that\ndoesn't improve anything either.\n\n-- \nFelipe Contreras\n"},{"id":"216382","messageId":"7vr4hnwru7.fsf@alter.siamese.dyndns.org","threadId":"33712","inReplyTo":"CAMP44s2o2EvmSwB0P_WkudxQWigUV20+ECio1rUGPt8qF5=0Ow@mail.gmail.com","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-03T23:45:20Z","receivedAt":"2013-05-03T23:45:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> A safe and sane approach may be to teach these an option to tell\n>> them to omit non-commits or to emit all kinds, and make remote-bzr\n>> use that to exclude non-commits.\n>\n> This has nothing to do with remote-bzr, or any remote helper. These\n> objects are not useful, not even to 'git fast-export'.\n>\n>> If the defaults is matched to the\n>> current behaviour, nobody will get hurt\n>\n> Changing nothing always ensures that nobody will get hurt, but that\n> doesn't improve anything either.\n\nI do not quite follow. Allowing existing code to depend on old\nbehaviour, while letting new code that knows it does not need\nanything but commits, will get the best of both worlds. The new code\nwill definitely improve (otherwise you won't be writing these\npatches), and the old code will not get hurt. Where is this \"doesn't\nimprove anything\" coming from?\n"},{"id":"216383","messageId":"CAMP44s3CPNyrWQoXoNDGaKfH6qniosfFMdjWVD_8FKD+Vge_8Q@mail.gmail.com","threadId":"33712","inReplyTo":"7vr4hnwru7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-04T00:01:09Z","receivedAt":"2013-05-04T00:01:09Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, May 3, 2013 at 6:45 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>>> A safe and sane approach may be to teach these an option to tell\n>>> them to omit non-commits or to emit all kinds, and make remote-bzr\n>>> use that to exclude non-commits.\n>>\n>> This has nothing to do with remote-bzr, or any remote helper. These\n>> objects are not useful, not even to 'git fast-export'.\n>>\n>>> If the defaults is matched to the\n>>> current behaviour, nobody will get hurt\n>>\n>> Changing nothing always ensures that nobody will get hurt, but that\n>> doesn't improve anything either.\n>\n> I do not quite follow. Allowing existing code to depend on old\n> behaviour, while letting new code that knows it does not need\n> anything but commits, will get the best of both worlds. The new code\n> will definitely improve (otherwise you won't be writing these\n> patches), and the old code will not get hurt. Where is this \"doesn't\n> improve anything\" coming from?\n\nSo let's suppose we add a new 'feature no-blob-store' to 'git\nfast-import', and some clients of 'git fast-import' enable it, and\nbenefit from it.\n\nHow does that benefit the rest of fast-import clients? You are\nworrying about clients that most likely don't exist, and don't let\nexisting real clients benefit for free. This is like premature\noptimization.\n\nBut whatever, let's keep writing and discarding these blobs, at least\nthe previous patches do make such operations fast.\n\nYou can drop this patch, because I'm not going to write code for\nclients that don't exist. And it seems clear that even if I\ninvestigate client apps of 'git fast-import' and I'm unable to find a\nsingle one that utilizes blobs, you still wouldn't apply this patch.\nSo there's no point in investigating what are the *actual* users, if\nall we are every going to do is worry about hypothetical ones.\n\nMy main interest is the previous patches, if anybody has any issues\nwith the way this patch is handled, they can either work on the patch\nitself, or start a new thread in which I will not participate, I will\nrather concentrate on issues that affect *real* users, and have real\nfixes reachable today, and the previous patches in this series fit\nthat bill.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"216387","messageId":"CAMP44s09KqFuXPf=3qG42jzChnNBoiXdStME6Ga7EzY7uwF88Q@mail.gmail.com","threadId":"33712","inReplyTo":"7vd2t7ybin.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/4] fast-export: don't parse all the commits","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-04T00:06:29Z","receivedAt":"2013-05-04T00:06:29Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, May 3, 2013 at 4:54 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> We don't need the parsed objects at this point, merely the information\n>> that they have marks.\n>>\n>> Seems to be three times faster in my setup with lots of objects.\n>>\n>> Signed-off-by: Felipe Contreras \n>> ---\n>> builtin/fast-export.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n>> index a5b8da8..3c5a701 100644\n>> --- a/builtin/fast-export.c\n>> +++ b/builtin/fast-export.c\n>> @@ -636,7 +636,7 @@ static void import_marks(char *input_file)\n>> /* only commits */\n>> continue;\n>>\n>> - object = parse_object(sha1);\n>> + object = lookup_unknown_object(sha1);\n>\n> This updates the parse_object() moved by the previous patch. At this\n> point in the codeflow, unlike the original, we already _know_ the\n> object must be a commit; wouldn't an equivalent of:\n>\n> object = &(lookup_commit(sha1)->object)\n>\n> be more correct here?\n\nMaybe, if we want to run some extra code we don't care about.\n\nThe only actual difference is that object->type will be OBJ_COMMIT,\nbut a) this is not going to be used anywhere, and b) we can set that\nourselves.\n\nIn fact, my original code was:\n\n\tobject = lookup_object(sha1);\n\tif (!object)\n\t\tobject = create_object(sha1, OBJ_COMMIT, alloc_object_node());\n\nBut I figured there's no need for those extra lines of code.\n\n-- \nFelipe Contreras\n"},{"id":"216419","messageId":"7vli7uwnws.fsf@alter.siamese.dyndns.org","threadId":"33712","inReplyTo":"CAMP44s09KqFuXPf=3qG42jzChnNBoiXdStME6Ga7EzY7uwF88Q@mail.gmail.com","subject":"Re: [PATCH 3/4] fast-export: don't parse all the commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-04T19:22:27Z","receivedAt":"2013-05-04T19:22:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> This updates the parse_object() moved by the previous patch. At this\n>> point in the codeflow, unlike the original, we already _know_ the\n>> object must be a commit; wouldn't an equivalent of:\n>>\n>> object = &(lookup_commit(sha1)->object)\n>>\n>> be more correct here?\n>\n> Maybe, if we want to run some extra code we don't care about.\n>\n> The only actual difference is that object->type will be OBJ_COMMIT,\n> ...\n\nThe reason to prefer lookup_commit() over lookup_unknown_object() is\nprimarily that we allocate the storage for known type, not union,\nout of the allocation pool (alloc.c) for the known type. Also we\nmark the result with the type so that we can catch mistakes in data\nwhen later code somehow expects an object with the same object name\nto be of different type; while the latter is a good safety measure,\nI do not think it matters that much in this codepath. The former is\nmore important in general.\n"},{"id":"216484","messageId":"518785B3.3050606@alum.mit.edu","threadId":"33712","inReplyTo":"CAMP44s1R9hAMZ=DQoPiTVi3+40NpADjVFU7tYovZA8W-PWEhhg@mail.gmail.com","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-06T10:28:03Z","receivedAt":"2013-05-06T10:28:03Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/03/2013 08:23 PM, Felipe Contreras wrote:\n> On Fri, May 3, 2013 at 12:56 PM, Thomas Rast wrote:\n>> Felipe Contreras writes:\n> \n>> How do we know that this doesn't break any users of fast-import? Your\n>> comment isn't very reassuring:\n>>\n>>> the vast majority of them will never be used again\n>>\n>> So what's with the minority?\n> \n> Actually I don't think there's any minority. If the client program\n> doesn't store blobs, the blob marks are not used anyway. So there's no\n> change.\n\nI haven't been following this conversation in detail, but your proposed\nchange sounds like something that would break cvs2git [1]. Let me\nexplain what cvs2git does and why:\n\nCVS stores all of the revisions of a single file in a single filename,v\nfile in rcsfile(5) format. The revisions are stored as deltas ordered\nso that a single revision can be reconstructed from a single serial read\nof the file.\n\ncvs2git reads each of these files once, reconstructing *all* of the\nrevisions for a file in a single go. It then pours them into a\ngit-fast-import stream as blobs and sets a mark on each blob.\n\nOnly much later in the conversion does it have enough information to\nreconstruct tree-wide commits. At that time it outputs git-fast-import\ndata (to a second file) defining the git commits and their ancestry.\nThe contents are defined by referring to the marks of blobs from the\nfirst git-fast-import stream file.\n\nThis strategy speeds up the conversion *enormously*.\n\nSo if I understand correctly that you are proposing to stop allowing\nmarks on blob objects to be set and/or referred to later, then I object\nvociferously.\n\nIf I've misunderstood then I'll go back into my hole :-)\n\nMichael\n\n[1] http://cvs2svn.tigris.org/cvs2git.html\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"216485","messageId":"87ip2wflg0.fsf@linux-k42r.v.cablecom.net","threadId":"33712","inReplyTo":"518785B3.3050606@alum.mit.edu","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-05-06T10:32:15Z","receivedAt":"2013-05-06T10:32:15Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Michael Haggerty writes:\n\n> On 05/03/2013 08:23 PM, Felipe Contreras wrote:\n>> On Fri, May 3, 2013 at 12:56 PM, Thomas Rast wrote:\n>>> Felipe Contreras writes:\n>> \n>>> How do we know that this doesn't break any users of fast-import? Your\n>>> comment isn't very reassuring:\n>>>\n>>>> the vast majority of them will never be used again\n>>>\n>>> So what's with the minority?\n>> \n>> Actually I don't think there's any minority. If the client program\n>> doesn't store blobs, the blob marks are not used anyway. So there's no\n>> change.\n>\n> I haven't been following this conversation in detail, but your proposed\n> change sounds like something that would break cvs2git [1]. Let me\n> explain what cvs2git does and why:\n>\n> CVS stores all of the revisions of a single file in a single filename,v\n> file in rcsfile(5) format. The revisions are stored as deltas ordered\n> so that a single revision can be reconstructed from a single serial read\n> of the file.\n>\n> cvs2git reads each of these files once, reconstructing *all* of the\n> revisions for a file in a single go. It then pours them into a\n> git-fast-import stream as blobs and sets a mark on each blob.\n>\n> Only much later in the conversion does it have enough information to\n> reconstruct tree-wide commits. At that time it outputs git-fast-import\n> data (to a second file) defining the git commits and their ancestry.\n> The contents are defined by referring to the marks of blobs from the\n> first git-fast-import stream file.\n>\n> This strategy speeds up the conversion *enormously*.\n>\n> So if I understand correctly that you are proposing to stop allowing\n> marks on blob objects to be set and/or referred to later, then I object\n> vociferously.\n\nThe proposed patch wants to stop writing marks (in --export-marks) for\nanything but commits. Does cvs2git depend on that? I.e., are you using\ntwo separate fast-import processes for the blob and tree/commit phases\nyou describe above?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"216486","messageId":"518789D1.4010905@alum.mit.edu","threadId":"33712","inReplyTo":"87ip2wflg0.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-06T10:45:37Z","receivedAt":"2013-05-06T10:45:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/06/2013 12:32 PM, Thomas Rast wrote:\n> Michael Haggerty writes:\n> \n>> On 05/03/2013 08:23 PM, Felipe Contreras wrote:\n>>> On Fri, May 3, 2013 at 12:56 PM, Thomas Rast wrote:\n>>>> Felipe Contreras writes:\n>>>\n>>>> How do we know that this doesn't break any users of fast-import? Your\n>>>> comment isn't very reassuring:\n>>>>\n>>>>> the vast majority of them will never be used again\n>>>>\n>>>> So what's with the minority?\n>>>\n>>> Actually I don't think there's any minority. If the client program\n>>> doesn't store blobs, the blob marks are not used anyway. So there's no\n>>> change.\n>>\n>> I haven't been following this conversation in detail, but your proposed\n>> change sounds like something that would break cvs2git [1]. Let me\n>> explain what cvs2git does and why:\n>>\n>> CVS stores all of the revisions of a single file in a single filename,v\n>> file in rcsfile(5) format. The revisions are stored as deltas ordered\n>> so that a single revision can be reconstructed from a single serial read\n>> of the file.\n>>\n>> cvs2git reads each of these files once, reconstructing *all* of the\n>> revisions for a file in a single go. It then pours them into a\n>> git-fast-import stream as blobs and sets a mark on each blob.\n>>\n>> Only much later in the conversion does it have enough information to\n>> reconstruct tree-wide commits. At that time it outputs git-fast-import\n>> data (to a second file) defining the git commits and their ancestry.\n>> The contents are defined by referring to the marks of blobs from the\n>> first git-fast-import stream file.\n>>\n>> This strategy speeds up the conversion *enormously*.\n>>\n>> So if I understand correctly that you are proposing to stop allowing\n>> marks on blob objects to be set and/or referred to later, then I object\n>> vociferously.\n> \n> The proposed patch wants to stop writing marks (in --export-marks) for\n> anything but commits. Does cvs2git depend on that? I.e., are you using\n> two separate fast-import processes for the blob and tree/commit phases\n> you describe above?\n\nYes, it can be handy to start loading the first \"blobfile\" in parallel\nwith the later stages of the conversion, before the second \"dumpfile\" is\nready. In that case the user needs to pass --export-marks to the first\nfast-import process to export marks on blobs so that the marks can be\npassed to the second fast-import via --import-marks.\n\nSo the proposed change would break a documented use of cvs2git.\n\nMaking the export of blob marks optional would of course be OK, as long\nas the default is to export them.\n\nMichael\n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"216490","messageId":"alpine.DEB.1.00.1305060720060.8213@s15462909.onlinehome-server.info","threadId":"33712","inReplyTo":"87ip2wflg0.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2013-05-06T12:20:51Z","receivedAt":"2013-05-06T12:20:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Thomas,\n\nOn Mon, 6 May 2013, Thomas Rast wrote:\n\n> The proposed patch wants to stop writing marks (in --export-marks) for\n> anything but commits.\n\nThen it should not go in.\n\nCiao,\nDscho\n"},{"id":"216504","messageId":"7v38u0t9va.fsf@alter.siamese.dyndns.org","threadId":"33712","inReplyTo":"518789D1.4010905@alum.mit.edu","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-06T15:18:33Z","receivedAt":"2013-05-06T15:18:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Yes, it can be handy to start loading the first \"blobfile\" in parallel\n> with the later stages of the conversion, before the second \"dumpfile\" is\n> ready. In that case the user needs to pass --export-marks to the first\n> fast-import process to export marks on blobs so that the marks can be\n> passed to the second fast-import via --import-marks.\n>\n> So the proposed change would break a documented use of cvs2git.\n>\n> Making the export of blob marks optional would of course be OK, as long\n> as the default is to export them.\n\nThanks for a concise summary. Your use case fits exactly what\nFelipe conjectured as the nonexistent minority.\n\nAn option that lets the caller say \"I only care about marks on these\ntypes of objects to be written to (and read from) the exported marks\nfile\" would help Felipe's use case without harming your use case,\nand would be a sane and safe way to go.\n"},{"id":"216551","messageId":"CAMP44s1Nk7YAjNkTq=ShQbzkMasw6bpcEPTXLb8x+2q-vXLRGg@mail.gmail.com","threadId":"33712","inReplyTo":"518789D1.4010905@alum.mit.edu","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-06T21:04:57Z","receivedAt":"2013-05-06T21:04:57Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, May 6, 2013 at 5:45 AM, Michael Haggerty wrote:\n> On 05/06/2013 12:32 PM, Thomas Rast wrote:\n>> Michael Haggerty writes:\n>>\n>>> On 05/03/2013 08:23 PM, Felipe Contreras wrote:\n>>>> On Fri, May 3, 2013 at 12:56 PM, Thomas Rast wrote:\n>>>>> Felipe Contreras writes:\n>>>>\n>>>>> How do we know that this doesn't break any users of fast-import? Your\n>>>>> comment isn't very reassuring:\n>>>>>\n>>>>>> the vast majority of them will never be used again\n>>>>>\n>>>>> So what's with the minority?\n>>>>\n>>>> Actually I don't think there's any minority. If the client program\n>>>> doesn't store blobs, the blob marks are not used anyway. So there's no\n>>>> change.\n>>>\n>>> I haven't been following this conversation in detail, but your proposed\n>>> change sounds like something that would break cvs2git [1]. Let me\n>>> explain what cvs2git does and why:\n>>>\n>>> CVS stores all of the revisions of a single file in a single filename,v\n>>> file in rcsfile(5) format. The revisions are stored as deltas ordered\n>>> so that a single revision can be reconstructed from a single serial read\n>>> of the file.\n>>>\n>>> cvs2git reads each of these files once, reconstructing *all* of the\n>>> revisions for a file in a single go. It then pours them into a\n>>> git-fast-import stream as blobs and sets a mark on each blob.\n>>>\n>>> Only much later in the conversion does it have enough information to\n>>> reconstruct tree-wide commits. At that time it outputs git-fast-import\n>>> data (to a second file) defining the git commits and their ancestry.\n>>> The contents are defined by referring to the marks of blobs from the\n>>> first git-fast-import stream file.\n>>>\n>>> This strategy speeds up the conversion *enormously*.\n>>>\n>>> So if I understand correctly that you are proposing to stop allowing\n>>> marks on blob objects to be set and/or referred to later, then I object\n>>> vociferously.\n>>\n>> The proposed patch wants to stop writing marks (in --export-marks) for\n>> anything but commits. Does cvs2git depend on that? I.e., are you using\n>> two separate fast-import processes for the blob and tree/commit phases\n>> you describe above?\n>\n> Yes, it can be handy to start loading the first \"blobfile\" in parallel\n> with the later stages of the conversion, before the second \"dumpfile\" is\n> ready. In that case the user needs to pass --export-marks to the first\n> fast-import process to export marks on blobs so that the marks can be\n> passed to the second fast-import via --import-marks.\n\nIt can be used that way, but it doesn't have to be.\n\n> So the proposed change would break a documented use of cvs2git.\n\nIt's documented as an alternative. How many people actually use this\nform over the other? Is there really any advantage? It's possibly that\nbasically nobody is using this form, and there's no benefits.\n\n> Making the export of blob marks optional would of course be OK, as long\n> as the default is to export them.\n\nNobody benefits from leaving the default as it is.\n\n-- \nFelipe Contreras\n"},{"id":"216552","messageId":"CAMP44s27ewVqLhJ5gT6T2B7V3vP=HX4Ughy9Q7zSi61X_9so_w@mail.gmail.com","threadId":"33712","inReplyTo":"alpine.DEB.1.00.1305060720060.8213@s15462909.onlinehome-server.info","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-06T21:06:45Z","receivedAt":"2013-05-06T21:06:45Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, May 6, 2013 at 7:20 AM, Johannes Schindelin\n wrote:\n> Hi Thomas,\n>\n> On Mon, 6 May 2013, Thomas Rast wrote:\n>\n>> The proposed patch wants to stop writing marks (in --export-marks) for\n>> anything but commits.\n>\n> Then it should not go in.\n\nIf that rationale was valid, no change in behavior should ever go in.\nOf course, that is not the case, we want to move forward, so changes\nin behavior do happen.\n\n-- \nFelipe Contreras\n"},{"id":"216553","messageId":"CAMP44s1HASAuF0ECCvJr66WeqopDzLZQ12pKFsc-j5_VCDrizg@mail.gmail.com","threadId":"33712","inReplyTo":"7v38u0t9va.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-06T21:19:03Z","receivedAt":"2013-05-06T21:19:03Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, May 6, 2013 at 10:18 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n>\n>> Yes, it can be handy to start loading the first \"blobfile\" in parallel\n>> with the later stages of the conversion, before the second \"dumpfile\" is\n>> ready. In that case the user needs to pass --export-marks to the first\n>> fast-import process to export marks on blobs so that the marks can be\n>> passed to the second fast-import via --import-marks.\n>>\n>> So the proposed change would break a documented use of cvs2git.\n>>\n>> Making the export of blob marks optional would of course be OK, as long\n>> as the default is to export them.\n>\n> Thanks for a concise summary. Your use case fits exactly what\n> Felipe conjectured as the nonexistent minority.\n\nNot true. cvs2git does *not* rely on the blobs being stored in a marks\nfile, because cvs2git does not rely on mark files at all.\n\n> An option that lets the caller say \"I only care about marks on these\n> types of objects to be written to (and read from) the exported marks\n> file\" would help Felipe's use case without harming your use case,\n> and would be a sane and safe way to go.\n\nHis case is not harmed at all. It's only the unfortunate command that\nis mentioned in the documentation that didn't need to be mentioned at\nall in the first place.\n\nIt should be the other way around, if it's only this documentation\nthat is affected, we could add a switch for that particular command,\nand the documentation should be updated, but it's overkill to add a\nswitch for one odd command in some documentation somewhere, it would\nbe much better to update the odd command to avoid using marks at all,\nwhich is what the more appropriate command does, right below in the\nsame documentation.\n\n cat ../cvs2svn-tmp/git-blob.dat ../cvs2svn-tmp/git-dump.dat | git fast-import\n\nShould the rest of the real world be punished because somebody added a\ncommand in some documentation somewhere, which wasn't actually needed\nin the first place?\n\n-- \nFelipe Contreras\n"},{"id":"216557","messageId":"CAMP44s2KHH1n0vHB0Mdv-M2xV97KA9FMc6UrPyYyxD+2jsvfTA@mail.gmail.com","threadId":"33712","inReplyTo":"CAMP44s1HASAuF0ECCvJr66WeqopDzLZQ12pKFsc-j5_VCDrizg@mail.gmail.com","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-06T21:36:18Z","receivedAt":"2013-05-06T21:36:18Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, May 6, 2013 at 4:19 PM, Felipe Contreras\n wrote:\n> On Mon, May 6, 2013 at 10:18 AM, Junio C Hamano wrote:\n>> Michael Haggerty writes:\n>>\n>>> Yes, it can be handy to start loading the first \"blobfile\" in parallel\n>>> with the later stages of the conversion, before the second \"dumpfile\" is\n>>> ready. In that case the user needs to pass --export-marks to the first\n>>> fast-import process to export marks on blobs so that the marks can be\n>>> passed to the second fast-import via --import-marks.\n>>>\n>>> So the proposed change would break a documented use of cvs2git.\n>>>\n>>> Making the export of blob marks optional would of course be OK, as long\n>>> as the default is to export them.\n>>\n>> Thanks for a concise summary. Your use case fits exactly what\n>> Felipe conjectured as the nonexistent minority.\n>\n> Not true. cvs2git does *not* rely on the blobs being stored in a marks\n> file, because cvs2git does not rely on mark files at all.\n>\n>> An option that lets the caller say \"I only care about marks on these\n>> types of objects to be written to (and read from) the exported marks\n>> file\" would help Felipe's use case without harming your use case,\n>> and would be a sane and safe way to go.\n>\n> His case is not harmed at all. It's only the unfortunate command that\n> is mentioned in the documentation that didn't need to be mentioned at\n> all in the first place.\n>\n> It should be the other way around, if it's only this documentation\n> that is affected, we could add a switch for that particular command,\n> and the documentation should be updated, but it's overkill to add a\n> switch for one odd command in some documentation somewhere, it would\n> be much better to update the odd command to avoid using marks at all,\n> which is what the more appropriate command does, right below in the\n> same documentation.\n\nThis would simplify the documentation, and obliterate the need to use\nmark files at all:\n\ndiff -ur cvs2svn-2.4.0/www/cvs2git.html cvs2svn-2.4.0-mod/www/cvs2git.html\n--- cvs2svn-2.4.0/www/cvs2git.html\t2012-09-22 01:49:55.000000000 -0500\n+++ cvs2svn-2.4.0-mod/www/cvs2git.html\t2013-05-06 16:33:12.070189985 -0500\n@@ -355,14 +355,13 @@\n fast-import:

\n\n 
\n-git fast-import --export-marks=../cvs2svn-tmp/git-marks.dat <\n../cvs2svn-tmp/git-blob.dat\n-git fast-import --import-marks=../cvs2svn-tmp/git-marks.dat <\n../cvs2svn-tmp/git-dump.dat\n+cat ../cvs2svn-tmp/git-blob.dat ../cvs2svn-tmp/git-dump.dat | git fast-import\n
\n\n-

On Linux/Unix this can be shortened to:

\n+

On Windows you should use type instead:

\n\n 
\n-cat ../cvs2svn-tmp/git-blob.dat ../cvs2svn-tmp/git-dump.dat | git fast-import\n+type ../cvs2svn-tmp/git-blob.dat ../cvs2svn-tmp/git-dump.dat | git fast-import\n
\n\n \nOnly in cvs2svn-2.4.0-mod/www: .cvs2git.html.swp\n\n\n-- \nFelipe Contreras\n"},{"id":"216577","messageId":"51886DE5.7030001@alum.mit.edu","threadId":"33712","inReplyTo":"CAMP44s1HASAuF0ECCvJr66WeqopDzLZQ12pKFsc-j5_VCDrizg@mail.gmail.com","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-07T02:58:45Z","receivedAt":"2013-05-07T02:58:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/06/2013 11:19 PM, Felipe Contreras wrote:\n> On Mon, May 6, 2013 at 10:18 AM, Junio C Hamano wrote:\n>> Michael Haggerty writes:\n>>\n>>> Yes, it can be handy to start loading the first \"blobfile\" in parallel\n>>> with the later stages of the conversion, before the second \"dumpfile\" is\n>>> ready. In that case the user needs to pass --export-marks to the first\n>>> fast-import process to export marks on blobs so that the marks can be\n>>> passed to the second fast-import via --import-marks.\n>>>\n>>> So the proposed change would break a documented use of cvs2git.\n>>>\n>>> Making the export of blob marks optional would of course be OK, as long\n>>> as the default is to export them.\n>>\n>> Thanks for a concise summary. Your use case fits exactly what\n>> Felipe conjectured as the nonexistent minority.\n> \n> Not true. cvs2git does *not* rely on the blobs being stored in a marks\n> file, because cvs2git does not rely on mark files at all.\n> \n>> An option that lets the caller say \"I only care about marks on these\n>> types of objects to be written to (and read from) the exported marks\n>> file\" would help Felipe's use case without harming your use case,\n>> and would be a sane and safe way to go.\n> \n> His case is not harmed at all. It's only the unfortunate command that\n> is mentioned in the documentation that didn't need to be mentioned at\n> all in the first place.\n> \n> It should be the other way around, if it's only this documentation\n> that is affected, we could add a switch for that particular command,\n> and the documentation should be updated, but it's overkill to add a\n> switch for one odd command in some documentation somewhere, it would\n> be much better to update the odd command to avoid using marks at all,\n> which is what the more appropriate command does, right below in the\n> same documentation.\n> \n> cat ../cvs2svn-tmp/git-blob.dat ../cvs2svn-tmp/git-dump.dat | git fast-import\n> \n> Should the rest of the real world be punished because somebody added a\n> command in some documentation somewhere, which wasn't actually needed\n> in the first place?\n\nDon't get too fixated on the documentation. The documentation just\ngives some examples of how cvs2git can be used.\n\nThe reason that cvs2git outputs two files is that the first file is\nemitted at the very beginning of the conversion and the second at the\nvery end. These conversions can take a long time (> 1 day for very big\nrepos), can be interrupted and restarted between \"passes\", and passes\ncan even be re-run with changed configurations.\n\nCVS write access has to be turned off before the start of the final\nconversion, so no VCS is possible until the conversion is over. So\nusers are very interested in keeping the downtime minimal. The blobfile\ncan also be unwieldy (its size is approximately the sum of the sizes of\nall revisions of all files in the project). Being able to load the\nblobfile into one fast-import process and the dumpfile into a different\nprocess (which relies on the feature that you propose removing) opens up\na lot of possibilities:\n\n* The first fast-import of the blobfile can be started as soon as the\nblobfile is complete and run in parallel with the rest of the conversion.\n\n* If the blobfile needs to be transferred over the network (e.g.,\nbecause Git will be served from a different server than the one doing\nthe conversion) the network transfer can also be done in parallel with\nthe rest of the conversion.\n\n* The blobfile could be written to a named pipe that is being read by a\ngit-fast-import process, to avoid having to write the blobfile to disk\nin the first place.\n\n* The user could run \"git repack\" between loading the blobfile and\nloading the dumpfile.\n\nThese are just the ways that cvs2git does and/or could benefit from the\nflexibility that is now in git-fast-import. Other tools might also be\nusing git-fast-import in ways that would be broken by your proposed change.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"216579","messageId":"518871A0.8030707@alum.mit.edu","threadId":"33712","inReplyTo":"CAMP44s2KHH1n0vHB0Mdv-M2xV97KA9FMc6UrPyYyxD+2jsvfTA@mail.gmail.com","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-07T03:14:40Z","receivedAt":"2013-05-07T03:14:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/06/2013 11:36 PM, Felipe Contreras wrote:\n> This would simplify the documentation, and obliterate the need to use\n> mark files at all:\n\nAs explained in my other email, this documentation change does not\nremove all of the reasons that users might want to use mark files. I\nwould still like to show users how they can load the files into Git as\ntwo separate steps.\n\n> diff -ur cvs2svn-2.4.0/www/cvs2git.html cvs2svn-2.4.0-mod/www/cvs2git.html\n> --- cvs2svn-2.4.0/www/cvs2git.html\t2012-09-22 01:49:55.000000000 -0500\n> +++ cvs2svn-2.4.0-mod/www/cvs2git.html\t2013-05-06 16:33:12.070189985 -0500\n> @@ -355,14 +355,13 @@\n> fast-import:

\n> \n> 
\n> -git fast-import --export-marks=../cvs2svn-tmp/git-marks.dat <\n> ../cvs2svn-tmp/git-blob.dat\n> -git fast-import --import-marks=../cvs2svn-tmp/git-marks.dat <\n> ../cvs2svn-tmp/git-dump.dat\n> +cat ../cvs2svn-tmp/git-blob.dat ../cvs2svn-tmp/git-dump.dat | git fast-import\n>
\n> \n> -

On Linux/Unix this can be shortened to:

\n> +

On Windows you should use type instead:

\n> \n> 
\n> -cat ../cvs2svn-tmp/git-blob.dat ../cvs2svn-tmp/git-dump.dat | git fast-import\n> +type ../cvs2svn-tmp/git-blob.dat ../cvs2svn-tmp/git-dump.dat | git fast-import\n>
\n> \n> \n> Only in cvs2svn-2.4.0-mod/www: .cvs2git.html.swp\n> \n> \n\nNevertheless, it *would* be nice to add the Windows equivalent of the\n\"cat\" pipeline. I knew about the \"type\" command but I was under the\nimpression that it is intended for text files and can corrupt binary\nfiles. Are you sure that using \"type\" as you suggest is binary-clean?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"216580","messageId":"518874A5.5050002@alum.mit.edu","threadId":"33712","inReplyTo":"CAMP44s1Nk7YAjNkTq=ShQbzkMasw6bpcEPTXLb8x+2q-vXLRGg@mail.gmail.com","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-07T03:27:33Z","receivedAt":"2013-05-07T03:27:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/06/2013 11:04 PM, Felipe Contreras wrote:\n> On Mon, May 6, 2013 at 5:45 AM, Michael Haggerty wrote:\n>> On 05/06/2013 12:32 PM, Thomas Rast wrote:\n>>> Michael Haggerty writes:\n>>>\n>>>> On 05/03/2013 08:23 PM, Felipe Contreras wrote:\n>>>>> On Fri, May 3, 2013 at 12:56 PM, Thomas Rast wrote:\n>>>>>> Felipe Contreras writes:\n>>>>>\n>>>>>> How do we know that this doesn't break any users of fast-import? Your\n>>>>>> comment isn't very reassuring:\n>>>>>>\n>>>>>>> the vast majority of them will never be used again\n>>>>>>\n>>>>>> So what's with the minority?\n>>>>>\n>>>>> Actually I don't think there's any minority. If the client program\n>>>>> doesn't store blobs, the blob marks are not used anyway. So there's no\n>>>>> change.\n>>>>\n>>>> I haven't been following this conversation in detail, but your proposed\n>>>> change sounds like something that would break cvs2git [1]. Let me\n>>>> explain what cvs2git does and why:\n>>>>\n>>>> CVS stores all of the revisions of a single file in a single filename,v\n>>>> file in rcsfile(5) format. The revisions are stored as deltas ordered\n>>>> so that a single revision can be reconstructed from a single serial read\n>>>> of the file.\n>>>>\n>>>> cvs2git reads each of these files once, reconstructing *all* of the\n>>>> revisions for a file in a single go. It then pours them into a\n>>>> git-fast-import stream as blobs and sets a mark on each blob.\n>>>>\n>>>> Only much later in the conversion does it have enough information to\n>>>> reconstruct tree-wide commits. At that time it outputs git-fast-import\n>>>> data (to a second file) defining the git commits and their ancestry.\n>>>> The contents are defined by referring to the marks of blobs from the\n>>>> first git-fast-import stream file.\n>>>>\n>>>> This strategy speeds up the conversion *enormously*.\n>>>>\n>>>> So if I understand correctly that you are proposing to stop allowing\n>>>> marks on blob objects to be set and/or referred to later, then I object\n>>>> vociferously.\n>>>\n>>> The proposed patch wants to stop writing marks (in --export-marks) for\n>>> anything but commits. Does cvs2git depend on that? I.e., are you using\n>>> two separate fast-import processes for the blob and tree/commit phases\n>>> you describe above?\n>>\n>> Yes, it can be handy to start loading the first \"blobfile\" in parallel\n>> with the later stages of the conversion, before the second \"dumpfile\" is\n>> ready. In that case the user needs to pass --export-marks to the first\n>> fast-import process to export marks on blobs so that the marks can be\n>> passed to the second fast-import via --import-marks.\n> \n> It can be used that way, but it doesn't have to be.\n\nPlease see my other email for an explanation of how the flexibility of\nloading the two files separately can bring concrete benefits.\n\n>> So the proposed change would break a documented use of cvs2git.\n> \n> It's documented as an alternative. How many people actually use this\n> form over the other? Is there really any advantage? It's possibly that\n> basically nobody is using this form, and there's no benefits.\n\nYou conjectured earlier that nobody uses blob marks, and I provided a\ncounterexample. Then you proposed a workaround that would require\nchanges to the cvs2git documentation, and I even explained how your\nproposed workaround is not as flexible as the status quo. Do you want\nto go through the same argument with every possible user of git-fast-import?\n\nIt would be insanity to change the default behavior when a workaround on\nthe Git side (namely adding an option that tells git-fast-import *not*\nto emit blob marks) would be quite straightforward to implement and have\nlittle maintenance cost.\n\n>> Making the export of blob marks optional would of course be OK, as long\n>> as the default is to export them.\n> \n> Nobody benefits from leaving the default as it is.\n\nSure they do. Any tool that *knows* that it doesn't need blob marks can\npass the new option and benefit. Other tools benefit from not being\nbroken by your change.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"216585","messageId":"alpine.DEB.1.00.1305062331190.8213@s15462909.onlinehome-server.info","threadId":"33712","inReplyTo":"518871A0.8030707@alum.mit.edu","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2013-05-07T04:32:29Z","receivedAt":"2013-05-07T04:32:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michael,\n\nOn Tue, 7 May 2013, Michael Haggerty wrote:\n\n> I knew about the \"type\" command but I was under the impression that it\n> is intended for text files and can corrupt binary files. Are you sure\n> that using \"type\" as you suggest is binary-clean?\n\n\"type\" is not binary-clean. At least on some Windows versions, \"type\" also\nhas a limit on file size.\n\nCiao,\nDscho\n"},{"id":"216586","messageId":"CAMP44s2-UNVMyTgUVMOagegB+_Gr9drXy-5s=3G+nTNKW=ZZ_Q@mail.gmail.com","threadId":"33712","inReplyTo":"alpine.DEB.1.00.1305062331190.8213@s15462909.onlinehome-server.info","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T04:36:59Z","receivedAt":"2013-05-07T04:36:59Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, May 6, 2013 at 11:32 PM, Johannes Schindelin\n wrote:\n> Hi Michael,\n>\n> On Tue, 7 May 2013, Michael Haggerty wrote:\n>\n>> I knew about the \"type\" command but I was under the impression that it\n>> is intended for text files and can corrupt binary files. Are you sure\n>> that using \"type\" as you suggest is binary-clean?\n>\n> \"type\" is not binary-clean. At least on some Windows versions, \"type\" also\n> has a limit on file size.\n\ncopy /b file1+file2 destfile\n\nThen.\n\n-- \nFelipe Contreras\n"},{"id":"216587","messageId":"CAMP44s09kkVKSKWhFfES=wCsL5CL6FMDMmZFdCfKCjc_-h=o8A@mail.gmail.com","threadId":"33712","inReplyTo":"51886DE5.7030001@alum.mit.edu","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T04:37:13Z","receivedAt":"2013-05-07T04:37:13Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, May 6, 2013 at 9:58 PM, Michael Haggerty wrote:\n> On 05/06/2013 11:19 PM, Felipe Contreras wrote:\n>> On Mon, May 6, 2013 at 10:18 AM, Junio C Hamano wrote:\n>>> Michael Haggerty writes:\n>>>\n>>>> Yes, it can be handy to start loading the first \"blobfile\" in parallel\n>>>> with the later stages of the conversion, before the second \"dumpfile\" is\n>>>> ready. In that case the user needs to pass --export-marks to the first\n>>>> fast-import process to export marks on blobs so that the marks can be\n>>>> passed to the second fast-import via --import-marks.\n>>>>\n>>>> So the proposed change would break a documented use of cvs2git.\n>>>>\n>>>> Making the export of blob marks optional would of course be OK, as long\n>>>> as the default is to export them.\n>>>\n>>> Thanks for a concise summary. Your use case fits exactly what\n>>> Felipe conjectured as the nonexistent minority.\n>>\n>> Not true. cvs2git does *not* rely on the blobs being stored in a marks\n>> file, because cvs2git does not rely on mark files at all.\n>>\n>>> An option that lets the caller say \"I only care about marks on these\n>>> types of objects to be written to (and read from) the exported marks\n>>> file\" would help Felipe's use case without harming your use case,\n>>> and would be a sane and safe way to go.\n>>\n>> His case is not harmed at all. It's only the unfortunate command that\n>> is mentioned in the documentation that didn't need to be mentioned at\n>> all in the first place.\n>>\n>> It should be the other way around, if it's only this documentation\n>> that is affected, we could add a switch for that particular command,\n>> and the documentation should be updated, but it's overkill to add a\n>> switch for one odd command in some documentation somewhere, it would\n>> be much better to update the odd command to avoid using marks at all,\n>> which is what the more appropriate command does, right below in the\n>> same documentation.\n>>\n>> cat ../cvs2svn-tmp/git-blob.dat ../cvs2svn-tmp/git-dump.dat | git fast-import\n>>\n>> Should the rest of the real world be punished because somebody added a\n>> command in some documentation somewhere, which wasn't actually needed\n>> in the first place?\n>\n> Don't get too fixated on the documentation. The documentation just\n> gives some examples of how cvs2git can be used.\n>\n> The reason that cvs2git outputs two files is that the first file is\n> emitted at the very beginning of the conversion and the second at the\n> very end. These conversions can take a long time (> 1 day for very big\n> repos), can be interrupted and restarted between \"passes\", and passes\n> can even be re-run with changed configurations.\n>\n> CVS write access has to be turned off before the start of the final\n> conversion, so no VCS is possible until the conversion is over. So\n> users are very interested in keeping the downtime minimal.\n\nOf course VCS is possible before the conversion is over, you can do\nall the cvs2git commands first, turn on CVS, and let 'git fast-import'\ndo it's thing afterwards, can you not?\n\n> The blobfile\n> can also be unwieldy (its size is approximately the sum of the sizes of\n> all revisions of all files in the project). Being able to load the\n> blobfile into one fast-import process and the dumpfile into a different\n> process (which relies on the feature that you propose removing) opens up\n> a lot of possibilities:\n>\n> * The first fast-import of the blobfile can be started as soon as the\n> blobfile is complete and run in parallel with the rest of the conversion.\n\nIs that reason enough to punish everyone else?\n\n> * If the blobfile needs to be transferred over the network (e.g.,\n> because Git will be served from a different server than the one doing\n> the conversion) the network transfer can also be done in parallel with\n> the rest of the conversion.\n\nDitto.\n\n> * The blobfile could be written to a named pipe that is being read by a\n> git-fast-import process, to avoid having to write the blobfile to disk\n> in the first place.\n\nAnd it would still be possible.\n\n{ cat file1; cat file2; } > pipe\n\nWhat am I missing?\n\n> * The user could run \"git repack\" between loading the blobfile and\n> loading the dumpfile.\n\nIs that reason enough to punish everyone else?\n\n> These are just the ways that cvs2git does and/or could benefit from the\n> flexibility that is now in git-fast-import.\n\nSure, all those things _migt_ be nice, but nothing would be broken,\nwould it? cvs2git can still achieve it's goal without it.\n\nAnd if this \"feature\" is removed by *default*, it would be trivial to\nenable an option to tell 'git fast-import' to store blobs from\ncvs2git, would it not?\n\n> Other tools might also be\n> using git-fast-import in ways that would be broken by your proposed change.\n\nYeah, everything is possible, but the key word is *might*. Will\nanything be *actually* broken by this change? My guess is, very few\nthings, if any.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"216588","messageId":"alpine.DEB.1.00.1305062332470.8213@s15462909.onlinehome-server.info","threadId":"33712","inReplyTo":"518874A5.5050002@alum.mit.edu","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2013-05-07T04:39:49Z","receivedAt":"2013-05-07T04:39:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 7 May 2013, Michael Haggerty wrote:\n\n> On 05/06/2013 11:04 PM, Felipe Contreras wrote:\n> > On Mon, May 6, 2013 at 5:45 AM, Michael Haggerty wrote:\n> >> On 05/06/2013 12:32 PM, Thomas Rast wrote:\n\n> >> So the proposed change would break a documented use of cvs2git.\n> > \n> > It's documented as an alternative. How many people actually use this\n> > form over the other? Is there really any advantage? It's possibly that\n> > basically nobody is using this form, and there's no benefits.\n> \n> You conjectured earlier that nobody uses blob marks, and I provided a\n> counterexample. Then you proposed a workaround that would require\n> changes to the cvs2git documentation, and I even explained how your\n> proposed workaround is not as flexible as the status quo. Do you want\n> to go through the same argument with every possible user of\n> git-fast-import?\n> \n> It would be insanity to change the default behavior when a workaround on\n> the Git side (namely adding an option that tells git-fast-import *not*\n> to emit blob marks) would be quite straightforward to implement and have\n> little maintenance cost.\n\nI really wonder how many more counterexamples are required to settle this\ndiscussion.\n\nThere is no good reason to artificially limit Git's capabilities here,\nespecially when it has been demonstrated that supporting that capability\nis not only possible, but also outright easy.\n\nCiao,\nDscho\n"},{"id":"216590","messageId":"CAMP44s3vGKv-Z6sx0sjAPH7_U1+AiJjFHjKz8ke+EUqW-u2Y=A@mail.gmail.com","threadId":"33712","inReplyTo":"518874A5.5050002@alum.mit.edu","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T04:47:37Z","receivedAt":"2013-05-07T04:47:37Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, May 6, 2013 at 10:27 PM, Michael Haggerty wrote:\n\n> You conjectured earlier that nobody uses blob marks, and I provided a\n> counterexample. Then you proposed a workaround that would require\n> changes to the cvs2git documentation, and I even explained how your\n> proposed workaround is not as flexible as the status quo.\n\ncvs2git does *not* need blob marks, it does not need marks at all.\n\nThe use-case that you mentioned has nothing to do with cvs2git, in\nfact. I can be described as this:\n\n% ./generate-blobs > blobs\n% git fast-import --export-marks=marks < blobs\n% ./generate-commits > commits\n% git fast-import --import-marks=marks < commits\n\nIn this example 'generate-commits' has no notion of marks at all, and\n'git fast-import' doesn't need marks to process both blobs and\ncommits.\n\n> Do you want\n> to go through the same argument with every possible user of git-fast-import?\n\nI don't care about possible users, I care about *real* users.\n\n> It would be insanity to change the default behavior when a workaround on\n> the Git side (namely adding an option that tells git-fast-import *not*\n> to emit blob marks) would be quite straightforward to implement and have\n> little maintenance cost.\n\nAnd nobody would benefit from that.\n\n>>> Making the export of blob marks optional would of course be OK, as long\n>>> as the default is to export them.\n>>\n>> Nobody benefits from leaving the default as it is.\n>\n> Sure they do. Any tool that *knows* that it doesn't need blob marks can\n> pass the new option and benefit. Other tools benefit from not being\n> broken by your change.\n\nWhich the *vastly* more common case? That blobs are needed, or that\nthey are not?\n\n-- \nFelipe Contreras\n"},{"id":"216592","messageId":"CAMP44s3OZX=f8=MdXq8jr57o94ubzqf0O3EVFKn4QdTXdg+A=Q@mail.gmail.com","threadId":"33712","inReplyTo":"alpine.DEB.1.00.1305062332470.8213@s15462909.onlinehome-server.info","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T04:49:56Z","receivedAt":"2013-05-07T04:49:56Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, May 6, 2013 at 11:39 PM, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Tue, 7 May 2013, Michael Haggerty wrote:\n>\n>> On 05/06/2013 11:04 PM, Felipe Contreras wrote:\n>> > On Mon, May 6, 2013 at 5:45 AM, Michael Haggerty wrote:\n>> >> On 05/06/2013 12:32 PM, Thomas Rast wrote:\n>\n>> >> So the proposed change would break a documented use of cvs2git.\n>> >\n>> > It's documented as an alternative. How many people actually use this\n>> > form over the other? Is there really any advantage? It's possibly that\n>> > basically nobody is using this form, and there's no benefits.\n>>\n>> You conjectured earlier that nobody uses blob marks, and I provided a\n>> counterexample. Then you proposed a workaround that would require\n>> changes to the cvs2git documentation, and I even explained how your\n>> proposed workaround is not as flexible as the status quo. Do you want\n>> to go through the same argument with every possible user of\n>> git-fast-import?\n>>\n>> It would be insanity to change the default behavior when a workaround on\n>> the Git side (namely adding an option that tells git-fast-import *not*\n>> to emit blob marks) would be quite straightforward to implement and have\n>> little maintenance cost.\n>\n> I really wonder how many more counterexamples are required to settle this\n> discussion.\n\nOne. I haven't seen one use-case that *requires* blob marks, I haven't\nseen one tool that utilizes blob marks. And no, cvs2git doesn't use\nmarks at all.\n\n> There is no good reason to artificially limit Git's capabilities here,\n> especially when it has been demonstrated that supporting that capability\n> is not only possible, but also outright easy.\n\nStrawman. Nobody is arguing that there shouldn't be an option to\nenable blob exporting, the argument is that there's no point in making\nthat the *default*.\n\n-- \nFelipe Contreras\n"},{"id":"216601","messageId":"5188A36A.8000408@alum.mit.edu","threadId":"33712","inReplyTo":"CAMP44s3vGKv-Z6sx0sjAPH7_U1+AiJjFHjKz8ke+EUqW-u2Y=A@mail.gmail.com","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-07T06:47:06Z","receivedAt":"2013-05-07T06:47:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/07/2013 06:47 AM, Felipe Contreras wrote:\n> On Mon, May 6, 2013 at 10:27 PM, Michael Haggerty wrote:\n> \n>> You conjectured earlier that nobody uses blob marks, and I provided a\n>> counterexample. Then you proposed a workaround that would require\n>> changes to the cvs2git documentation, and I even explained how your\n>> proposed workaround is not as flexible as the status quo.\n> \n> cvs2git does *not* need blob marks, it does not need marks at all.\n> \n> The use-case that you mentioned has nothing to do with cvs2git, in\n> fact. I can be described as this:\n> \n> % ./generate-blobs > blobs\n> % git fast-import --export-marks=marks < blobs\n> % ./generate-commits > commits\n> % git fast-import --import-marks=marks < commits\n> \n> In this example 'generate-commits' has no notion of marks at all, and\n> 'git fast-import' doesn't need marks to process both blobs and\n> commits.\n\nThe \"generate-blobs\" program generates a mark for each blob and\nremembers the marks in a database. The \"generate-commits\" program reads\nthe marks from the database and incorporates them in the definitions of\nthe commits that it writes to its output. So yes, the program pair\n*does* rely on marks for blobs being exported correctly.\n\nI've tired of this discussion. I am quite sure that your change will\nnot be accepted, so I see no need to participate further. Please do not\ninterpret my silence as agreement with your quarrelsome arguments.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"216603","messageId":"CAMP44s1Y6rjyKZ_CGWJJ81s16DTu7cqKeFY48J_ndzpp2t8=dQ@mail.gmail.com","threadId":"33712","inReplyTo":"5188A36A.8000408@alum.mit.edu","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T07:07:45Z","receivedAt":"2013-05-07T07:07:45Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, May 7, 2013 at 1:47 AM, Michael Haggerty wrote:\n> On 05/07/2013 06:47 AM, Felipe Contreras wrote:\n>> On Mon, May 6, 2013 at 10:27 PM, Michael Haggerty wrote:\n>>\n>>> You conjectured earlier that nobody uses blob marks, and I provided a\n>>> counterexample. Then you proposed a workaround that would require\n>>> changes to the cvs2git documentation, and I even explained how your\n>>> proposed workaround is not as flexible as the status quo.\n>>\n>> cvs2git does *not* need blob marks, it does not need marks at all.\n>>\n>> The use-case that you mentioned has nothing to do with cvs2git, in\n>> fact. I can be described as this:\n>>\n>> % ./generate-blobs > blobs\n>> % git fast-import --export-marks=marks < blobs\n>> % ./generate-commits > commits\n>> % git fast-import --import-marks=marks < commits\n>>\n>> In this example 'generate-commits' has no notion of marks at all, and\n>> 'git fast-import' doesn't need marks to process both blobs and\n>> commits.\n>\n> The \"generate-blobs\" program generates a mark for each blob and\n> remembers the marks in a database. The \"generate-commits\" program reads\n> the marks from the database and incorporates them in the definitions of\n> the commits that it writes to its output. So yes, the program pair\n> *does* rely on marks for blobs being exported correctly.\n\nFine:\n\n% ./generate-data > data\n% ./split-blobs data > blobs\n% ./split-commits data > commits\n\nHow exactly the files 'blobs' and 'commits' are generated is\nirrelevant, 'git fast-import' will need them *once*, and never again,\nin fact, doesn't even need to know they are two files. There's no need\nfor marks.\n\n> I've tired of this discussion. I am quite sure that your change will\n> not be accepted, so I see no need to participate further. Please do not\n> interpret my silence as agreement with your quarrelsome arguments.\n\nHow convenient. I ask the though questions, and you suddenly get tired\nof the discussion. So, let me answer for you:\n\n* Which the *vastly* more common case? That blobs are needed, or that\nthey are not?\n\nThat they are not. To suggest otherwise would be ridiculous.\n\nAnd of course this patch will not be accepted, because nobody is\ninterested in improving things in the most easy and sensible way.\n\nYours and everybody else's argument is one and the same: status quo,\nwhich is of course, not an argument at all.\n\nBut it's pointless to try to convince you, because a) you are not\ninterested in changing your mind b) you are not interested in seeking\nthe most sensible solution c) you are only interested in doing what\nyou are used to do, which is to not change anything, ever d) you know\nmaking this the default is only slightly risky, at best, and not only\nthe world is not going to end, but most likely nobody would even\nnotice, and the ones that would, are probably already participating in\nthis conversation, but you don't even want to do something slightly\nrisky, because it would show that others were right, and your concerns\ndon't actually affect any real users at all.\n\nBut as I said, drop the patch. Who needs progress?\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"216604","messageId":"7vehdjmffp.fsf@alter.siamese.dyndns.org","threadId":"33712","inReplyTo":"518874A5.5050002@alum.mit.edu","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-07T07:12:26Z","receivedAt":"2013-05-07T07:12:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n>>>>> CVS stores all of the revisions of a single file in a single filename,v\n>>>>> file in rcsfile(5) format. The revisions are stored as deltas ordered\n>>>>> so that a single revision can be reconstructed from a single serial read\n>>>>> of the file.\n>>>>>\n>>>>> cvs2git reads each of these files once, reconstructing *all* of the\n>>>>> revisions for a file in a single go. It then pours them into a\n>>>>> git-fast-import stream as blobs and sets a mark on each blob.\n\nThis is more or less off-topic but in the bigger picture it is more\ninteresting and important X-<.\n\nThe way you describe how cvs2git handles the blobs is the more\nefficient way, given that fast-import does not even attempt to\nbother to create good deltas. The only thing it does is to see if\nthe current data deltifies against the last object.\n\nIIRC, CVS's backend storage is mostly recorded in backward delta, so\nif you are feeding the blob data from new to old, then the resulting\npack would follow Linus's law (the file generally grows over time)\nand would generally give you a good deltified chain of objects.\n"},{"id":"216605","messageId":"5188AE6F.3030701@alum.mit.edu","threadId":"33712","inReplyTo":"7vehdjmffp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] fast-import: only store commit objects","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-07T07:34:07Z","receivedAt":"2013-05-07T07:34:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/07/2013 09:12 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>>>>>> CVS stores all of the revisions of a single file in a single filename,v\n>>>>>> file in rcsfile(5) format. The revisions are stored as deltas ordered\n>>>>>> so that a single revision can be reconstructed from a single serial read\n>>>>>> of the file.\n>>>>>>\n>>>>>> cvs2git reads each of these files once, reconstructing *all* of the\n>>>>>> revisions for a file in a single go. It then pours them into a\n>>>>>> git-fast-import stream as blobs and sets a mark on each blob.\n> \n> This is more or less off-topic but in the bigger picture it is more\n> interesting and important X-<.\n> \n> The way you describe how cvs2git handles the blobs is the more\n> efficient way, given that fast-import does not even attempt to\n> bother to create good deltas. The only thing it does is to see if\n> the current data deltifies against the last object.\n> \n> IIRC, CVS's backend storage is mostly recorded in backward delta, so\n> if you are feeding the blob data from new to old, then the resulting\n> pack would follow Linus's law (the file generally grows over time)\n> and would generally give you a good deltified chain of objects.\n\nYes, you are correct about how CVS orders commits on the mainline.\nBranches are stored in the opposite order--oldest to newest--but CVS\nusers don't tend to get carried away with branches anyway, and if the\nchanges are small deltafication should help a lot anyway.\n\nCool. I knew that fast-import didn't do much in the way of compression,\nbut I didn't realize that it can compute deltas only between adjacent\nblobs. So cvs2git happily hits the sweet-spot of fast-import.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}