{"thread":{"id":"33730","subject":"[PATCH v2 0/3] fast-export: speed improvements","startedAt":"2013-05-05T22:38:51Z","lastAt":"2013-05-07T23:19:09Z","messageCount":25,"participants":["Felipe Contreras","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"216467","messageId":"1367793534-8401-1-git-send-email-felipe.contreras@gmail.com","threadId":"33730","inReplyTo":null,"subject":"[PATCH v2 0/3] fast-export: speed improvements","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-05T22:38:51Z","receivedAt":"2013-05-05T22:38:51Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nSecond try, dropped one fix, and address a few comments.\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 (3):\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\n builtin/fast-export.c | 22 +++++++++++++++-------\n fast-import.c | 10 +++++-----\n 2 files changed, 20 insertions(+), 12 deletions(-)\n\n-- \n1.8.3.rc0.401.g45bba44\n"},{"id":"216468","messageId":"1367793534-8401-2-git-send-email-felipe.contreras@gmail.com","threadId":"33730","inReplyTo":"1367793534-8401-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 1/3] fast-{import,export}: use get_sha1_hex() directly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-05T22:38:52Z","receivedAt":"2013-05-05T22:38:52Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"It's wrong to call get_sha1() if they should be SHA-1s, plus\ninefficient.\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":"216469","messageId":"1367793534-8401-3-git-send-email-felipe.contreras@gmail.com","threadId":"33730","inReplyTo":"1367793534-8401-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-05T22:38:53Z","receivedAt":"2013-05-05T22:38:53Z","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":"216470","messageId":"1367793534-8401-4-git-send-email-felipe.contreras@gmail.com","threadId":"33730","inReplyTo":"1367793534-8401-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 3/3] fast-export: don't parse all the commits","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-05T22:38:54Z","receivedAt":"2013-05-05T22:38:54Z","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 | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex a5b8da8..d1d68e9 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\tstruct commit *commit;\n \t\tenum object_type type;\n \n \t\tline_end = strchr(line, '\\n');\n@@ -636,7 +637,11 @@ 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\tcommit = lookup_commit(sha1);\n+\t\tif (!commit)\n+\t\t\tdie(\"not a commit? can't happen: %s\", sha1_to_hex(sha1));\n+\n+\t\tobject = &commit->object;\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":"216491","messageId":"20130506123111.GB3809@sigill.intra.peff.net","threadId":"33730","inReplyTo":"1367793534-8401-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-06T12:31:11Z","receivedAt":"2013-05-06T12:31:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 05, 2013 at 05:38:53PM -0500, Felipe Contreras wrote:\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\nI did a double-take on reading this subject line and first paragraph,\nthinking \"surely fast-export needs to actually output blobs?\".\n\nReading the patch, I see that this is only about not bothering to load\nblob marks from --import-marks. It might be nice to mention that in the\ncommit message, which is otherwise quite confusing.\n\nI'm also not sure why your claim \"we don't care about blobs\" is true,\nbecause naively we would want future runs of fast-export to avoid having\nto write out the whole blob content when mentioning the blob again. I\nthink one argument could be \"if we write a mark for blob X, we will also\nhave written a mark for commit Y which contains it; on subsequent runs,\nwe will just show the mark for Y in the first place, and not even care\nabout showing X (as a part of Y) either way. We would only refer to the\nmark for X if it appears as part of a different commit, but that is a\nrare case not worth worrying about.\"\n\nDoes that match your reasoning?\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\nPresumably most of that speed improvement comes from not parsing the\nblob objects. I wonder if you could get similar speedups by applying the\n\"do not bother parsing\" rule from your patch 3. You would still incur\nsome cost to create a \"struct blob\", but it may or may not be\nmeasurable. That would mean we get the \"case not worth worrying about\"\nfrom above for free. I doubt it would make that big a difference,\nthough, given the rarity of it. So I am OK with it either way.\n\n-Peff\n"},{"id":"216503","messageId":"7v7gjctabm.fsf@alter.siamese.dyndns.org","threadId":"33730","inReplyTo":"20130506123111.GB3809@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-06T15:08:45Z","receivedAt":"2013-05-06T15:08:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, May 05, 2013 at 05:38:53PM -0500, Felipe Contreras wrote:\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> I did a double-take on reading this subject line and first paragraph,\n> thinking \"surely fast-export needs to actually output blobs?\".\n>\n> Reading the patch, I see that this is only about not bothering to load\n> blob marks from --import-marks. It might be nice to mention that in the\n> commit message, which is otherwise quite confusing.\n\nI had the same reaction first, but not writing the blob _objects_\nout to the output stream would not make any sense, so it was fairly\neasy to guess what the author wanted to say ;-).\n\n> I'm also not sure why your claim \"we don't care about blobs\" is true,\n> because naively we would want future runs of fast-export to avoid having\n> to write out the whole blob content when mentioning the blob again.\n\nThe existing documentation is fairly clear that marks for objects\nother than commits are not exported, and the import-marks codepath\ndiscards anything but commits, so there is no mechanism for the\nexisting fast-export users to leave blob marks in the marks file for\nlater runs of fast-export to take advantage of. The second\ninvocation cannot refer to such a blob in the first place.\n\nThe story is different on the fast-import side, where we do say we\ndump the full table and a later run can depend on these marks.\n\nBy discarding marks on blobs, we may be robbing some optimization\npossibilities, and by discarding marks on tags, we may be robbing\nsome features, from users of fast-export; we might want to add an\noption \"--use-object-marks={blob,commit,tag}\" or something to both\nfast-export and fast-import, so that the former can optionally write\nmarks for non-commits out, and the latter can omit non commit marks\nif the user do not need them. But that is a separate issue.\n"},{"id":"216512","messageId":"7vppx4rskp.fsf@alter.siamese.dyndns.org","threadId":"33730","inReplyTo":"7v7gjctabm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-06T16:17:26Z","receivedAt":"2013-05-06T16:17:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> By discarding marks on blobs, we may be robbing some optimization\n> possibilities, and by discarding marks on tags, we may be robbing\n> some features, from users of fast-export; we might want to add an\n> option \"--use-object-marks={blob,commit,tag}\" or something to both\n> fast-export and fast-import, so that the former can optionally write\n> marks for non-commits out, and the latter can omit non commit marks\n> if the user do not need them. But that is a separate issue.\n\ns/--use-object-marks/--persistent-object-marks/; I think that would\nexpress the issue better.\n"},{"id":"216513","messageId":"20130506162008.GB7992@sigill.intra.peff.net","threadId":"33730","inReplyTo":"7v7gjctabm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-06T16:20:08Z","receivedAt":"2013-05-06T16:20:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 06, 2013 at 08:08:45AM -0700, Junio C Hamano wrote:\n\n> > I'm also not sure why your claim \"we don't care about blobs\" is true,\n> > because naively we would want future runs of fast-export to avoid having\n> > to write out the whole blob content when mentioning the blob again.\n> \n> The existing documentation is fairly clear that marks for objects\n> other than commits are not exported, and the import-marks codepath\n> discards anything but commits, so there is no mechanism for the\n> existing fast-export users to leave blob marks in the marks file for\n> later runs of fast-export to take advantage of. The second\n> invocation cannot refer to such a blob in the first place.\n\nOK. If the argument is \"we do not write them, so do not bother reading\nthem back in\", I think that is reasonable. It could hurt anybody trying\nto run \"fast-export\" against a marks file created by somebody else, but\nthat is also the same case that is being helped here (since otherwise,\nwe would not be seeing blob entries at all).\n\nI do not offhand know enough about the internals of import/export-style\nremote-helpers to say whether the \"hurt\" case even exists, let alone how\ncommon it is.\n\n> By discarding marks on blobs, we may be robbing some optimization\n> possibilities, and by discarding marks on tags, we may be robbing\n> some features, from users of fast-export; we might want to add an\n> option \"--use-object-marks={blob,commit,tag}\" or something to both\n> fast-export and fast-import, so that the former can optionally write\n> marks for non-commits out, and the latter can omit non commit marks\n> if the user do not need them. But that is a separate issue.\n\nYeah, that would allow the old behavior (and more) if anybody is hurt by\nthis. It is nice if the order of implementation is \"more features, then\nflip the default\" because it provides an immediate escape hatch for\nanybody who is hurt by the change in default. But again, I do not know\nenough to say whether such hurt cases even exist.\n\n-Peff\n"},{"id":"216516","messageId":"7vli7srrva.fsf@alter.siamese.dyndns.org","threadId":"33730","inReplyTo":"20130506162008.GB7992@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-06T16:32:41Z","receivedAt":"2013-05-06T16:32:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, May 06, 2013 at 08:08:45AM -0700, Junio C Hamano wrote:\n>\n>> > I'm also not sure why your claim \"we don't care about blobs\" is true,\n>> > because naively we would want future runs of fast-export to avoid having\n>> > to write out the whole blob content when mentioning the blob again.\n>> \n>> The existing documentation is fairly clear that marks for objects\n>> other than commits are not exported, and the import-marks codepath\n>> discards anything but commits, so there is no mechanism for the\n>> existing fast-export users to leave blob marks in the marks file for\n>> later runs of fast-export to take advantage of. The second\n>> invocation cannot refer to such a blob in the first place.\n>\n> OK. If the argument is \"we do not write them, so do not bother reading\n> them back in\", I think that is reasonable.\n\nThe way I read builtin/fast-export.c::import_marks() is that it is\nmore like \"we do not write them, and we do not read them back in\neither IN THE CURRENT CODE\".\n"},{"id":"216517","messageId":"20130506164046.GA20257@sigill.intra.peff.net","threadId":"33730","inReplyTo":"7vli7srrva.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-06T16:40:46Z","receivedAt":"2013-05-06T16:40:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 06, 2013 at 09:32:41AM -0700, Junio C Hamano wrote:\n\n> > OK. If the argument is \"we do not write them, so do not bother reading\n> > them back in\", I think that is reasonable.\n> \n> The way I read builtin/fast-export.c::import_marks() is that it is\n> more like \"we do not write them, and we do not read them back in\n> either IN THE CURRENT CODE\".\n\nAhh...I see now. It is not about skipping the blobs as a new behavior,\nbut rather about skipping them _earlier_, before we have loaded the\nobject contents from disk.\n\nI took the \"we don't care about\" as \"the general use of fast-export does\nnot care about\", but it is \"we will literally just drop them a few lines\nlater\".\n\nSo yes, I think this is an obviously correct optimization. Thanks for\nclarifying, and sorry to be so slow.\n\n-Peff\n"},{"id":"216521","messageId":"7v8v3srpsa.fsf@alter.siamese.dyndns.org","threadId":"33730","inReplyTo":"20130506164046.GA20257@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-06T17:17:41Z","receivedAt":"2013-05-06T17:17:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So yes, I think this is an obviously correct optimization. Thanks for\n> clarifying, and sorry to be so slow.\n\nNo need to be sorry. It just shows that the log message could have\nbeen more helpful.\n\nHere is what I tentatively queued.\n\ncommit 83582e91d22c66413b291d4d6d45bbeafddc2af9\nAuthor: Felipe Contreras \nDate: Sun May 5 17:38:53 2013 -0500\n\n fast-export: do not parse non-commit objects while reading marks file\n \n We read from the marks file and keep only marked commits, but in\n order to find the type of object, we are parsing the whole thing,\n which 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 \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 Signed-off-by: Junio C Hamano \n"},{"id":"216522","messageId":"20130506171935.GA21892@sigill.intra.peff.net","threadId":"33730","inReplyTo":"7v8v3srpsa.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-06T17:19:35Z","receivedAt":"2013-05-06T17:19:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 06, 2013 at 10:17:41AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > So yes, I think this is an obviously correct optimization. Thanks for\n> > clarifying, and sorry to be so slow.\n> \n> No need to be sorry. It just shows that the log message could have\n> been more helpful.\n> \n> Here is what I tentatively queued.\n> [...]\n\nYeah, that is much for to understand (to me, at least).\n\nThanks.\n\n-Peff\n"},{"id":"216524","messageId":"20130506174158.GA23213@sigill.intra.peff.net","threadId":"33730","inReplyTo":"20130506171935.GA21892@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-06T17:41:58Z","receivedAt":"2013-05-06T17:41:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 06, 2013 at 01:19:35PM -0400, Jeff King wrote:\n\n> > Here is what I tentatively queued.\n> > [...]\n> \n> Yeah, that is much for to understand (to me, at least).\n\nUgh. That was supposed to be \"much easier to understand\". Perhaps I will\nlearn to type one day.\n\n-Peff\n"},{"id":"216529","messageId":"CAMP44s2rvu2PaazHEx-OXaZbuBAX7sZb04Oh38ifyAEG6kcxkw@mail.gmail.com","threadId":"33730","inReplyTo":"20130506123111.GB3809@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-06T19:02:13Z","receivedAt":"2013-05-06T19:02: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 7:31 AM, Jeff King wrote:\n> On Sun, May 05, 2013 at 05:38:53PM -0500, Felipe Contreras wrote:\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> I did a double-take on reading this subject line and first paragraph,\n> thinking \"surely fast-export needs to actually output blobs?\".\n\nIf you think that, then you are not familiar with the code.\n\n--export-marks=::\n\tDumps the internal marks table to when complete.\n\tMarks are written one per line as `:markid SHA-1`. Only marks\n\tfor revisions are dumped; marks for blobs are ignored.\n\n\t\tif (deco->base && deco->base->type == 1) {\n\t\t\tmark = ptr_to_mark(deco->decoration);\n\t\t\tif (fprintf(f, \":%\"PRIu32\" %s\\n\", mark,\n\t\t\t\tsha1_to_hex(deco->base->sha1)) < 0) {\n\t\t\t e = 1;\n\t\t\t break;\n\t\t\t}\n\t\t}\n\n> Reading the patch, I see that this is only about not bothering to load\n> blob marks from --import-marks. It might be nice to mention that in the\n> commit message, which is otherwise quite confusing.\n\nThe commit message says it exactly like it is: we don't care about blobs.\n\nIf an object is not a commit, we *already* skip it. But as the commit\nmessage already says, we do so by parsing the whole thing.\n\n> I'm also not sure why your claim \"we don't care about blobs\" is true,\n> because naively we would want future runs of fast-export to avoid having\n> to write out the whole blob content when mentioning the blob again.\n\nBecause it's pointless to have hundreds and thousands of blob marks\nthat are *never* going to be used, only for an extremely tiny minority\nthat would.\n\n> Does that match your reasoning?\n\nIt doesn't matter, it has been that way since --export-marks was introduced.\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>\n> Presumably most of that speed improvement comes from not parsing the\n> blob objects. I wonder if you could get similar speedups by applying the\n> \"do not bother parsing\" rule from your patch 3. You would still incur\n> some cost to create a \"struct blob\", but it may or may not be\n> measurable. That would mean we get the \"case not worth worrying about\"\n> from above for free. I doubt it would make that big a difference,\n> though, given the rarity of it. So I am OK with it either way.\n\nHow would I know if it's a blob or a commit, if not by the code this\npatch introduces?\n\n-- \nFelipe Contreras\n"},{"id":"216530","messageId":"CAMP44s2rdkND40QDQA9T7MNGoKPtnr50nV98aExUe4bCOXZGyA@mail.gmail.com","threadId":"33730","inReplyTo":"7v7gjctabm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-06T19:09:56Z","receivedAt":"2013-05-06T19:09: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 10:08 AM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> On Sun, May 05, 2013 at 05:38:53PM -0500, Felipe Contreras wrote:\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>> I did a double-take on reading this subject line and first paragraph,\n>> thinking \"surely fast-export needs to actually output blobs?\".\n>>\n>> Reading the patch, I see that this is only about not bothering to load\n>> blob marks from --import-marks. It might be nice to mention that in the\n>> commit message, which is otherwise quite confusing.\n>\n> I had the same reaction first, but not writing the blob _objects_\n> out to the output stream would not make any sense, so it was fairly\n> easy to guess what the author wanted to say ;-).\n\nThat's how fast-export has worked since --export-marks was introduced.\n\n>> I'm also not sure why your claim \"we don't care about blobs\" is true,\n>> because naively we would want future runs of fast-export to avoid having\n>> to write out the whole blob content when mentioning the blob again.\n>\n> The existing documentation is fairly clear that marks for objects\n> other than commits are not exported, and the import-marks codepath\n> discards anything but commits, so there is no mechanism for the\n> existing fast-export users to leave blob marks in the marks file for\n> later runs of fast-export to take advantage of. The second\n> invocation cannot refer to such a blob in the first place.\n>\n> The story is different on the fast-import side, where we do say we\n> dump the full table and a later run can depend on these marks.\n\nYes, and gaining nothing but increased disk-space.\n\n> By discarding marks on blobs, we may be robbing some optimization\n> possibilities, and by discarding marks on tags, we may be robbing\n> some features, from users of fast-export; we might want to add an\n> option \"--use-object-marks={blob,commit,tag}\" or something to both\n> fast-export and fast-import, so that the former can optionally write\n> marks for non-commits out, and the latter can omit non commit marks\n> if the user do not need them. But that is a separate issue.\n\nHow? The only way we might rob optimizations is if there's an obscene\namount files, otherwise the number of blob marks that we are\n*actually* going to use ever again is extremely tiny.\n\n-- \nFelipe Contreras\n"},{"id":"216531","messageId":"20130506191146.GA3056@sigill.intra.peff.net","threadId":"33730","inReplyTo":"CAMP44s2rvu2PaazHEx-OXaZbuBAX7sZb04Oh38ifyAEG6kcxkw@mail.gmail.com","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-06T19:11:46Z","receivedAt":"2013-05-06T19:11:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 06, 2013 at 02:02:13PM -0500, Felipe Contreras wrote:\n\n> > I did a double-take on reading this subject line and first paragraph,\n> > thinking \"surely fast-export needs to actually output blobs?\".\n> \n> If you think that, then you are not familiar with the code.\n> \n> --export-marks=::\n> [...]\n\nMy point was that nothing in the subject line nor that first paragraph\n(nor, for that matter, the entire commit message) says that we are\ntalking about marks here.\n\n> > Reading the patch, I see that this is only about not bothering to load\n> > blob marks from --import-marks. It might be nice to mention that in the\n> > commit message, which is otherwise quite confusing.\n> \n> The commit message says it exactly like it is: we don't care about blobs.\n\nIf you guess that \"we\" means the marks code and not all of fast-export,\nthen yes. But I do not have any desire to get into another debate trying\nto convince you that there is value to having a clear commit message.\nJunio has already proposed a much more readable one.\n\n-Peff\n"},{"id":"216532","messageId":"CAMP44s16pX-n9AAMu8zdSPPBhJxYQAisgDd2-gD2eiS=g-uuFg@mail.gmail.com","threadId":"33730","inReplyTo":"20130506162008.GB7992@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-06T19:12:57Z","receivedAt":"2013-05-06T19:12: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 11:20 AM, Jeff King wrote:\n> On Mon, May 06, 2013 at 08:08:45AM -0700, Junio C Hamano wrote:\n>\n>> > I'm also not sure why your claim \"we don't care about blobs\" is true,\n>> > because naively we would want future runs of fast-export to avoid having\n>> > to write out the whole blob content when mentioning the blob again.\n>>\n>> The existing documentation is fairly clear that marks for objects\n>> other than commits are not exported, and the import-marks codepath\n>> discards anything but commits, so there is no mechanism for the\n>> existing fast-export users to leave blob marks in the marks file for\n>> later runs of fast-export to take advantage of. The second\n>> invocation cannot refer to such a blob in the first place.\n>\n> OK. If the argument is \"we do not write them, so do not bother reading\n> them back in\", I think that is reasonable.\n\nWe already do that:\n\n5d3698f fast-export: avoid importing blob marks\n\n> It could hurt anybody trying\n> to run \"fast-export\" against a marks file created by somebody else, but\n> that is also the same case that is being helped here (since otherwise,\n> we would not be seeing blob entries at all).\n>\n> I do not offhand know enough about the internals of import/export-style\n> remote-helpers to say whether the \"hurt\" case even exists, let alone how\n> common it is.\n>\n>> By discarding marks on blobs, we may be robbing some optimization\n>> possibilities, and by discarding marks on tags, we may be robbing\n>> some features, from users of fast-export; we might want to add an\n>> option \"--use-object-marks={blob,commit,tag}\" or something to both\n>> fast-export and fast-import, so that the former can optionally write\n>> marks for non-commits out, and the latter can omit non commit marks\n>> if the user do not need them. But that is a separate issue.\n>\n> Yeah, that would allow the old behavior (and more) if anybody is hurt by\n> this.\n\nThere is no behavior change in this patch. We do *exactly* the same as before.\n\n-- \nFelipe Contreras\n"},{"id":"216533","messageId":"CAMP44s3N53fHswhayqGehTJAP0huJqRup73jxYuiv7sVYUnN_g@mail.gmail.com","threadId":"33730","inReplyTo":"20130506191146.GA3056@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-06T19:15:52Z","receivedAt":"2013-05-06T19:15:52Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, May 6, 2013 at 2:11 PM, Jeff King wrote:\n> On Mon, May 06, 2013 at 02:02:13PM -0500, Felipe Contreras wrote:\n>\n>> > I did a double-take on reading this subject line and first paragraph,\n>> > thinking \"surely fast-export needs to actually output blobs?\".\n>>\n>> If you think that, then you are not familiar with the code.\n>>\n>> --export-marks=::\n>> [...]\n>\n> My point was that nothing in the subject line nor that first paragraph\n> (nor, for that matter, the entire commit message) says that we are\n> talking about marks here.\n\ns/$/ while loading marks/. Fixed.\n\n-- \nFelipe Contreras\n"},{"id":"216550","messageId":"7vppx3q0z6.fsf@alter.siamese.dyndns.org","threadId":"33730","inReplyTo":"CAMP44s2rdkND40QDQA9T7MNGoKPtnr50nV98aExUe4bCOXZGyA@mail.gmail.com","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-06T20:58:53Z","receivedAt":"2013-05-06T20:58:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> The story is different on the fast-import side, where we do say we\n>> dump the full table and a later run can depend on these marks.\n>\n> Yes, and gaining nothing but increased disk-space.\n\nI thought that the \"gaining nothing\" has already been refuted by the\ndiscussion several hours ago...\n\ncf. http://thread.gmane.org/gmane.comp.version-control.git/223275/focus=223440\n\nPuzzled...\n\n>> By discarding marks on blobs, we may be robbing some optimization\n>> possibilities, and by discarding marks on tags, we may be robbing\n>> some features, from users of fast-export; we might want to add an\n>> option \"--use-object-marks={blob,commit,tag}\" or something to both\n>> fast-export and fast-import, so that the former can optionally write\n>> marks for non-commits out, and the latter can omit non commit marks\n>> if the user do not need them. But that is a separate issue.\n>\n> How?\n\n * if we teach fast-import to optionally not write marks for blobs\n and trees out, your remote-bzr can take advantage of it, because\n it does not reuse marks for non-commits in later runs, right?\n Existing users like cvs2git that do not ask to skip marks for\n non-commits will not be hurt and keep referring to blobs an\n earlier run wrote out.\n\n * if we teach fast-export to optionally write marks for blobs and\n trees out, the users of fast-export could reuse marks for blobs\n and trees in later runs (perhaps they can drive fast-export from\n the output of \"git log --raw\", noticing blob object names they\n already saw). Existing users that do not ask for such a feature\n will not be hurt.\n"},{"id":"216555","messageId":"CAMP44s2wh71vW6GaCfyCO398BWvVbYnwSE76bO=46XVJF3jWNg@mail.gmail.com","threadId":"33730","inReplyTo":"7vppx3q0z6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-06T21:30:29Z","receivedAt":"2013-05-06T21:30:29Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, May 6, 2013 at 3:58 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>>> The story is different on the fast-import side, where we do say we\n>>> dump the full table and a later run can depend on these marks.\n>>\n>> Yes, and gaining nothing but increased disk-space.\n>\n> I thought that the \"gaining nothing\" has already been refuted by the\n> discussion several hours ago...\n>\n> cf. http://thread.gmane.org/gmane.comp.version-control.git/223275/focus=223440\n>\n> Puzzled...\n\nWhat is being gained there? Nothing.\n\n>>> By discarding marks on blobs, we may be robbing some optimization\n>>> possibilities, and by discarding marks on tags, we may be robbing\n>>> some features, from users of fast-export; we might want to add an\n>>> option \"--use-object-marks={blob,commit,tag}\" or something to both\n>>> fast-export and fast-import, so that the former can optionally write\n>>> marks for non-commits out, and the latter can omit non commit marks\n>>> if the user do not need them. But that is a separate issue.\n>>\n>> How?\n>\n> * if we teach fast-import to optionally not write marks for blobs\n> and trees out, your remote-bzr can take advantage of it,\n\nI already said remote-bzr is irrelevant. *Everybody* benefits.\n\n> Existing users like cvs2git that do not ask to skip marks for\n> non-commits will not be hurt and keep referring to blobs an\n> earlier run wrote out.\n\ncvs2git does *not* store marks. It doesn't get any benefit or get hurt *at all*.\n\n> * if we teach fast-export to optionally write marks for blobs and\n> trees out, the users of fast-export could reuse marks for blobs\n> and trees in later runs (perhaps they can drive fast-export from\n> the output of \"git log --raw\", noticing blob object names they\n> already saw). Existing users that do not ask for such a feature\n> will not be hurt.\n\nThere's absolutely no benefit from fast-export being able to load and\nstore blobs, the only effect is that the mark files will have tons of\nentries nobody will ever use.\n\nIf you want to add options for features that only hurt, go ahead, but\nthe only sane default is to only store commit marks, both for\nfast-export, and fast-import. Period.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"216567","messageId":"7vmws7o8hx.fsf@alter.siamese.dyndns.org","threadId":"33730","inReplyTo":"CAMP44s2wh71vW6GaCfyCO398BWvVbYnwSE76bO=46XVJF3jWNg@mail.gmail.com","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-07T01:59:22Z","receivedAt":"2013-05-07T01:59:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>>> How?\n>>\n>> * if we teach fast-import to optionally not write marks for blobs\n>> and trees out, your remote-bzr can take advantage of it,\n>\n> I already said remote-bzr is irrelevant. *Everybody* benefits.\n\nEverybody who does _not_ need to look at marks for non-commits from\nprevious run does. What about the others who do?\n\nSurely, some lucky ones may get the benefit of a new optimization\nfor free if you turn it on uncondtionally without an escape hatch,\nbut that goes against our goal of not to knowingly introduce any\nregression. Michael's cvs2git might have a way to work the breakage\naround (I will let him comment on your suggested workaround), but as\nlong as he has been coding it using the documented feature, why\nshould he even change anything for no gain at all in the first\nplace? Even if you have a workaround, that does not change the fact\nthat a removal of a feature that somebody has been depending on is a\nregression.\n\nWhat's so difficult to understand that by default the responsibility\nof making sure an optimization applies safely to a program that uses\na new optmization lies on that program, in other words, a new\nfeature is by default an opt-in?\n"},{"id":"216581","messageId":"CAMP44s0VBiWN5fZachCDkg9EHZKHRU5idNBdktV2a5wBMJvxjQ@mail.gmail.com","threadId":"33730","inReplyTo":"7vmws7o8hx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] fast-export: improve speed by skipping blobs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T03:49:28Z","receivedAt":"2013-05-07T03:49:28Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, May 6, 2013 at 8:59 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>>>> How?\n>>>\n>>> * if we teach fast-import to optionally not write marks for blobs\n>>> and trees out, your remote-bzr can take advantage of it,\n>>\n>> I already said remote-bzr is irrelevant. *Everybody* benefits.\n>\n> Everybody who does _not_ need to look at marks for non-commits from\n> previous run does.\n\nIOW; everyone.\n\n> What about the others who do?\n\nLike who?\n\n> Surely, some lucky ones may get the benefit of a new optimization\n> for free if you turn it on uncondtionally without an escape hatch,\n> but that goes against our goal of not to knowingly introduce any\n> regression.\n\nThat's different. One thing is to turn it on unconditionally, and\nanother thing is to turn it on by *default*.\n\n> Michael's cvs2git might have a way to work the breakage\n> around (I will let him comment on your suggested workaround), but as\n> long as he has been coding it using the documented feature, why\n> should he even change anything for no gain at all in the first\n> place? Even if you have a workaround, that does not change the fact\n> that a removal of a feature that somebody has been depending on is a\n> regression.\n\nWho is depending on it? Michael didn't say that he used that feature,\nmerely that it was documented in cvs2git, because Windows doesn't have\n'cat'. He claimed that other people *might* be using that \"feature\",\nbut we don't *know*.\n\nIs a couple of commands somebody wrote in some documentation which can\nbe easily fixed, reason enough to punish everyone else?\n\n> What's so difficult to understand that by default the responsibility\n> of making sure an optimization applies safely to a program that uses\n> a new optmization lies on that program, in other words, a new\n> feature is by default an opt-in?\n\nIs that written in some git bible descended from some god that I\nmissed? If not, everything are guidelines, and guidelines are there\nfor a reason, and those reasons can be challenged, and so can the\nguidelines.\n\nSometimes it makes sense to make a new feature opt-in, sometimes it\ndoesn't, there are no absolutes, there should be no dogmas.\n\n-- \nFelipe Contreras\n"},{"id":"216615","messageId":"7v38tyn9cq.fsf@alter.siamese.dyndns.org","threadId":"33730","inReplyTo":"1367793534-8401-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 1/3] fast-{import,export}: use get_sha1_hex() directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-07T14:38:29Z","receivedAt":"2013-05-07T14:38:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> It's wrong to call get_sha1() if they should be SHA-1s, plus\n> inefficient.\n>\n> Signed-off-by: Felipe Contreras \n> ---\n\nIt appears that \"they should be SHA-1s\" assumption does not hold;\nthis patch breaks at least 3303, 9020, and 9300.\n\nAlso assuming these are always 40-hex goes directly against what is\ndocumented in Documentation/git-fast-import.txt (look for \"Here\ncommittish is any of the following\"). My bad while reviewing the\nearlier round.\n\nI've redone 'pu' (which was failing the test last night) after\ndropping this and keeping only patches 2 and 3 from the series.\n"},{"id":"216663","messageId":"CAMP44s0AipiEVhHrDS0-dB9jCoCqHYDd_s5gcGzqyuh0A+qehg@mail.gmail.com","threadId":"33730","inReplyTo":"7v38tyn9cq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/3] fast-{import,export}: use get_sha1_hex() directly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-07T22:13:19Z","receivedAt":"2013-05-07T22:13:19Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, May 7, 2013 at 9:38 AM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> It's wrong to call get_sha1() if they should be SHA-1s, plus\n>> inefficient.\n>>\n>> Signed-off-by: Felipe Contreras \n>> ---\n>\n> It appears that \"they should be SHA-1s\" assumption does not hold;\n> this patch breaks at least 3303, 9020, and 9300.\n>\n> Also assuming these are always 40-hex goes directly against what is\n> documented in Documentation/git-fast-import.txt (look for \"Here\n> committish is any of the following\"). My bad while reviewing the\n> earlier round.\n>\n> I've redone 'pu' (which was failing the test last night) after\n> dropping this and keeping only patches 2 and 3 from the series.\n\nTurns out most of the get_sha1() calls were correct; this does the trick:\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 18fdfb3..d1d68e9 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -623,7 +623,7 @@ static void import_marks(char *input_file)\n\n mark = strtoumax(line + 1, &mark_end, 10);\n if (!mark || mark_end == line + 1\n- || *mark_end != ' ' || get_sha1(mark_end + 1, sha1))\n+ || *mark_end != ' ' || get_sha1_hex(mark_end + 1, sha1))\n die(\"corrupt mark line: %s\", line);\n\n if (last_idnum < mark)\ndiff --git a/fast-import.c b/fast-import.c\nindex 5f539d7..3f32149 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1822,7 +1822,7 @@ static void read_marks(void)\n *end = 0;\n mark = strtoumax(line + 1, &end, 10);\n if (!mark || end == line + 1\n- || *end != ' ' || get_sha1(end + 1, sha1))\n+ || *end != ' ' || get_sha1_hex(end + 1, sha1))\n die(\"corrupt mark line: %s\", line);\n e = find_object(sha1);\n if (!e) {\n\n-- \nFelipe Contreras\n"},{"id":"216668","messageId":"7vli7qidjm.fsf@alter.siamese.dyndns.org","threadId":"33730","inReplyTo":"CAMP44s0AipiEVhHrDS0-dB9jCoCqHYDd_s5gcGzqyuh0A+qehg@mail.gmail.com","subject":"Re: [PATCH v2 1/3] fast-{import,export}: use get_sha1_hex() directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-07T23:19:09Z","receivedAt":"2013-05-07T23:19:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> Turns out most of the get_sha1() calls were correct; this does the trick:\n>\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index 18fdfb3..d1d68e9 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -623,7 +623,7 @@ static void import_marks(char *input_file)\n>\n> mark = strtoumax(line + 1, &mark_end, 10);\n> if (!mark || mark_end == line + 1\n> - || *mark_end != ' ' || get_sha1(mark_end + 1, sha1))\n> + || *mark_end != ' ' || get_sha1_hex(mark_end + 1, sha1))\n> die(\"corrupt mark line: %s\", line);\n>\n> if (last_idnum < mark)\n> diff --git a/fast-import.c b/fast-import.c\n> index 5f539d7..3f32149 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -1822,7 +1822,7 @@ static void read_marks(void)\n> *end = 0;\n> mark = strtoumax(line + 1, &end, 10);\n> if (!mark || end == line + 1\n> - || *end != ' ' || get_sha1(end + 1, sha1))\n> + || *end != ' ' || get_sha1_hex(end + 1, sha1))\n\nThis is where --import-marks is handled, and we should be seeing\n\n\t:markid SHA-1\n\none per each line (according to Documentation/git-fast-import.txt).\nSo this one should be get_sha1_hex().\n\nThe other one in fast-export.c would be the same.\n\nThe other ones in the original patch were reading from the\nfast-import stream and shouldn't have insisted on 40-hex.\n\nWill replace the body of the change with only these two hunks and\nrequeue. Thanks.\n"}]}