{"thread":{"id":"11993","subject":"[PATCH] Fix \"git clone\" for git:// protocol","startedAt":"2008-02-09T16:58:07Z","lastAt":"2008-02-10T13:45:08Z","messageCount":8,"participants":["Johannes Schindelin","Daniel Barkalow","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"68100","messageId":"alpine.LSU.1.00.0802091657000.11591@racer.site","threadId":"11993","inReplyTo":null,"subject":"[PATCH] Fix \"git clone\" for git:// protocol","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-09T16:58:07Z","receivedAt":"2008-02-09T16:58:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nIn the commit \"Reduce the number of connects when fetching\", we checked \nthe return value of git_connect() to see if the connection was successful.\n\nHowever, for the git:// protocol, there is no need to have another \nprocess, so the return value is NULL.\n\nThe thing is: git_connect() does not return at all if it fails, so we need \nnot check the return value of git_connect().\n\nSigned-off-by: Johannes Schindelin \n---\n builtin-fetch-pack.c | 16 ++++++----------\n 1 files changed, 6 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c\nindex f401352..184782f 100644\n--- a/builtin-fetch-pack.c\n+++ b/builtin-fetch-pack.c\n@@ -707,17 +707,13 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \n \tconn = git_connect(fd, (char *)dest, args.uploadpack,\n \t\t\t args.verbose ? CONNECT_VERBOSE : 0);\n-\tif (conn) {\n-\t\tget_remote_heads(fd[0], &ref, 0, NULL, 0);\n-\n-\t\tref = fetch_pack(&args, fd, conn, ref, dest, nr_heads, heads, NULL);\n-\t\tclose(fd[0]);\n-\t\tclose(fd[1]);\n-\t\tif (finish_connect(conn))\n-\t\t\tref = NULL;\n-\t} else {\n+\tget_remote_heads(fd[0], &ref, 0, NULL, 0);\n+\n+\tref = fetch_pack(&args, fd, conn, ref, dest, nr_heads, heads, NULL);\n+\tclose(fd[0]);\n+\tclose(fd[1]);\n+\tif (finish_connect(conn))\n \t\tref = NULL;\n-\t}\n \tret = !ref;\n \n \tif (!ret && nr_heads) {\n-- \n1.5.4.1264.g42770c\n"},{"id":"68102","messageId":"alpine.LNX.1.00.0802091205530.13593@iabervon.org","threadId":"11993","inReplyTo":"alpine.LSU.1.00.0802091657000.11591@racer.site","subject":"Re: [PATCH] Fix \"git clone\" for git:// protocol","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-09T17:16:04Z","receivedAt":"2008-02-09T17:16:04Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 9 Feb 2008, Johannes Schindelin wrote:\n\n> In the commit \"Reduce the number of connects when fetching\", we checked \n> the return value of git_connect() to see if the connection was successful.\n> \n> However, for the git:// protocol, there is no need to have another \n> process, so the return value is NULL.\n> \n> The thing is: git_connect() does not return at all if it fails, so we need \n> not check the return value of git_connect().\n\nHuh. Sure enough. Actually, there's a similar problem in transport.c, \nwhere it assumes that the return value of git_connect is non-zero, which \nmakes it not reuse the connection (not that you can really tell). It might \nbe good to roll in a fix for that. Or maybe git_connect should return a \npointer to a static struct child_process if it doesn't need a subprocess, \njust to distinguish \"we're doing it ourselves\" from \"it's not being done\"? \nIf not, maybe the variables that store the return from git_connect should \nbe renamed to \"subproc\" or something that doesn't suggest they can't be \nNULL if you're actually connected.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"68109","messageId":"alpine.LSU.1.00.0802091805190.11591@racer.site","threadId":"11993","inReplyTo":"alpine.LNX.1.00.0802091205530.13593@iabervon.org","subject":"Re: [PATCH] Fix \"git clone\" for git:// protocol","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-09T18:05:52Z","receivedAt":"2008-02-09T18:05:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 Feb 2008, Daniel Barkalow wrote:\n\n> On Sat, 9 Feb 2008, Johannes Schindelin wrote:\n> \n> > In the commit \"Reduce the number of connects when fetching\", we \n> > checked the return value of git_connect() to see if the connection was \n> > successful.\n> > \n> > However, for the git:// protocol, there is no need to have another \n> > process, so the return value is NULL.\n> > \n> > The thing is: git_connect() does not return at all if it fails, so we \n> > need not check the return value of git_connect().\n> \n> Huh. Sure enough. Actually, there's a similar problem in transport.c, \n> where it assumes that the return value of git_connect is non-zero, which \n> makes it not reuse the connection (not that you can really tell). It might \n> be good to roll in a fix for that. Or maybe git_connect should return a \n> pointer to a static struct child_process if it doesn't need a subprocess, \n> just to distinguish \"we're doing it ourselves\" from \"it's not being done\"? \n\nYeah, I like that. Will do that tomorrow.\n\nCiao,\nDscho\n"},{"id":"68157","messageId":"alpine.LSU.1.00.0802100302050.11591@racer.site","threadId":"11993","inReplyTo":"alpine.LNX.1.00.0802091205530.13593@iabervon.org","subject":"[PATCH] Fix \"git clone\" for git:// protocol","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-10T03:06:57Z","receivedAt":"2008-02-10T03:06:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nIn ba227857(Reduce the number of connects when fetching), we checked\nthe return value of git_connect() to see if the connection was\nsuccessful.\n\nHowever, for the git:// protocol, there is no need to have another\nprocess, so the return value was NULL.\n\nNow, it makes sense to assume the rule that git_connect() will return\nNULL if it fails (at the moment, it die()s if it fails), so return\na dummy child process.\n\nSigned-off-by: Johannes Schindelin \n---\n\t\n\tOn Sat, 9 Feb 2008, Daniel Barkalow wrote:\n\n\t> On Sat, 9 Feb 2008, Johannes Schindelin wrote:\n\t> \n\t> > In the commit \"Reduce the number of connects when fetching\", \n\t> > we checked the return value of git_connect() to see if the \n\t> > connection was successful.\n\t> > \n\t> > However, for the git:// protocol, there is no need to have \n\t> > another process, so the return value is NULL.\n\t> > \n\t> > The thing is: git_connect() does not return at all if it \n\t> > fails, so we need not check the return value of git_connect().\n\t> \n\t> Huh. Sure enough. Actually, there's a similar problem in \n\t> transport.c, where it assumes that the return value of \n\t> git_connect is non-zero, which makes it not reuse the connection \n\t> (not that you can really tell). It might be good to roll in a \n\t> fix for that. Or maybe git_connect should return a pointer to a \n\t> static struct child_process if it doesn't need a subprocess, \n\t> just to distinguish \"we're doing it ourselves\" from \"it's not \n\t> being done\"? If not, maybe the variables that store the return \n\t> from git_connect should be renamed to \"subproc\" or something \n\t> that doesn't suggest they can't be NULL if you're actually \n\t> connected.\n\n\tHow about this?\n\n connect.c | 20 ++++++++++++--------\n 1 files changed, 12 insertions(+), 8 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 3aefd4a..700ceba 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -472,14 +472,18 @@ char *get_port(char *host)\n \treturn NULL;\n }\n \n+static struct child_process no_fork;\n+\n /*\n- * This returns NULL if the transport protocol does not need fork(2), or a\n- * struct child_process object if it does. Once done, finish the connection\n- * with finish_connect() with the value returned from this function\n- * (it is safe to call finish_connect() with NULL to support the former\n- * case).\n+ * This returns a dummy child_process if the transport protocol does not\n+ * need fork(2), or a struct child_process object if it does. Once done,\n+ * finish the connection with finish_connect() with the value returned from\n+ * this function (it is safe to call finish_connect() with NULL to support\n+ * the former case).\n *\n- * If it returns, the connect is successful; it just dies on errors.\n+ * If it returns, the connect is successful; it just dies on errors (this\n+ * will hopefully be changed in a libification effort, to return NULL when\n+ * the connection failed).\n */\n struct child_process *git_connect(int fd[2], const char *url_orig,\n \t\t\t\t const char *prog, int flags)\n@@ -577,7 +581,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t\tfree(url);\n \t\tif (free_path)\n \t\t\tfree(path);\n-\t\treturn NULL;\n+\t\treturn &no_fork;\n \t}\n \n \tconn = xcalloc(1, sizeof(*conn));\n@@ -635,7 +639,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n int finish_connect(struct child_process *conn)\n {\n \tint code;\n-\tif (!conn)\n+\tif (!conn || conn == &no_fork)\n \t\treturn 0;\n \n \tcode = finish_command(conn);\n-- \n1.5.4.1264.g42770c\n"},{"id":"68160","messageId":"alpine.LNX.1.00.0802092208280.13593@iabervon.org","threadId":"11993","inReplyTo":"alpine.LSU.1.00.0802100302050.11591@racer.site","subject":"Re: [PATCH] Fix \"git clone\" for git:// protocol","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-10T03:14:16Z","receivedAt":"2008-02-10T03:14:16Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 10 Feb 2008, Johannes Schindelin wrote:\n\n> \tHow about this?\n\nLooks good to me.\n\nAcked-by: Daniel Barkalow \n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"68166","messageId":"7vhcgh4fb9.fsf@gitster.siamese.dyndns.org","threadId":"11993","inReplyTo":"alpine.LNX.1.00.0802092208280.13593@iabervon.org","subject":"Re: [PATCH] Fix \"git clone\" for git:// protocol","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-10T04:15:54Z","receivedAt":"2008-02-10T04:15:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks. The patch makes sense.\n\nI wonder this deserves a new test case to protect the fix from\nfuture regressions.\n"},{"id":"68198","messageId":"alpine.LSU.1.00.0802101210340.11591@racer.site","threadId":"11993","inReplyTo":"7vhcgh4fb9.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix \"git clone\" for git:// protocol","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-10T12:12:15Z","receivedAt":"2008-02-10T12:12:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 Feb 2008, Junio C Hamano wrote:\n\n> Thanks. The patch makes sense.\n> \n> I wonder this deserves a new test case to protect the fix from future \n> regressions.\n\nIt would probably make sense; this would also exercise git-daemon for the \nfirst time in our test suite.\n\nWill work on it right away.\n\nCiao,\nDscho\n"},{"id":"68212","messageId":"alpine.LSU.1.00.0802101343480.11591@racer.site","threadId":"11993","inReplyTo":"alpine.LSU.1.00.0802101210340.11591@racer.site","subject":"[PATCH] Add a test for git-daemon and clone via git:// protocol","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-10T13:45:08Z","receivedAt":"2008-02-10T13:45:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nThe new test, t5703-daemon.sh, sets up a simple git-daemon at port 8111\n(you can override it with the environment variable GIT_DAEMON_TEST_PORT),\nand then tries to clone via git:// from it, first without the\ngit-daemon-export-ok file (which should fail), and then with it (which\nshould succeed).\n\nSigned-off-by: Johannes Schindelin \n---\n\n\n\tOn Sun, 10 Feb 2008, Johannes Schindelin wrote:\n\n\t> On Sat, 9 Feb 2008, Junio C Hamano wrote:\n\t> \n\t> > Thanks. The patch makes sense.\n\t> > \n\t> > I wonder this deserves a new test case to protect the fix from \n\t> > future regressions.\n\t> \n\t> It would probably make sense; this would also exercise \n\t> git-daemon for the first time in our test suite.\n\n\tHow's this?\n\n t/t5703-daemon.sh | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 55 insertions(+), 0 deletions(-)\n create mode 100755 t/t5703-daemon.sh\n\ndiff --git a/t/t5703-daemon.sh b/t/t5703-daemon.sh\nnew file mode 100755\nindex 0000000..10696a1\n--- /dev/null\n+++ b/t/t5703-daemon.sh\n@@ -0,0 +1,55 @@\n+#!/bin/sh\n+\n+test_description='git daemon and cloning via git:// protocol'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\n+\tmkdir first &&\n+\t(cd first &&\n+\t git init &&\n+\t echo biomimicry > ted &&\n+\t git add ted &&\n+\t test_tick &&\n+\t git commit -m initial &&\n+\t git gc)\n+\n+'\n+\n+PORT=${GIT_DAEMON_TEST_PORT:-8111}\n+\n+start_daemon () {\n+\ttrap 'kill \"$DAEMON_PID\"' 0\n+\tgit daemon --base-path=\"$(pwd)\" \\\n+\t\t--port=\"$PORT\" \\\n+\t\t--detach \\\n+\t\t--pid-file=pid-file \\\n+\t\t-- \"$(pwd)/first\"\n+\tDAEMON_PID=$(cat pid-file)\n+}\n+\n+test_expect_success 'daemon' '\n+\n+\tstart_daemon &&\n+\ttest -f pid-file\n+\n+'\n+\n+test_expect_success 'clone fails without export-ok' '\n+\n+\t! git clone git://127.0.0.1:\"$PORT\"/first second\n+\n+'\n+\n+test_expect_success 'clone succeeds with export-ok' '\n+\n+\t: > first/.git/git-daemon-export-ok &&\n+\tgit clone git://127.0.0.1:\"$PORT\"/first second &&\n+\tfor f in refs/heads/master objects/pack/*.pack\n+\tdo\n+\t\tcmp first/.git/$f second/.git/$f || break\n+\tdone\n+\n+'\n+\n+test_done\n-- \n1.5.4.1264.gb53928\n"}]}