{"thread":{"id":"5557","subject":"[PATCH] Trivial support for cloning and fetching via ftp://.","startedAt":"2006-09-14T02:24:04Z","lastAt":"2006-09-16T19:54:51Z","messageCount":11,"participants":["Sasha Khapyorsky","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"26858","messageId":"20060914022404.GA900@sashak.voltaire.com","threadId":"5557","inReplyTo":null,"subject":"[PATCH] Trivial support for cloning and fetching via ftp://.","fromName":"Sasha Khapyorsky","fromEmail":"sashak@voltaire.com","sentAt":"2006-09-14T02:24:04Z","receivedAt":"2006-09-14T02:24:04Z","isPatch":true,"sender":{"key":"sashak@voltaire.com","avatar":null},"body":"This adds trivial support for cloning and fetching via ftp://.\n\nSigned-off-by: Sasha Khapyorsky <sashak@voltaire.com>\n---\n git-clone.sh     |    2 +-\n git-fetch.sh     |    4 ++--\n git-ls-remote.sh |    2 +-\n 3 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/git-clone.sh b/git-clone.sh\nindex 7060bda..e1b3bf3 100755\n--- a/git-clone.sh\n+++ b/git-clone.sh\n@@ -298,7 +298,7 @@ yes,yes)\n \t\tfi\n \t\tgit-ls-remote \"$repo\" >\"$GIT_DIR/CLONE_HEAD\" || exit 1\n \t\t;;\n-\thttps://*|http://*)\n+\thttps://*|http://*|ftp://*)\n \t\tif test -z \"@@NO_CURL@@\"\n \t\tthen\n \t\t\tclone_dumb_http \"$repo\" \"$D\"\ndiff --git a/git-fetch.sh b/git-fetch.sh\nindex c2eebee..09a5d6c 100755\n--- a/git-fetch.sh\n+++ b/git-fetch.sh\n@@ -286,7 +286,7 @@ fetch_main () {\n \n       # There are transports that can fetch only one head at a time...\n       case \"$remote\" in\n-      http://* | https://*)\n+      http://* | https://* | ftp://*)\n \t  if [ -n \"$GIT_SSL_NO_VERIFY\" ]; then\n \t      curl_extra_args=\"-k\"\n \t  fi\n@@ -350,7 +350,7 @@ fetch_main () {\n   done\n \n   case \"$remote\" in\n-  http://* | https://* | rsync://* )\n+  http://* | https://* | ftp://* | rsync://* )\n       ;; # we are already done.\n   *)\n     ( : subshell because we muck with IFS\ndiff --git a/git-ls-remote.sh b/git-ls-remote.sh\nindex 2fdcaf7..2c0b521 100755\n--- a/git-ls-remote.sh\n+++ b/git-ls-remote.sh\n@@ -49,7 +49,7 @@ trap \"rm -fr $tmp-*\" 0 1 2 3 15\n tmpdir=$tmp-d\n \n case \"$peek_repo\" in\n-http://* | https://* )\n+http://* | https://* | ftp://* )\n         if [ -n \"$GIT_SSL_NO_VERIFY\" ]; then\n             curl_extra_args=\"-k\"\n         fi\n-- \n1.4.2.gffe87-dirty\n"},{"id":"26873","messageId":"7vk6475408.fsf@assigned-by-dhcp.cox.net","threadId":"5557","inReplyTo":"20060914022404.GA900@sashak.voltaire.com","subject":"Re: [PATCH] Trivial support for cloning and fetching via ftp://.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-14T06:57:59Z","receivedAt":"2006-09-14T06:57:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sasha Khapyorsky <sashak@voltaire.com> writes:\n\n> This adds trivial support for cloning and fetching via ftp://.\n\nInteresting.\n\nI was wondering myself if our use of curl libraries in\nhttp-fetch allows us to do this when I was looking at the\nalternates breakage yesterday.\n\nAt a few places we do look at http error code that is returned\nfrom the curl library, and change our behaviour based on that.\nBut it appears the difference between error code from ftp and\nhttp has no bad effect on us.  In an empty repository, we can\nrun this:\n\n\t$ git-http-fetch -a -v heads/merge \\\n\t  ftp://ftp.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc.git\n\n(of course, this should normally be with http://www.kernel.org).\nWe notice that we get an error from a request for one object,\nand switch to pack & alternates transfer.  The only difference\nbetween http://www and ftp://ftp is that for the former we know\nerror code 404 and supress the error message but for the latter\nwe do not treat error 550 from RETR response any specially and\nshow an error message.  We still fall back to retrieve packs,\nhoping that the missing object is in a pack.\n\nI'd take this patch as is, but we might want to add some error\nmessage supression logic just like we do for http.\n"},{"id":"26990","messageId":"20060916023717.GA13570@sashak.voltaire.com","threadId":"5557","inReplyTo":"7vk6475408.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Trivial support for cloning and fetching via ftp://.","fromName":"Sasha Khapyorsky","fromEmail":"sashak@voltaire.com","sentAt":"2006-09-16T02:37:17Z","receivedAt":"2006-09-16T02:37:17Z","isPatch":true,"sender":{"key":"sashak@voltaire.com","avatar":null},"body":"On 23:57 Wed 13 Sep     , Junio C Hamano wrote:\n> Sasha Khapyorsky <sashak@voltaire.com> writes:\n> \n> > This adds trivial support for cloning and fetching via ftp://.\n> \n> Interesting.\n> \n> I was wondering myself if our use of curl libraries in\n> http-fetch allows us to do this when I was looking at the\n> alternates breakage yesterday.\n> \n> At a few places we do look at http error code that is returned\n> from the curl library, and change our behaviour based on that.\n> But it appears the difference between error code from ftp and\n> http has no bad effect on us.  In an empty repository, we can\n> run this:\n> \n> \t$ git-http-fetch -a -v heads/merge \\\n> \t  ftp://ftp.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc.git\n> \n> (of course, this should normally be with http://www.kernel.org).\n> We notice that we get an error from a request for one object,\n> and switch to pack & alternates transfer.  The only difference\n> between http://www and ftp://ftp is that for the former we know\n> error code 404 and supress the error message but for the latter\n> we do not treat error 550 from RETR response any specially and\n> show an error message.  We still fall back to retrieve packs,\n> hoping that the missing object is in a pack.\n> \n> I'd take this patch as is, but we might want to add some error\n> message supression logic just like we do for http.\n\nSomething like this?\n\nWith this change I'm able to clone\nftp://ftp.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc.git\n\n\ndiff --git a/http-fetch.c b/http-fetch.c\nindex a113bb8..46d6029 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -324,7 +324,9 @@ static void process_object_response(void\n \n \t/* Use alternates if necessary */\n \tif (obj_req->http_code == 404 ||\n-\t    obj_req->curl_result == CURLE_FILE_COULDNT_READ_FILE) {\n+\t    obj_req->curl_result == CURLE_FILE_COULDNT_READ_FILE ||\n+\t    (obj_req->http_code == 550 &&\n+\t     obj_req->curl_result == CURLE_FTP_COULDNT_RETR_FILE)) {\n \t\tfetch_alternates(alt->base);\n \t\tif (obj_req->repo->next != NULL) {\n \t\t\tobj_req->repo =\n@@ -538,7 +540,9 @@ static void process_alternates_response(\n \t\t}\n \t} else if (slot->curl_result != CURLE_OK) {\n \t\tif (slot->http_code != 404 &&\n-\t\t    slot->curl_result != CURLE_FILE_COULDNT_READ_FILE) {\n+\t\t    slot->curl_result != CURLE_FILE_COULDNT_READ_FILE &&\n+\t\t    (slot->http_code != 550 &&\n+\t\t     slot->curl_result != CURLE_FTP_COULDNT_RETR_FILE)) {\n \t\t\tgot_alternates = -1;\n \t\t\treturn;\n \t\t}\n@@ -942,7 +946,9 @@ #endif\n \t\trun_active_slot(slot);\n \t\tif (results.curl_result != CURLE_OK) {\n \t\t\tif (results.http_code == 404 ||\n-\t\t\t    results.curl_result == CURLE_FILE_COULDNT_READ_FILE) {\n+\t\t\t    results.curl_result == CURLE_FILE_COULDNT_READ_FILE ||\n+\t\t\t    (results.http_code == 550 &&\n+\t\t\t     results.curl_result == CURLE_FTP_COULDNT_RETR_FILE)) {\n \t\t\t\trepo->got_indices = 1;\n \t\t\t\tfree(buffer.buffer);\n \t\t\t\treturn 0;\n@@ -1124,7 +1130,9 @@ #endif\n \t} else if (obj_req->curl_result != CURLE_OK &&\n \t\t   obj_req->http_code != 416) {\n \t\tif (obj_req->http_code == 404 ||\n-\t\t    obj_req->curl_result == CURLE_FILE_COULDNT_READ_FILE)\n+\t\t    obj_req->curl_result == CURLE_FILE_COULDNT_READ_FILE ||\n+\t\t    (obj_req->http_code == 550 &&\n+\t\t     obj_req->curl_result == CURLE_FTP_COULDNT_RETR_FILE))\n \t\t\tret = -1; /* Be silent, it is probably in a pack. */\n \t\telse\n \t\t\tret = error(\"%s (curl_result = %d, http_code = %ld, sha1 = %s)\",\n"},{"id":"26997","messageId":"7vwt849nv6.fsf@assigned-by-dhcp.cox.net","threadId":"5557","inReplyTo":"20060916023717.GA13570@sashak.voltaire.com","subject":"Re: [PATCH] Trivial support for cloning and fetching via ftp://.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-16T09:12:13Z","receivedAt":"2006-09-16T09:12:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sasha Khapyorsky <sashak@voltaire.com> writes:\n\n> Something like this?\n>\n> With this change I'm able to clone\n> ftp://ftp.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc.git\n\nI think without you would have, just with extra error messages\nthat http codepath filters out.\n\n> diff --git a/http-fetch.c b/http-fetch.c\n> index a113bb8..46d6029 100644\n> --- a/http-fetch.c\n> +++ b/http-fetch.c\n> @@ -324,7 +324,9 @@ static void process_object_response(void\n>  \n>  \t/* Use alternates if necessary */\n>  \tif (obj_req->http_code == 404 ||\n> -\t    obj_req->curl_result == CURLE_FILE_COULDNT_READ_FILE) {\n> +\t    obj_req->curl_result == CURLE_FILE_COULDNT_READ_FILE ||\n> +\t    (obj_req->http_code == 550 &&\n> +\t     obj_req->curl_result == CURLE_FTP_COULDNT_RETR_FILE)) {\n\nHere you do the same as the code would for HTTP 404 when you get\n550 _and_ RETR failure...\n\n> @@ -538,7 +540,9 @@ static void process_alternates_response(\n>  \t\t}\n>  \t} else if (slot->curl_result != CURLE_OK) {\n>  \t\tif (slot->http_code != 404 &&\n> -\t\t    slot->curl_result != CURLE_FILE_COULDNT_READ_FILE) {\n> +\t\t    slot->curl_result != CURLE_FILE_COULDNT_READ_FILE &&\n> +\t\t    (slot->http_code != 550 &&\n> +\t\t     slot->curl_result != CURLE_FTP_COULDNT_RETR_FILE)) {\n>  \t\t\tgot_alternates = -1;\n\n... but you say, while the original code says \"declare error if\nit is not HTTP 404\", \"oh by the way, if it is 550 _or_ if it\nis RETR failure then do not trigger this if()\".  I suspect you\nmeant to say this?\n\n\t    (slot->http_code != 550 ||\n\t     slot->curl_result != CURLE_FTP_COULDNT_RETR_FILE)) {\n"},{"id":"26999","messageId":"20060916100147.GA17504@sashak.voltaire.com","threadId":"5557","inReplyTo":"7vwt849nv6.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Trivial support for cloning and fetching via ftp://.","fromName":"Sasha Khapyorsky","fromEmail":"sashak@voltaire.com","sentAt":"2006-09-16T10:01:47Z","receivedAt":"2006-09-16T10:01:47Z","isPatch":true,"sender":{"key":"sashak@voltaire.com","avatar":null},"body":"On 02:12 Sat 16 Sep     , Junio C Hamano wrote:\n> Sasha Khapyorsky <sashak@voltaire.com> writes:\n> \n> > Something like this?\n> >\n> > With this change I'm able to clone\n> > ftp://ftp.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc.git\n> \n> I think without you would have, just with extra error messages\n> that http codepath filters out.\n\nNo, not really, without change it fails later:\n\n$ git-clone ftp://ftp.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc.git\nerror: RETR response: 550 (curl_result = 19, http_code = 550, sha1 = 63b98080daa35f0d682db04f4fb7ada010888752)\nGetting pack list for ftp://ftp.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc.git/\nGetting alternates list for ftp://ftp.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc.git/\nAlso look at ftp://ftp.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git/\nGetting pack list for ftp://ftp.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git/\nGetting index for pack 477061883bee3d10bece6e3432355b61ba02e594\nerror: Unable to find 63b98080daa35f0d682db04f4fb7ada010888752 under ftp://ftp.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc.git/\nCannot obtain needed none 63b98080daa35f0d682db04f4fb7ada010888752\nwhile processing commit 0000000000000000000000000000000000000000.\n\n> \n> > diff --git a/http-fetch.c b/http-fetch.c\n> > index a113bb8..46d6029 100644\n> > --- a/http-fetch.c\n> > +++ b/http-fetch.c\n> > @@ -324,7 +324,9 @@ static void process_object_response(void\n> >  \n> >  \t/* Use alternates if necessary */\n> >  \tif (obj_req->http_code == 404 ||\n> > -\t    obj_req->curl_result == CURLE_FILE_COULDNT_READ_FILE) {\n> > +\t    obj_req->curl_result == CURLE_FILE_COULDNT_READ_FILE ||\n> > +\t    (obj_req->http_code == 550 &&\n> > +\t     obj_req->curl_result == CURLE_FTP_COULDNT_RETR_FILE)) {\n> \n> Here you do the same as the code would for HTTP 404 when you get\n> 550 _and_ RETR failure...\n> \n> > @@ -538,7 +540,9 @@ static void process_alternates_response(\n> >  \t\t}\n> >  \t} else if (slot->curl_result != CURLE_OK) {\n> >  \t\tif (slot->http_code != 404 &&\n> > -\t\t    slot->curl_result != CURLE_FILE_COULDNT_READ_FILE) {\n> > +\t\t    slot->curl_result != CURLE_FILE_COULDNT_READ_FILE &&\n> > +\t\t    (slot->http_code != 550 &&\n> > +\t\t     slot->curl_result != CURLE_FTP_COULDNT_RETR_FILE)) {\n> >  \t\t\tgot_alternates = -1;\n> \n> ... but you say, while the original code says \"declare error if\n> it is not HTTP 404\", \"oh by the way, if it is 550 _or_ if it\n> is RETR failure then do not trigger this if()\".  I suspect you\n> meant to say this?\n> \n> \t    (slot->http_code != 550 ||\n> \t     slot->curl_result != CURLE_FTP_COULDNT_RETR_FILE)) {\n\nI think with less strict checking this could be done so, but with _and_\nthis also ensures that we are really in FTP mode.\n\nSasha\n"},{"id":"27000","messageId":"20060916105131.GC17504@sashak.voltaire.com","threadId":"5557","inReplyTo":"20060916100147.GA17504@sashak.voltaire.com","subject":"Re: [PATCH] Trivial support for cloning and fetching via ftp://.","fromName":"Sasha Khapyorsky","fromEmail":"sashak@voltaire.com","sentAt":"2006-09-16T10:51:31Z","receivedAt":"2006-09-16T10:51:31Z","isPatch":true,"sender":{"key":"sashak@voltaire.com","avatar":null},"body":"On 13:01 Sat 16 Sep     , Sasha Khapyorsky wrote:\n> On 02:12 Sat 16 Sep     , Junio C Hamano wrote:\n> > Sasha Khapyorsky <sashak@voltaire.com> writes:\n> > \n> > > Something like this?\n> > >\n> > > With this change I'm able to clone\n> > > ftp://ftp.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc.git\n> > \n> > I think without you would have, just with extra error messages\n> > that http codepath filters out.\n> \n> No, not really, without change it fails later:\n> \n> $ git-clone ftp://ftp.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc.git\n> error: RETR response: 550 (curl_result = 19, http_code = 550, sha1 = 63b98080daa35f0d682db04f4fb7ada010888752)\n> Getting pack list for ftp://ftp.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc.git/\n> Getting alternates list for ftp://ftp.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc.git/\n> Also look at ftp://ftp.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git/\n> Getting pack list for ftp://ftp.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git/\n> Getting index for pack 477061883bee3d10bece6e3432355b61ba02e594\n> error: Unable to find 63b98080daa35f0d682db04f4fb7ada010888752 under ftp://ftp.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc.git/\n> Cannot obtain needed none 63b98080daa35f0d682db04f4fb7ada010888752\n> while processing commit 0000000000000000000000000000000000000000.\n> \n> > \n> > > diff --git a/http-fetch.c b/http-fetch.c\n> > > index a113bb8..46d6029 100644\n> > > --- a/http-fetch.c\n> > > +++ b/http-fetch.c\n> > > @@ -324,7 +324,9 @@ static void process_object_response(void\n> > >  \n> > >  \t/* Use alternates if necessary */\n> > >  \tif (obj_req->http_code == 404 ||\n> > > -\t    obj_req->curl_result == CURLE_FILE_COULDNT_READ_FILE) {\n> > > +\t    obj_req->curl_result == CURLE_FILE_COULDNT_READ_FILE ||\n> > > +\t    (obj_req->http_code == 550 &&\n> > > +\t     obj_req->curl_result == CURLE_FTP_COULDNT_RETR_FILE)) {\n> > \n> > Here you do the same as the code would for HTTP 404 when you get\n> > 550 _and_ RETR failure...\n> > \n> > > @@ -538,7 +540,9 @@ static void process_alternates_response(\n> > >  \t\t}\n> > >  \t} else if (slot->curl_result != CURLE_OK) {\n> > >  \t\tif (slot->http_code != 404 &&\n> > > -\t\t    slot->curl_result != CURLE_FILE_COULDNT_READ_FILE) {\n> > > +\t\t    slot->curl_result != CURLE_FILE_COULDNT_READ_FILE &&\n> > > +\t\t    (slot->http_code != 550 &&\n> > > +\t\t     slot->curl_result != CURLE_FTP_COULDNT_RETR_FILE)) {\n> > >  \t\t\tgot_alternates = -1;\n> > \n> > ... but you say, while the original code says \"declare error if\n> > it is not HTTP 404\", \"oh by the way, if it is 550 _or_ if it\n> > is RETR failure then do not trigger this if()\".  I suspect you\n> > meant to say this?\n> > \n> > \t    (slot->http_code != 550 ||\n> > \t     slot->curl_result != CURLE_FTP_COULDNT_RETR_FILE)) {\n> \n> I think with less strict checking this could be done so, but with _and_\n> this also ensures that we are really in FTP mode.\n\nHmm, saying this I see that original code doesn't do it for specific\ncase. So for this case we could do:\n\n \t    !(slot->http_code == 550 &&\n \t     slot->curl_result == CURLE_FTP_COULDNT_RETR_FILE)) {\n\nSasha\n"},{"id":"27003","messageId":"7virjnafev.fsf@assigned-by-dhcp.cox.net","threadId":"5557","inReplyTo":"20060916100147.GA17504@sashak.voltaire.com","subject":"Re: [PATCH] Trivial support for cloning and fetching via ftp://.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-16T17:29:28Z","receivedAt":"2006-09-16T17:29:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sasha Khapyorsky <sashak@voltaire.com> writes:\n\n>> > diff --git a/http-fetch.c b/http-fetch.c\n>> > index a113bb8..46d6029 100644\n>> > --- a/http-fetch.c\n>> > +++ b/http-fetch.c\n>> > @@ -324,7 +324,9 @@ static void process_object_response(void\n>> >  \n>> >  \t/* Use alternates if necessary */\n>> >  \tif (obj_req->http_code == 404 ||\n>> > -\t    obj_req->curl_result == CURLE_FILE_COULDNT_READ_FILE) {\n>> > +\t    obj_req->curl_result == CURLE_FILE_COULDNT_READ_FILE ||\n>> > +\t    (obj_req->http_code == 550 &&\n>> > +\t     obj_req->curl_result == CURLE_FTP_COULDNT_RETR_FILE)) {\n>> \n>> Here you do the same as the code would for HTTP 404 when you get\n>> 550 _and_ RETR failure...\n>> \n>> > @@ -538,7 +540,9 @@ static void process_alternates_response(\n>> >  \t\t}\n>> >  \t} else if (slot->curl_result != CURLE_OK) {\n>> >  \t\tif (slot->http_code != 404 &&\n>> > -\t\t    slot->curl_result != CURLE_FILE_COULDNT_READ_FILE) {\n>> > +\t\t    slot->curl_result != CURLE_FILE_COULDNT_READ_FILE &&\n>> > +\t\t    (slot->http_code != 550 &&\n>> > +\t\t     slot->curl_result != CURLE_FTP_COULDNT_RETR_FILE)) {\n>> >  \t\t\tgot_alternates = -1;\n>> \n>> ... but you say, while the original code says \"declare error if\n>> it is not HTTP 404\", \"oh by the way, if it is 550 _or_ if it\n>> is RETR failure then do not trigger this if()\".  I suspect you\n>> meant to say this?\n>> \n>> \t    (slot->http_code != 550 ||\n>> \t     slot->curl_result != CURLE_FTP_COULDNT_RETR_FILE)) {\n>\n> I think with less strict checking this could be done so, but with _and_\n> this also ensures that we are really in FTP mode.\n\nI was merely pointing out that in one place you have:\n\n\t(http_code == 550 && result == ERETR)\n\nand another place that tries to say the opposite you have:\n\n\t(http_code != 550 && result != ERETR)\n\nwhich is not the same thing as\n\n\t!(http_code == 550 && result == ERETR)\n\nI understood, from the former \"Use alternates if necessary\"\npart, that you wanted to make sure that 550 is really from\nFTP_RETR and not other random HTTP error message, and I think\nthat is a reasonable thing to do.\n"},{"id":"27004","messageId":"20060916174134.GE17504@sashak.voltaire.com","threadId":"5557","inReplyTo":"7virjnafev.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Trivial support for cloning and fetching via ftp://.","fromName":"Sasha Khapyorsky","fromEmail":"sashak@voltaire.com","sentAt":"2006-09-16T17:41:34Z","receivedAt":"2006-09-16T17:41:34Z","isPatch":true,"sender":{"key":"sashak@voltaire.com","avatar":null},"body":"On 10:29 Sat 16 Sep     , Junio C Hamano wrote:\n> Sasha Khapyorsky <sashak@voltaire.com> writes:\n> \n> >> > diff --git a/http-fetch.c b/http-fetch.c\n> >> > index a113bb8..46d6029 100644\n> >> > --- a/http-fetch.c\n> >> > +++ b/http-fetch.c\n> >> > @@ -324,7 +324,9 @@ static void process_object_response(void\n> >> >  \n> >> >  \t/* Use alternates if necessary */\n> >> >  \tif (obj_req->http_code == 404 ||\n> >> > -\t    obj_req->curl_result == CURLE_FILE_COULDNT_READ_FILE) {\n> >> > +\t    obj_req->curl_result == CURLE_FILE_COULDNT_READ_FILE ||\n> >> > +\t    (obj_req->http_code == 550 &&\n> >> > +\t     obj_req->curl_result == CURLE_FTP_COULDNT_RETR_FILE)) {\n> >> \n> >> Here you do the same as the code would for HTTP 404 when you get\n> >> 550 _and_ RETR failure...\n> >> \n> >> > @@ -538,7 +540,9 @@ static void process_alternates_response(\n> >> >  \t\t}\n> >> >  \t} else if (slot->curl_result != CURLE_OK) {\n> >> >  \t\tif (slot->http_code != 404 &&\n> >> > -\t\t    slot->curl_result != CURLE_FILE_COULDNT_READ_FILE) {\n> >> > +\t\t    slot->curl_result != CURLE_FILE_COULDNT_READ_FILE &&\n> >> > +\t\t    (slot->http_code != 550 &&\n> >> > +\t\t     slot->curl_result != CURLE_FTP_COULDNT_RETR_FILE)) {\n> >> >  \t\t\tgot_alternates = -1;\n> >> \n> >> ... but you say, while the original code says \"declare error if\n> >> it is not HTTP 404\", \"oh by the way, if it is 550 _or_ if it\n> >> is RETR failure then do not trigger this if()\".  I suspect you\n> >> meant to say this?\n> >> \n> >> \t    (slot->http_code != 550 ||\n> >> \t     slot->curl_result != CURLE_FTP_COULDNT_RETR_FILE)) {\n> >\n> > I think with less strict checking this could be done so, but with _and_\n> > this also ensures that we are really in FTP mode.\n> \n> I was merely pointing out that in one place you have:\n> \n> \t(http_code == 550 && result == ERETR)\n> \n> and another place that tries to say the opposite you have:\n> \n> \t(http_code != 550 && result != ERETR)\n> \n> which is not the same thing as\n> \n> \t!(http_code == 550 && result == ERETR)\n> \n> I understood, from the former \"Use alternates if necessary\"\n> part, that you wanted to make sure that 550 is really from\n> FTP_RETR and not other random HTTP error message, and I think\n> that is a reasonable thing to do.\n\nGood. Am I need to send the patch or you will integrate it?\n\nSasha\n"},{"id":"27005","messageId":"7vd59vae2r.fsf@assigned-by-dhcp.cox.net","threadId":"5557","inReplyTo":"20060916174134.GE17504@sashak.voltaire.com","subject":"Re: [PATCH] Trivial support for cloning and fetching via ftp://.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-16T17:58:20Z","receivedAt":"2006-09-16T17:58:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sasha Khapyorsky <sashak@voltaire.com> writes:\n\n> Good. Am I need to send the patch or you will integrate it?\n\nActually, I am thinking of doing this in two steps.\n\nThe attached is the first \"clean-up\" step, which should be\nobvious enough.\n\nAnd you already know what the second one that would come on top\nof this should look like ;-).\n\n-- >8 --\nhttp-fetch.c: consolidate code to detect missing fetch target\n\nAt a handful places we check two error codes from curl library\nto see if the file we asked was missing from the remote (e.g.\nwe asked for a loose object when it is in a pack) to decide what\nto do next.  This consolidates the check into a single function.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\ndiff --git a/http-fetch.c b/http-fetch.c\nindex a113bb8..bc74f30 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -144,6 +144,19 @@ static size_t fwrite_sha1_file(void *ptr\n \treturn size;\n }\n \n+static int missing__target(int code, int result)\n+{\n+\treturn\t/* file:// URL -- do we ever use one??? */\n+\t\t(result == CURLE_FILE_COULDNT_READ_FILE) ||\n+\t\t/* http:// and https:// URL */\n+\t\t(code == 404 && result == CURLE_HTTP_RETURNED_ERROR) ||\n+\t\t/* ftp:// URL */\n+\t\t(code == 550 && result == CURLE_FTP_COULDNT_RETR_FILE)\n+\t\t;\n+}\n+\n+#define missing_target(a) missing__target((a)->http_code, (a)->curl_result)\n+\n static void fetch_alternates(const char *base);\n \n static void process_object_response(void *callback_data);\n@@ -323,8 +336,7 @@ static void process_object_response(void\n \tobj_req->state = COMPLETE;\n \n \t/* Use alternates if necessary */\n-\tif (obj_req->http_code == 404 ||\n-\t    obj_req->curl_result == CURLE_FILE_COULDNT_READ_FILE) {\n+\tif (missing_target(obj_req)) {\n \t\tfetch_alternates(alt->base);\n \t\tif (obj_req->repo->next != NULL) {\n \t\t\tobj_req->repo =\n@@ -537,8 +549,7 @@ static void process_alternates_response(\n \t\t\treturn;\n \t\t}\n \t} else if (slot->curl_result != CURLE_OK) {\n-\t\tif (slot->http_code != 404 &&\n-\t\t    slot->curl_result != CURLE_FILE_COULDNT_READ_FILE) {\n+\t\tif (!missing_target(slot)) {\n \t\t\tgot_alternates = -1;\n \t\t\treturn;\n \t\t}\n@@ -941,8 +952,7 @@ #endif\n \tif (start_active_slot(slot)) {\n \t\trun_active_slot(slot);\n \t\tif (results.curl_result != CURLE_OK) {\n-\t\t\tif (results.http_code == 404 ||\n-\t\t\t    results.curl_result == CURLE_FILE_COULDNT_READ_FILE) {\n+\t\t\tif (missing_target(&results)) {\n \t\t\t\trepo->got_indices = 1;\n \t\t\t\tfree(buffer.buffer);\n \t\t\t\treturn 0;\n@@ -1123,8 +1133,7 @@ #endif\n \t\tret = error(\"Request for %s aborted\", hex);\n \t} else if (obj_req->curl_result != CURLE_OK &&\n \t\t   obj_req->http_code != 416) {\n-\t\tif (obj_req->http_code == 404 ||\n-\t\t    obj_req->curl_result == CURLE_FILE_COULDNT_READ_FILE)\n+\t\tif (missing_target(obj_req))\n \t\t\tret = -1; /* Be silent, it is probably in a pack. */\n \t\telse\n \t\t\tret = error(\"%s (curl_result = %d, http_code = %ld, sha1 = %s)\",\n"},{"id":"27007","messageId":"7v8xkjadzv.fsf@assigned-by-dhcp.cox.net","threadId":"5557","inReplyTo":"7vd59vae2r.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Trivial support for cloning and fetching via ftp://.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-16T18:00:04Z","receivedAt":"2006-09-16T18:00:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> Sasha Khapyorsky <sashak@voltaire.com> writes:\n>\n>> Good. Am I need to send the patch or you will integrate it?\n>\n> Actually, I am thinking of doing this in two steps.\n>\n> The attached is the first \"clean-up\" step, which should be\n> obvious enough.\n>\n> And you already know what the second one that would come on top\n> of this should look like ;-).\n\nOops, thinko.  I sent a rolled-up one out.\n"},{"id":"27014","messageId":"20060916195451.GF17504@sashak.voltaire.com","threadId":"5557","inReplyTo":"7v8xkjadzv.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Trivial support for cloning and fetching via ftp://.","fromName":"Sasha Khapyorsky","fromEmail":"sashak@voltaire.com","sentAt":"2006-09-16T19:54:51Z","receivedAt":"2006-09-16T19:54:51Z","isPatch":true,"sender":{"key":"sashak@voltaire.com","avatar":null},"body":"On 11:00 Sat 16 Sep     , Junio C Hamano wrote:\n> Junio C Hamano <junkio@cox.net> writes:\n> \n> > Sasha Khapyorsky <sashak@voltaire.com> writes:\n> >\n> >> Good. Am I need to send the patch or you will integrate it?\n> >\n> > Actually, I am thinking of doing this in two steps.\n> >\n> > The attached is the first \"clean-up\" step, which should be\n> > obvious enough.\n> >\n> > And you already know what the second one that would come on top\n> > of this should look like ;-).\n> \n> Oops, thinko.  I sent a rolled-up one out.\n\nYes, and it looks good. Thanks :)\n\nSasha\n"}]}