{"thread":{"id":"10694","subject":"[PATCH] git-fetch: avoid local fetching from alternate (again)","startedAt":"2007-11-07T02:41:18Z","lastAt":"2007-11-08T08:04:46Z","messageCount":7,"participants":["Shawn O. Pearce","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"58631","messageId":"20071107024118.GA11043@spearce.org","threadId":"10694","inReplyTo":null,"subject":"[PATCH] git-fetch: avoid local fetching from alternate (again)","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-11-07T02:41:18Z","receivedAt":"2007-11-07T02:41:18Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Back in e3c6f240fd9c5bdeb33f2d47adc859f37935e2df Junio taught\ngit-fetch to avoid copying objects when we are fetching from\na repository that is already registered as an alternate object\ndatabase. In such a case there is no reason to copy any objects\nas we can already obtain them through the alternate.\n\nHowever we need to ensure the objects are all reachable, so we\nrun `git rev-list --objects $theirs --not --all` to verify this.\nIf any object is missing or unreadable then we need to instead copy\nthe objects from the remote. When a missing object is detected\nthe git-rev-list process will exit with a non-zero exit status,\nmaking this condition quite easy to detect.\n\nAlthough git-fetch is currently a builtin (and so is rev-list) we\nreally cannot invoke the traverse_objects() API at this point in the\ntransport code. The object walker within traverse_objects() calls\ndie() as soon as it finds an object it cannot read. If that happens\nwe want to resume the fetch process by calling do_fetch_pack(),\ninstead of terminating. To get aroaund this we spawn git-rev-list\ninto a background process to prevent a die() from killing the\nforeground fetch process.\n\nWe aren't interested in the output of rev-list (a list of SHA-1\nobject names that are reachable) or its errors (a \"spurious\" error\nabout an object not being found as we need to copy it) so we redirect\nboth stdout and stderr to /dev/null.\n\nSigned-off-by: Shawn O. Pearce \n---\n run-command.c | 6 ++++--\n run-command.h | 1 +\n transport.c | 48 +++++++++++++++++++++++++++++++++++++++++++++---\n 3 files changed, 50 insertions(+), 5 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex d99a6c4..476d00c 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -41,7 +41,7 @@ int start_command(struct child_process *cmd)\n \t\tcmd->close_out = 1;\n \t}\n \n-\tneed_err = cmd->err < 0;\n+\tneed_err = !cmd->no_stderr && cmd->err < 0;\n \tif (need_err) {\n \t\tif (pipe(fderr) < 0) {\n \t\t\tif (need_in)\n@@ -87,7 +87,9 @@ int start_command(struct child_process *cmd)\n \t\t\tclose(cmd->out);\n \t\t}\n \n-\t\tif (need_err) {\n+\t\tif (cmd->no_stderr)\n+\t\t\tdup_devnull(2);\n+\t\telse if (need_err) {\n \t\t\tdup2(fderr[1], 2);\n \t\t\tclose_pair(fderr);\n \t\t}\ndiff --git a/run-command.h b/run-command.h\nindex 94e1e9d..1fc781d 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -23,6 +23,7 @@ struct child_process {\n \tunsigned close_out:1;\n \tunsigned no_stdin:1;\n \tunsigned no_stdout:1;\n+\tunsigned no_stderr:1;\n \tunsigned git_cmd:1; /* if this is to be git sub-command */\n \tunsigned stdout_to_stderr:1;\n };\ndiff --git a/transport.c b/transport.c\nindex f4577b7..8505a84 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -615,17 +615,56 @@ static struct ref *get_refs_via_connect(struct transport *transport)\n \treturn refs;\n }\n \n+static int fetch_local_nocopy(struct transport *transport,\n+\t\t\t int nr_heads, struct ref **to_fetch)\n+{\n+\tstruct stat sb;\n+\tstruct child_process revlist;\n+\tchar **argv;\n+\tint i, j, err;\n+\n+\tif (stat(transport->url, &sb) || !S_ISDIR(sb.st_mode))\n+\t\treturn -1;\n+\n+\ti = 0;\n+\targv = xmalloc(sizeof(*argv) * (nr_heads + 5));\n+\targv[i++] = xstrdup(\"rev-list\");\n+\targv[i++] = xstrdup(\"--objects\");\n+\tfor (j = 0; j < nr_heads; j++)\n+\t\targv[i++] = xstrdup(sha1_to_hex(to_fetch[j]->old_sha1));\n+\targv[i++] = xstrdup(\"--not\");\n+\targv[i++] = xstrdup(\"--all\");\n+\targv[i++] = NULL;\n+\n+\tmemset(&revlist, 0, sizeof(revlist));\n+\trevlist.argv = (const char**)argv;\n+\trevlist.git_cmd = 1;\n+\trevlist.no_stdin = 1;\n+\trevlist.no_stdout = 1;\n+\trevlist.no_stderr = 1;\n+\terr = start_command(&revlist);\n+\tif (!err)\n+\t\terr |= finish_command(&revlist);\n+\n+\tfor (i = 0; argv[i]; i++)\n+\t\tfree(argv[i]);\n+\tfree(argv);\n+\treturn err;\n+}\n+\n static int fetch_refs_via_pack(struct transport *transport,\n \t\t\t int nr_heads, struct ref **to_fetch)\n {\n \tstruct git_transport_data *data = transport->data;\n-\tchar **heads = xmalloc(nr_heads * sizeof(*heads));\n-\tchar **origh = xmalloc(nr_heads * sizeof(*origh));\n+\tchar **heads, **origh;\n \tstruct ref *refs;\n-\tchar *dest = xstrdup(transport->url);\n+\tchar *dest;\n \tstruct fetch_pack_args args;\n \tint i;\n \n+\tif (!fetch_local_nocopy(transport, nr_heads, to_fetch))\n+\t\treturn 0;\n+\n \tmemset(&args, 0, sizeof(args));\n \targs.uploadpack = data->uploadpack;\n \targs.keep_pack = data->keep;\n@@ -634,6 +673,9 @@ static int fetch_refs_via_pack(struct transport *transport,\n \targs.verbose = transport->verbose > 0;\n \targs.depth = data->depth;\n \n+\theads = xmalloc(nr_heads * sizeof(*heads));\n+\torigh = xmalloc(nr_heads * sizeof(*origh));\n+\tdest = xstrdup(transport->url);\n \tfor (i = 0; i < nr_heads; i++)\n \t\torigh[i] = heads[i] = xstrdup(to_fetch[i]->name);\n \trefs = fetch_pack(&args, dest, nr_heads, heads, &transport->pack_lockfile);\n-- \n1.5.3.5.1590.gfadfad\n"},{"id":"58637","messageId":"7vsl3iefoj.fsf@gitster.siamese.dyndns.org","threadId":"10694","inReplyTo":"20071107024118.GA11043@spearce.org","subject":"Re: [PATCH] git-fetch: avoid local fetching from alternate (again)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-07T06:24:28Z","receivedAt":"2007-11-07T06:24:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Back in e3c6f240fd9c5bdeb33f2d47adc859f37935e2df Junio taught\n> git-fetch to avoid copying objects when we are fetching from\n> a repository that is already registered as an alternate object\n> database. In such a case there is no reason to copy any objects\n> as we can already obtain them through the alternate.\n\nWell spotted. It would be a good idea to commit the big comment\nfrom contrib/examples/git-fetch.sh to fetch_local_nocopy()\nfunction, which would have made us realize that the patch does\nnot refrain from applying this optimization even when shallow\nis in effect. But I think that is actually a good change.\n\nThe run-command change and the main part of the fix are\nlogically independent.\n\nThe regression the patch fixes should be testable with a\nscript. Please have a new test for it.\n\nThanks.\n"},{"id":"58642","messageId":"7vd4umebxc.fsf@gitster.siamese.dyndns.org","threadId":"10694","inReplyTo":"7vsl3iefoj.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-fetch: avoid local fetching from alternate (again)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-07T07:45:35Z","receivedAt":"2007-11-07T07:45:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Shawn O. Pearce\" writes:\n>\n>> Back in e3c6f240fd9c5bdeb33f2d47adc859f37935e2df Junio taught\n>> git-fetch to avoid copying objects when we are fetching from\n>> a repository that is already registered as an alternate object\n>> database. In such a case there is no reason to copy any objects\n>> as we can already obtain them through the alternate.\n>\n> Well spotted. It would be a good idea to commit the big comment\n> from contrib/examples/git-fetch.sh to fetch_local_nocopy()\n> function, which would have made us realize that the patch does\n> not refrain from applying this optimization even when shallow\n> is in effect. But I think that is actually a good change.\n\nHaving thought about this further by writing that comment myself\n(patch attached), I suspect that the test at the beginning of\nthe function to see if we are talking to another local\nrepository is not necessary. Even if we are _not_ fetching from\nthe remote, we could have all the necessary objects connected,\nalbeit the chance of that is rather slim. But that means the\nrev-list test will error out rather quickly because objects near\nthe tips are more likely to be missing, and if we are talking\nabout a true remote connection, networking cost will most likely\noutweigh the cost to do this checking.\n\n---\n\n transport.c | 14 ++++++++++++++\n 1 files changed, 14 insertions(+), 0 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 0604dc6..a887491 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -614,6 +614,20 @@ static struct ref *get_refs_via_connect(const struct transport *transport)\n \treturn refs;\n }\n \n+/*\n+ * We would want to bypass the object transfer altogether if\n+ * everything we are going to fetch already exists and connected\n+ * locally.\n+ *\n+ * The refs we are going to fetch are in to_fetch (nr_heads in\n+ * total). If running\n+ *\n+ * $ git-rev-list --objects to_fetch[0] to_fetch[1] ... --not --all\n+ *\n+ * does not error out, that means everything reachable from the\n+ * refs we are going to fetch exists and is connected to some of\n+ * our existing refs.\n+ */\n static int fetch_local_nocopy(struct transport *transport,\n \t\t\t int nr_heads, struct ref **to_fetch)\n {\n"},{"id":"58649","messageId":"47317380.4030102@viscovery.net","threadId":"10694","inReplyTo":"20071107024118.GA11043@spearce.org","subject":"Re: [PATCH] git-fetch: avoid local fetching from alternate (again)","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2007-11-07T08:12:48Z","receivedAt":"2007-11-07T08:12:48Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Shawn O. Pearce schrieb:\n> +\terr = start_command(&revlist);\n> +\tif (!err)\n> +\t\terr |= finish_command(&revlist);\n\nThere's a short-hand:\n\n\terr = run_command(&revlist);\n\n-- Hannes\n"},{"id":"58715","messageId":"7vr6j1bxuf.fsf@gitster.siamese.dyndns.org","threadId":"10694","inReplyTo":"7vsl3iefoj.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-fetch: avoid local fetching from alternate (again)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-07T20:32:40Z","receivedAt":"2007-11-07T20:32:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Well spotted. It would be a good idea to commit the big comment\n> from contrib/examples/git-fetch.sh to fetch_local_nocopy()\n> function, which would have made us realize that the patch does\n> not refrain from applying this optimization even when shallow\n> is in effect. But I think that is actually a good change.\n\nI take this back. This regresses badly.\n\nWhy?\n\nBecause the optimization is useless when we are trying to deepen\nthe shallow history. When you are trying to deepen a shallow\nhistory and the tips of remotes haven't moved since you fetched\nfrom there the last time, you have everything near the tip, and\nbecuse your history is shallow, your ancestry chain is\ncauterized to make it appear that the history is complete. The\nrev-list reachability test would not fail as we expect.\n\nThe breakage can be seen with t5500.\n"},{"id":"58809","messageId":"20071108073505.GI14735@spearce.org","threadId":"10694","inReplyTo":"7vr6j1bxuf.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-fetch: avoid local fetching from alternate (again)","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-11-08T07:35:05Z","receivedAt":"2007-11-08T07:35:05Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Well spotted. It would be a good idea to commit the big comment\n> > from contrib/examples/git-fetch.sh to fetch_local_nocopy()\n> > function, which would have made us realize that the patch does\n> > not refrain from applying this optimization even when shallow\n> > is in effect. But I think that is actually a good change.\n> \n> I take this back. This regresses badly.\n> \n> Why?\n\nWhoops. Good catch. This is why you had a check for the shallow\nhistory file in git-fetch.sh before you took this optimization path.\nMy fault for not including it in this patch.\n \n> Because the optimization is useless when we are trying to deepen\n> the shallow history. When you are trying to deepen a shallow\n> history and the tips of remotes haven't moved since you fetched\n> from there the last time, you have everything near the tip, and\n> becuse your history is shallow, your ancestry chain is\n> cauterized to make it appear that the history is complete. The\n> rev-list reachability test would not fail as we expect.\n\nWhat about just inserting a check to see if --depth was supplied\nto git-fetch on the command line? If so then we are deepening the\nhistory and we just bypass the rev-list \"fast path\" test.\n\nI will be posting an updated patch (series now) shortly.\n\n-- \nShawn.\n"},{"id":"58817","messageId":"20071108080446.GJ14735@spearce.org","threadId":"10694","inReplyTo":"7vsl3iefoj.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-fetch: avoid local fetching from alternate (again)","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-11-08T08:04:46Z","receivedAt":"2007-11-08T08:04:46Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> \n> > Back in e3c6f240fd9c5bdeb33f2d47adc859f37935e2df Junio taught\n> > git-fetch to avoid copying objects when we are fetching from\n> > a repository that is already registered as an alternate object\n> > database. In such a case there is no reason to copy any objects\n> > as we can already obtain them through the alternate.\n> \n> The regression the patch fixes should be testable with a\n> script. Please have a new test for it.\n\nHmmph. t5502-quickfetch should have covered this. It obviously\nwasn't testing the right thing here. I'll figure out why and post\na patch to fix t5502.\n\n-- \nShawn.\n"}]}