{"thread":{"id":"19672","subject":"[WIP] Shift rev-list enumeration from upload-pack to pack-objects","startedAt":"2009-06-05T05:45:00Z","lastAt":"2009-06-08T08:51:22Z","messageCount":15,"participants":["sam@vilain.net","Sam Vilain","Johannes Sixt","Nicolas Pitre","Nick Edelen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"115478","messageId":"20090605054500.06A9D21C3F4@mail.utsl.gen.nz","threadId":"19672","inReplyTo":null,"subject":"[WIP] Shift rev-list enumeration from upload-pack to pack-objects","fromName":"","fromEmail":"sam@vilain.net","sentAt":"2009-06-05T05:45:00Z","receivedAt":"2009-06-05T05:45:00Z","isPatch":false,"sender":{"key":"sam@vilain.net","avatar":"https://gravatar.com/avatar/8fc840ca854dbf6f7065b4335e3b934951c1dca3b11db688e95e471901f8f4a8?d=mp&s=160"},"body":"instead of using the internal revision walker and piping object refs\nto pack-objects this patch passes only the revs to pack-objects, which\nin turn handles both enumeration and packing.\n\nSigned-off-by: Sam Vilain \n---\n Submitted on behalf of Nick in order to get wider feedback on this.\n This version passes the test suite.\n\n upload-pack.c | 54 +++++++++++++++++++++++++++++++++++++++++++++---------\n 1 files changed, 45 insertions(+), 9 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex edc7861..7eda8fd 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -155,13 +155,27 @@ static void create_pack_file(void)\n \tconst char *argv[10];\n \tint arg = 0;\n \n-\trev_list.proc = do_rev_list;\n-\t/* .data is just a boolean: any non-NULL value will do */\n-\trev_list.data = create_full_pack ? &rev_list : NULL;\n-\tif (start_async(&rev_list))\n-\t\tdie(\"git upload-pack: unable to fork git-rev-list\");\n-\n-\targv[arg++] = \"pack-objects\";\n+\t/* sending rev params to pack-objects directly is great, but unfortunately pack-objects \n+\t * has no way of turning off thin pack generation. this would be a relatively simple \n+\t * addition, but as we also have to deal with shallow grafts and all it's simplest to \n+\t * just resort to piping object refs.\n+\t */\n+\tif (!use_thin_pack) {\n+\t\trev_list.proc = do_rev_list;\n+\t\t/* .data is just a boolean: any non-NULL value will do */\n+\t\trev_list.data = create_full_pack ? &rev_list : NULL;\n+\t\tif (start_async(&rev_list))\n+\t\t\tdie(\"git upload-pack: unable to fork git-rev-list\");\n+\t\t\n+\t\targv[arg++] = \"pack-objects\";\n+\t} else {\n+\t\targv[arg++] = \"pack-objects\";\n+\t\targv[arg++] = \"--revs\";\n+\t\targv[arg++] = \"--include-tag\";\n+\t\tif (create_full_pack)\n+\t\t\targv[arg++] = \"--all\";\n+\t}\n+\t\n \targv[arg++] = \"--stdout\";\n \tif (!no_progress)\n \t\targv[arg++] = \"--progress\";\n@@ -172,7 +186,7 @@ static void create_pack_file(void)\n \targv[arg++] = NULL;\n \n \tmemset(&pack_objects, 0, sizeof(pack_objects));\n-\tpack_objects.in = rev_list.out;\t/* start_command closes it */\n+\tpack_objects.in = !use_thin_pack ? rev_list.out : -1;\n \tpack_objects.out = -1;\n \tpack_objects.err = -1;\n \tpack_objects.git_cmd = 1;\n@@ -181,6 +195,28 @@ static void create_pack_file(void)\n \tif (start_command(&pack_objects))\n \t\tdie(\"git upload-pack: unable to fork git-pack-objects\");\n \n+\t/* pass on revisions we (don't) want \n+\t * (do we need to check the validity of pack_objects.in?)\n+\t */\n+\tif (use_thin_pack) {\n+\t\tFILE *pipe_fd = fdopen(pack_objects.in, \"w\");\n+\t\tif (!create_full_pack) {\n+\t\t\tint i;\n+\t\t\tfor (i = 0; i < want_obj.nr; i++) {\n+\t\t\t\tfprintf(pipe_fd, \"%s\\n\", sha1_to_hex(want_obj.objects[i].item->sha1));\n+\t\t\t}\n+\t\t\tfprintf(pipe_fd, \"--not\\n\");\n+\t\t\tfor (i = 0; i < have_obj.nr; i++) {\n+\t\t\t\tfprintf(pipe_fd, \"%s\\n\", sha1_to_hex(have_obj.objects[i].item->sha1));\n+\t\t\t}\n+\t\t}\n+\t\t\n+\t\tfprintf(pipe_fd, \"\\n\");\n+\t\tfflush(pipe_fd);\n+\t\tfclose(pipe_fd);\n+\t}\n+\n+\n \t/* We read from pack_objects.err to capture stderr output for\n \t * progress bar, and pack_objects.out to capture the pack data.\n \t */\n@@ -276,7 +312,7 @@ static void create_pack_file(void)\n \t\terror(\"git upload-pack: git-pack-objects died with error.\");\n \t\tgoto fail;\n \t}\n-\tif (finish_async(&rev_list))\n+\tif (!use_thin_pack && finish_async(&rev_list))\n \t\tgoto fail;\t/* error was already reported */\n \n \t/* flush the data */\n-- \ntg: (9d7169f..) t/revcache/upload-pack-dont-enumerate (depends on: git-daemon-caching)\ntg: The patch is out-of-date wrt. the base! Run `tg update`.\n"},{"id":"115481","messageId":"4A28B128.6040709@vilain.net","threadId":"19672","inReplyTo":"20090605054500.06A9D21C3F4@mail.utsl.gen.nz","subject":"Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects","fromName":"Sam Vilain","fromEmail":"sam@vilain.net","sentAt":"2009-06-05T05:46:16Z","receivedAt":"2009-06-05T05:46:16Z","isPatch":false,"sender":{"key":"sam@vilain.net","avatar":"https://gravatar.com/avatar/8fc840ca854dbf6f7065b4335e3b934951c1dca3b11db688e95e471901f8f4a8?d=mp&s=160"},"body":"Sorry, stuffed up the headers on this one, too - that should have been\nme in the \"From:\" of the e-mail, but it is Nick's work...\n\nSam.\n\nNick Edelen wrote:\n> instead of using the internal revision walker and piping object refs\n> to pack-objects this patch passes only the revs to pack-objects, which\n> in turn handles both enumeration and packing.\n>\n> Signed-off-by: Sam Vilain \n> ---\n> Submitted on behalf of Nick in order to get wider feedback on this.\n> This version passes the test suite.\n>\n> upload-pack.c | 54 +++++++++++++++++++++++++++++++++++++++++++++---------\n> 1 files changed, 45 insertions(+), 9 deletions(-)\n>\n> diff --git a/upload-pack.c b/upload-pack.c\n> index edc7861..7eda8fd 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -155,13 +155,27 @@ static void create_pack_file(void)\n> \tconst char *argv[10];\n> \tint arg = 0;\n> \n> -\trev_list.proc = do_rev_list;\n> -\t/* .data is just a boolean: any non-NULL value will do */\n> -\trev_list.data = create_full_pack ? &rev_list : NULL;\n> -\tif (start_async(&rev_list))\n> -\t\tdie(\"git upload-pack: unable to fork git-rev-list\");\n> -\n> -\targv[arg++] = \"pack-objects\";\n> +\t/* sending rev params to pack-objects directly is great, but unfortunately pack-objects \n> +\t * has no way of turning off thin pack generation. this would be a relatively simple \n> +\t * addition, but as we also have to deal with shallow grafts and all it's simplest to \n> +\t * just resort to piping object refs.\n> +\t */\n> +\tif (!use_thin_pack) {\n> +\t\trev_list.proc = do_rev_list;\n> +\t\t/* .data is just a boolean: any non-NULL value will do */\n> +\t\trev_list.data = create_full_pack ? &rev_list : NULL;\n> +\t\tif (start_async(&rev_list))\n> +\t\t\tdie(\"git upload-pack: unable to fork git-rev-list\");\n> +\t\t\n> +\t\targv[arg++] = \"pack-objects\";\n> +\t} else {\n> +\t\targv[arg++] = \"pack-objects\";\n> +\t\targv[arg++] = \"--revs\";\n> +\t\targv[arg++] = \"--include-tag\";\n> +\t\tif (create_full_pack)\n> +\t\t\targv[arg++] = \"--all\";\n> +\t}\n> +\t\n> \targv[arg++] = \"--stdout\";\n> \tif (!no_progress)\n> \t\targv[arg++] = \"--progress\";\n> @@ -172,7 +186,7 @@ static void create_pack_file(void)\n> \targv[arg++] = NULL;\n> \n> \tmemset(&pack_objects, 0, sizeof(pack_objects));\n> -\tpack_objects.in = rev_list.out;\t/* start_command closes it */\n> +\tpack_objects.in = !use_thin_pack ? rev_list.out : -1;\n> \tpack_objects.out = -1;\n> \tpack_objects.err = -1;\n> \tpack_objects.git_cmd = 1;\n> @@ -181,6 +195,28 @@ static void create_pack_file(void)\n> \tif (start_command(&pack_objects))\n> \t\tdie(\"git upload-pack: unable to fork git-pack-objects\");\n> \n> +\t/* pass on revisions we (don't) want \n> +\t * (do we need to check the validity of pack_objects.in?)\n> +\t */\n> +\tif (use_thin_pack) {\n> +\t\tFILE *pipe_fd = fdopen(pack_objects.in, \"w\");\n> +\t\tif (!create_full_pack) {\n> +\t\t\tint i;\n> +\t\t\tfor (i = 0; i < want_obj.nr; i++) {\n> +\t\t\t\tfprintf(pipe_fd, \"%s\\n\", sha1_to_hex(want_obj.objects[i].item->sha1));\n> +\t\t\t}\n> +\t\t\tfprintf(pipe_fd, \"--not\\n\");\n> +\t\t\tfor (i = 0; i < have_obj.nr; i++) {\n> +\t\t\t\tfprintf(pipe_fd, \"%s\\n\", sha1_to_hex(have_obj.objects[i].item->sha1));\n> +\t\t\t}\n> +\t\t}\n> +\t\t\n> +\t\tfprintf(pipe_fd, \"\\n\");\n> +\t\tfflush(pipe_fd);\n> +\t\tfclose(pipe_fd);\n> +\t}\n> +\n> +\n> \t/* We read from pack_objects.err to capture stderr output for\n> \t * progress bar, and pack_objects.out to capture the pack data.\n> \t */\n> @@ -276,7 +312,7 @@ static void create_pack_file(void)\n> \t\terror(\"git upload-pack: git-pack-objects died with error.\");\n> \t\tgoto fail;\n> \t}\n> -\tif (finish_async(&rev_list))\n> +\tif (!use_thin_pack && finish_async(&rev_list))\n> \t\tgoto fail;\t/* error was already reported */\n> \n> \t/* flush the data */\n> \n"},{"id":"115491","messageId":"4A28D2EF.8040704@viscovery.net","threadId":"19672","inReplyTo":"20090605054500.06A9D21C3F4@mail.utsl.gen.nz","subject":"Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-06-05T08:10:23Z","receivedAt":"2009-06-05T08:10:23Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"sam@vilain.net schrieb:\n> instead of using the internal revision walker and piping object refs\n> to pack-objects this patch passes only the revs to pack-objects, which\n> in turn handles both enumeration and packing.\n\nI appreciate this move. We have one test failing in MinGW git (t5530.6)\nbecause of the rev-list that is run using start_async(). Even though this\npatch doesn't change that (the test case still uses the start_async()\npath), it is one step closer to the solution.\n\n> +\t/* sending rev params to pack-objects directly is great, but unfortunately pack-objects \n> +\t * has no way of turning off thin pack generation. this would be a relatively simple \n> +\t * addition, but as we also have to deal with shallow grafts and all it's simplest to \n> +\t * just resort to piping object refs.\n> +\t */\n\nYou certainly will reformat comments like this to shorter lines, proper\ncapitalization, without trailing spaces, and adjust the style (initial /*\nis on its own line)?\n\n> @@ -181,6 +195,28 @@ static void create_pack_file(void)\n> \tif (start_command(&pack_objects))\n> \t\tdie(\"git upload-pack: unable to fork git-pack-objects\");\n> \n> +\t/* pass on revisions we (don't) want \n> +\t * (do we need to check the validity of pack_objects.in?)\n\nNo, you don't need to check. It's valid, or you would have died above.\n\nFYI, with this patch MinGW git hangs in t5530.8; the test-case exercises\nthe new code path.\n\n-- Hannes\n"},{"id":"115540","messageId":"alpine.LFD.2.00.0906051224510.3906@xanadu.home","threadId":"19672","inReplyTo":"20090605054500.06A9D21C3F4@mail.utsl.gen.nz","subject":"Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-06-05T16:51:08Z","receivedAt":"2009-06-05T16:51:08Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 5 Jun 2009, sam@vilain.net wrote:\n\n> instead of using the internal revision walker and piping object refs\n> to pack-objects this patch passes only the revs to pack-objects, which\n> in turn handles both enumeration and packing.\n> \n> Signed-off-by: Sam Vilain \n> ---\n> Submitted on behalf of Nick in order to get wider feedback on this.\n> This version passes the test suite.\n> \n> upload-pack.c | 54 +++++++++++++++++++++++++++++++++++++++++++++---------\n> 1 files changed, 45 insertions(+), 9 deletions(-)\n> \n> diff --git a/upload-pack.c b/upload-pack.c\n> index edc7861..7eda8fd 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -155,13 +155,27 @@ static void create_pack_file(void)\n> \tconst char *argv[10];\n> \tint arg = 0;\n> \n> -\trev_list.proc = do_rev_list;\n> -\t/* .data is just a boolean: any non-NULL value will do */\n> -\trev_list.data = create_full_pack ? &rev_list : NULL;\n> -\tif (start_async(&rev_list))\n> -\t\tdie(\"git upload-pack: unable to fork git-rev-list\");\n> -\n> -\targv[arg++] = \"pack-objects\";\n> +\t/* sending rev params to pack-objects directly is great, but unfortunately pack-objects \n> +\t * has no way of turning off thin pack generation. this would be a relatively simple \n> +\t * addition, but as we also have to deal with shallow grafts and all it's simplest to \n> +\t * just resort to piping object refs.\n> +\t */\n\nWhat's that? Where did you get that?\n\nThe way to not generate a thin pack is to not specify --thin to \npack-objects. If you get a thin pack without specifying --thin then \nthis is a bug that needs to be fixed first.\n\n> +\tif (!use_thin_pack) {\n> +\t\trev_list.proc = do_rev_list;\n> +\t\t/* .data is just a boolean: any non-NULL value will do */\n> +\t\trev_list.data = create_full_pack ? &rev_list : NULL;\n> +\t\tif (start_async(&rev_list))\n> +\t\t\tdie(\"git upload-pack: unable to fork git-rev-list\");\n> +\t\t\n> +\t\targv[arg++] = \"pack-objects\";\n> +\t} else {\n> +\t\targv[arg++] = \"pack-objects\";\n> +\t\targv[arg++] = \"--revs\";\n> +\t\targv[arg++] = \"--include-tag\";\n\nShouldn't this be specified only if corresponding capability was \nprovided by the client?\n\n\nNicolas\n"},{"id":"115701","messageId":"c77435a80906070625i4daaa69bi134df765d7a77cdf@mail.gmail.com","threadId":"19672","inReplyTo":"alpine.LFD.2.00.0906051224510.3906@xanadu.home","subject":"Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects","fromName":"Nick Edelen","fromEmail":"sirnot@gmail.com","sentAt":"2009-06-07T13:25:26Z","receivedAt":"2009-06-07T13:25:26Z","isPatch":false,"sender":{"key":"sirnot@gmail.com","avatar":null},"body":"hi guys,\n\nI wasn't aware that Sam was going to upload this, otherwise I would've\ncleaned it up a bit. not a problem though; thanks for taking the time\nto do this Sam.\n\n@Johannes:\nthe real reason I'm using start_async at all is because of shallow\ncommit grafts. we could potentially add an interface to pack-objects\nto allow it to accept those types of commit grafts if you wanted.\nlike, in addition to the --not flag we could add a --shallow flag or\nsomething... it'd actually be pretty easy to implement, but I don't\nknow if you guys'd want that in pack-objects.\n\n@Nicolas:\nI'm using the --revs flag in pack-objects, which causes it to use\nget_object_list(). you'll notice, regardless of whether --thin is\nset, this function still calls\n\tmark_edges_uninteresting(revs.commits, &revs, show_edge);\nwhich sets uninteresting objects as preferred bases, which I'd think\nwould create a thin pack. I could be wrong though...\n\nas I mentioned in the comment and above, it's an easy fix, but even\nthen I wasn't sure what to do with commit grafts. as use_thin_pack\nseemed to be predominantly set on shallow interactions, I just didn't\nbother seperating the cases 'normal but thick pack' and 'shallow\nstuff'.\n\n(btw, I have a really cool idea for shallow/narrow/lazy interaction if\nyou have the time. it basically uses 'fantom' placeholder objects to\ncover for missing blobs, so a clone/fetch would get all commits/trees\nbut only retrieve blobs when a user specifies. I'll get a proof of\nconcept done after this rev-cache project).\n\nthank you both for looking over this though.\n\n - Nick\n\nOn Fri, Jun 5, 2009 at 6:51 PM, Nicolas Pitre wrote:\n> On Fri, 5 Jun 2009, sam@vilain.net wrote:\n>\n>> instead of using the internal revision walker and piping object refs\n>> to pack-objects this patch passes only the revs to pack-objects, which\n>> in turn handles both enumeration and packing.\n>>\n>> Signed-off-by: Sam Vilain \n>> ---\n>>   Submitted on behalf of Nick in order to get wider feedback on this.\n>>   This version passes the test suite.\n>>\n>>  upload-pack.c |   54 +++++++++++++++++++++++++++++++++++++++++++++---------\n>>  1 files changed, 45 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/upload-pack.c b/upload-pack.c\n>> index edc7861..7eda8fd 100644\n>> --- a/upload-pack.c\n>> +++ b/upload-pack.c\n>> @@ -155,13 +155,27 @@ static void create_pack_file(void)\n>>       const char *argv[10];\n>>       int arg = 0;\n>>\n>> -     rev_list.proc = do_rev_list;\n>> -     /* .data is just a boolean: any non-NULL value will do */\n>> -     rev_list.data = create_full_pack ? &rev_list : NULL;\n>> -     if (start_async(&rev_list))\n>> -             die(\"git upload-pack: unable to fork git-rev-list\");\n>> -\n>> -     argv[arg++] = \"pack-objects\";\n>> +     /* sending rev params to pack-objects directly is great, but unfortunately pack-objects\n>> +      * has no way of turning off thin pack generation.  this would be a relatively simple\n>> +      * addition, but as we also have to deal with shallow grafts and all it's simplest to\n>> +      * just resort to piping object refs.\n>> +      */\n>\n> What's that?  Where did you get that?\n>\n> The way to not generate a thin pack is to not specify --thin to\n> pack-objects.  If you get a thin pack without specifying --thin then\n> this is a bug that needs to be fixed first.\n>\n>> +     if (!use_thin_pack) {\n>> +             rev_list.proc = do_rev_list;\n>> +             /* .data is just a boolean: any non-NULL value will do */\n>> +             rev_list.data = create_full_pack ? &rev_list : NULL;\n>> +             if (start_async(&rev_list))\n>> +                     die(\"git upload-pack: unable to fork git-rev-list\");\n>> +\n>> +             argv[arg++] = \"pack-objects\";\n>> +     } else {\n>> +             argv[arg++] = \"pack-objects\";\n>> +             argv[arg++] = \"--revs\";\n>> +             argv[arg++] = \"--include-tag\";\n>\n> Shouldn't this be specified only if corresponding capability was\n> provided by the client?\n>\n>\n> Nicolas\n>\n"},{"id":"115702","messageId":"c77435a80906070631x5c030b9cve4c8963c0d33e71b@mail.gmail.com","threadId":"19672","inReplyTo":"c77435a80906070625i4daaa69bi134df765d7a77cdf@mail.gmail.com","subject":"Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects","fromName":"Nick Edelen","fromEmail":"sirnot@gmail.com","sentAt":"2009-06-07T13:31:57Z","receivedAt":"2009-06-07T13:31:57Z","isPatch":false,"sender":{"key":"sirnot@gmail.com","avatar":null},"body":"by the way, since I'm on the list now anyway:\n\nis there any reason why you guys are calling\nmark_parents_uninteresting() after add_parents_to_list() in\nlimit_list() (revision.c), considering the latter does everything the\nformer does? sorta trivial, but it's been bugging me...\n\nOn Sun, Jun 7, 2009 at 3:25 PM, Nick Edelen wrote:\n> hi guys,\n>\n> I wasn't aware that Sam was going to upload this, otherwise I would've\n> cleaned it up a bit.  not a problem though; thanks for taking the time\n> to do this Sam.\n>\n> @Johannes:\n> the real reason I'm using start_async at all is because of shallow\n> commit grafts.  we could potentially add an interface to pack-objects\n> to allow it to accept those types of commit grafts if you wanted.\n> like, in addition to the --not flag we could add a --shallow flag or\n> something...  it'd actually be pretty easy to implement, but I don't\n> know if you guys'd want that in pack-objects.\n>\n> @Nicolas:\n> I'm using the --revs flag in pack-objects, which causes it to use\n> get_object_list().  you'll notice, regardless of whether --thin is\n> set, this function still calls\n>        mark_edges_uninteresting(revs.commits, &revs, show_edge);\n> which sets uninteresting objects as preferred bases, which I'd think\n> would create a thin pack.  I could be wrong though...\n>\n> as I mentioned in the comment and above, it's an easy fix, but even\n> then I wasn't sure what to do with commit grafts.  as use_thin_pack\n> seemed to be predominantly set on shallow interactions, I just didn't\n> bother seperating the cases 'normal but thick pack' and 'shallow\n> stuff'.\n>\n> (btw, I have a really cool idea for shallow/narrow/lazy interaction if\n> you have the time.  it basically uses 'fantom' placeholder objects to\n> cover for missing blobs, so a clone/fetch would get all commits/trees\n> but only retrieve blobs when a user specifies.  I'll get a proof of\n> concept done after this rev-cache project).\n>\n> thank you both for looking over this though.\n>\n>  - Nick\n>\n> On Fri, Jun 5, 2009 at 6:51 PM, Nicolas Pitre wrote:\n>> On Fri, 5 Jun 2009, sam@vilain.net wrote:\n>>\n>>> instead of using the internal revision walker and piping object refs\n>>> to pack-objects this patch passes only the revs to pack-objects, which\n>>> in turn handles both enumeration and packing.\n>>>\n>>> Signed-off-by: Sam Vilain \n>>> ---\n>>>   Submitted on behalf of Nick in order to get wider feedback on this.\n>>>   This version passes the test suite.\n>>>\n>>>  upload-pack.c |   54 +++++++++++++++++++++++++++++++++++++++++++++---------\n>>>  1 files changed, 45 insertions(+), 9 deletions(-)\n>>>\n>>> diff --git a/upload-pack.c b/upload-pack.c\n>>> index edc7861..7eda8fd 100644\n>>> --- a/upload-pack.c\n>>> +++ b/upload-pack.c\n>>> @@ -155,13 +155,27 @@ static void create_pack_file(void)\n>>>       const char *argv[10];\n>>>       int arg = 0;\n>>>\n>>> -     rev_list.proc = do_rev_list;\n>>> -     /* .data is just a boolean: any non-NULL value will do */\n>>> -     rev_list.data = create_full_pack ? &rev_list : NULL;\n>>> -     if (start_async(&rev_list))\n>>> -             die(\"git upload-pack: unable to fork git-rev-list\");\n>>> -\n>>> -     argv[arg++] = \"pack-objects\";\n>>> +     /* sending rev params to pack-objects directly is great, but unfortunately pack-objects\n>>> +      * has no way of turning off thin pack generation.  this would be a relatively simple\n>>> +      * addition, but as we also have to deal with shallow grafts and all it's simplest to\n>>> +      * just resort to piping object refs.\n>>> +      */\n>>\n>> What's that?  Where did you get that?\n>>\n>> The way to not generate a thin pack is to not specify --thin to\n>> pack-objects.  If you get a thin pack without specifying --thin then\n>> this is a bug that needs to be fixed first.\n>>\n>>> +     if (!use_thin_pack) {\n>>> +             rev_list.proc = do_rev_list;\n>>> +             /* .data is just a boolean: any non-NULL value will do */\n>>> +             rev_list.data = create_full_pack ? &rev_list : NULL;\n>>> +             if (start_async(&rev_list))\n>>> +                     die(\"git upload-pack: unable to fork git-rev-list\");\n>>> +\n>>> +             argv[arg++] = \"pack-objects\";\n>>> +     } else {\n>>> +             argv[arg++] = \"pack-objects\";\n>>> +             argv[arg++] = \"--revs\";\n>>> +             argv[arg++] = \"--include-tag\";\n>>\n>> Shouldn't this be specified only if corresponding capability was\n>> provided by the client?\n>>\n>>\n>> Nicolas\n>>\n>\n"},{"id":"115712","messageId":"alpine.LFD.2.00.0906071225060.3906@xanadu.home","threadId":"19672","inReplyTo":"c77435a80906070625i4daaa69bi134df765d7a77cdf@mail.gmail.com","subject":"Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-06-07T16:41:44Z","receivedAt":"2009-06-07T16:41:44Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 7 Jun 2009, Nick Edelen wrote:\n\n> I'm using the --revs flag in pack-objects, which causes it to use\n> get_object_list(). you'll notice, regardless of whether --thin is\n> set, this function still calls\n> \tmark_edges_uninteresting(revs.commits, &revs, show_edge);\n> which sets uninteresting objects as preferred bases, which I'd think\n> would create a thin pack. I could be wrong though...\n\nLook at the arguments passed to setup_revisions().\nWhen --thin is set, the --objects-edge flag is passed instead of \n--objects. Now see what effect this has on the third argument of \nmark_edges_uninteresting().\n\n> as I mentioned in the comment and above, it's an easy fix, but even \n> then I wasn't sure what to do with commit grafts. as use_thin_pack \n> seemed to be predominantly set on shallow interactions, I just didn't \n> bother seperating the cases 'normal but thick pack' and 'shallow \n> stuff'.\n\nPlease do separate them. In theory you could use thin packs with a \nrelative deepening of a shallow clone. In other words, !thin and \nshallow is a wrong association to make.\n\n\nNicolas\n"},{"id":"115714","messageId":"c77435a80906070947u9bf8ce9m9d59f86e5a5f18ab@mail.gmail.com","threadId":"19672","inReplyTo":"alpine.LFD.2.00.0906071225060.3906@xanadu.home","subject":"Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects","fromName":"Nick Edelen","fromEmail":"sirnot@gmail.com","sentAt":"2009-06-07T16:47:33Z","receivedAt":"2009-06-07T16:47:33Z","isPatch":false,"sender":{"key":"sirnot@gmail.com","avatar":null},"body":"crap, your right. somehow I managed to miss that...\n\nI'll go ahead and seperate them then...\n\nOn Sun, Jun 7, 2009 at 6:41 PM, Nicolas Pitre wrote:\n> On Sun, 7 Jun 2009, Nick Edelen wrote:\n>\n>> I'm using the --revs flag in pack-objects, which causes it to use\n>> get_object_list().  you'll notice, regardless of whether --thin is\n>> set, this function still calls\n>>       mark_edges_uninteresting(revs.commits, &revs, show_edge);\n>> which sets uninteresting objects as preferred bases, which I'd think\n>> would create a thin pack.  I could be wrong though...\n>\n> Look at the arguments passed to setup_revisions().\n> When --thin is set, the --objects-edge flag is passed instead of\n> --objects.  Now see what effect this has on the third argument of\n> mark_edges_uninteresting().\n>\n>> as I mentioned in the comment and above, it's an easy fix, but even\n>> then I wasn't sure what to do with commit grafts.  as use_thin_pack\n>> seemed to be predominantly set on shallow interactions, I just didn't\n>> bother seperating the cases 'normal but thick pack' and 'shallow\n>> stuff'.\n>\n> Please do separate them.  In theory you could use thin packs with a\n> relative deepening of a shallow clone.  In other words, !thin and\n> shallow is a wrong association to make.\n>\n>\n> Nicolas\n>\n"},{"id":"115723","messageId":"c77435a80906071155g5530ccdel286907b7c6022838@mail.gmail.com","threadId":"19672","inReplyTo":"c77435a80906070947u9bf8ce9m9d59f86e5a5f18ab@mail.gmail.com","subject":"Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects","fromName":"Nick Edelen","fromEmail":"sirnot@gmail.com","sentAt":"2009-06-07T18:55:48Z","receivedAt":"2009-06-07T18:55:48Z","isPatch":false,"sender":{"key":"sirnot@gmail.com","avatar":null},"body":"how does this look?\n\nSigned-off-by: Nick Edelen \n\n---\n t/t5530-upload-pack-error.sh | 2 +-\n upload-pack.c | 50 +++++++++++++++++++++++++++++++++--------\n 2 files changed, 41 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\nindex f5102b9..26bcd1e 100755\n--- a/t/t5530-upload-pack-error.sh\n+++ b/t/t5530-upload-pack-error.sh\n@@ -51,7 +51,7 @@ test_expect_success 'fsck fails' '\n test_expect_success 'upload-pack fails due to error in rev-list' '\n\n \t! echo \"0032want $(git rev-parse HEAD)\n-00000009done\n+0034shallow $(git rev-parse HEAD^)00000009done\n 0000\" | git upload-pack . > /dev/null 2> output.err &&\n \tgrep \"waitpid (async) failed\" output.err\n '\ndiff --git a/upload-pack.c b/upload-pack.c\nindex edc7861..c8f2dca 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -29,6 +29,7 @@ static unsigned long oldest_have;\n static int multi_ack, nr_our_refs;\n static int use_thin_pack, use_ofs_delta, use_include_tag;\n static int no_progress;\n+static int shallow_nr;\n static struct object_array have_obj;\n static struct object_array want_obj;\n static unsigned int timeout;\n@@ -107,8 +108,6 @@ static int do_rev_list(int fd, void *create_full_pack)\n \tstruct rev_info revs;\n\n \tpack_pipe = fdopen(fd, \"w\");\n-\tif (create_full_pack)\n-\t\tuse_thin_pack = 0; /* no point doing it */\n \tinit_revisions(&revs, NULL);\n \trevs.tag_objects = 1;\n \trevs.tree_objects = 1;\n@@ -155,13 +154,22 @@ static void create_pack_file(void)\n \tconst char *argv[10];\n \tint arg = 0;\n\n-\trev_list.proc = do_rev_list;\n-\t/* .data is just a boolean: any non-NULL value will do */\n-\trev_list.data = create_full_pack ? &rev_list : NULL;\n-\tif (start_async(&rev_list))\n-\t\tdie(\"git upload-pack: unable to fork git-rev-list\");\n+\tif (shallow_nr) {\n+\t\trev_list.proc = do_rev_list;\n+\t\trev_list.data = 0;\n+\t\tif (start_async(&rev_list))\n+\t\t\tdie(\"git upload-pack: unable to fork git-rev-list\");\n+\t\targv[arg++] = \"pack-objects\";\n+\t} else {\n+\t\targv[arg++] = \"pack-objects\";\n+\t\targv[arg++] = \"--revs\";\n+\t\targv[arg++] = \"--include-tag\";\n+\t\tif (create_full_pack)\n+\t\t\targv[arg++] = \"--all\";\n+\t\tif (use_thin_pack)\n+\t\t\targv[arg++] = \"--thin\";\n+\t}\n\n-\targv[arg++] = \"pack-objects\";\n \targv[arg++] = \"--stdout\";\n \tif (!no_progress)\n \t\targv[arg++] = \"--progress\";\n@@ -172,7 +180,7 @@ static void create_pack_file(void)\n \targv[arg++] = NULL;\n\n \tmemset(&pack_objects, 0, sizeof(pack_objects));\n-\tpack_objects.in = rev_list.out;\t/* start_command closes it */\n+\tpack_objects.in = shallow_nr ? rev_list.out : -1;\n \tpack_objects.out = -1;\n \tpack_objects.err = -1;\n \tpack_objects.git_cmd = 1;\n@@ -181,6 +189,24 @@ static void create_pack_file(void)\n \tif (start_command(&pack_objects))\n \t\tdie(\"git upload-pack: unable to fork git-pack-objects\");\n\n+\t/* pass on revisions we (don't) want */\n+\tif (!shallow_nr) {\n+\t\tFILE *pipe_fd = fdopen(pack_objects.in, \"w\");\n+\t\tif (!create_full_pack) {\n+\t\t\tint i;\n+\t\t\tfor (i = 0; i < want_obj.nr; i++)\n+\t\t\t\tfprintf(pipe_fd, \"%s\\n\", sha1_to_hex(want_obj.objects[i].item->sha1));\n+\t\t\tfprintf(pipe_fd, \"--not\\n\");\n+\t\t\tfor (i = 0; i < have_obj.nr; i++)\n+\t\t\t\tfprintf(pipe_fd, \"%s\\n\", sha1_to_hex(have_obj.objects[i].item->sha1));\n+\t\t}\n+\n+\t\tfprintf(pipe_fd, \"\\n\");\n+\t\tfflush(pipe_fd);\n+\t\tfclose(pipe_fd);\n+\t}\n+\n+\n \t/* We read from pack_objects.err to capture stderr output for\n \t * progress bar, and pack_objects.out to capture the pack data.\n \t */\n@@ -276,7 +302,7 @@ static void create_pack_file(void)\n \t\terror(\"git upload-pack: git-pack-objects died with error.\");\n \t\tgoto fail;\n \t}\n-\tif (finish_async(&rev_list))\n+\tif (shallow_nr && finish_async(&rev_list))\n \t\tgoto fail;\t/* error was already reported */\n\n \t/* flush the data */\n@@ -451,6 +477,7 @@ static void receive_needs(void)\n \tstatic char line[1000];\n \tint len, depth = 0;\n\n+\tshallow_nr = 0;\n \tif (debug_fd)\n \t\twrite_in_full(debug_fd, \"#S\\n\", 3);\n \tfor (;;) {\n@@ -534,6 +561,7 @@ static void receive_needs(void)\n \t\t\t\tpacket_write(1, \"shallow %s\",\n \t\t\t\t\t\tsha1_to_hex(object->sha1));\n \t\t\t\tregister_shallow(object->sha1);\n+\t\t\t\tshallow_nr++;\n \t\t\t}\n \t\t\tresult = result->next;\n \t\t}\n@@ -567,6 +595,8 @@ static void receive_needs(void)\n \t\t\tfor (i = 0; i < shallows.nr; i++)\n \t\t\t\tregister_shallow(shallows.objects[i].item->sha1);\n \t\t}\n+\n+\tshallow_nr += shallows.nr;\n \tfree(shallows.objects);\n }\n\n-- \ntg: (503f464..) t/revcache/upload-pack-dont-enumerate (depends on: master)\n\n\nOn Sun, Jun 7, 2009 at 6:47 PM, Nick Edelen wrote:\n> crap, your right.  somehow I managed to miss that...\n>\n> I'll go ahead and seperate them then...\n>\n> On Sun, Jun 7, 2009 at 6:41 PM, Nicolas Pitre wrote:\n>> On Sun, 7 Jun 2009, Nick Edelen wrote:\n>>\n>>> I'm using the --revs flag in pack-objects, which causes it to use\n>>> get_object_list().  you'll notice, regardless of whether --thin is\n>>> set, this function still calls\n>>>       mark_edges_uninteresting(revs.commits, &revs, show_edge);\n>>> which sets uninteresting objects as preferred bases, which I'd think\n>>> would create a thin pack.  I could be wrong though...\n>>\n>> Look at the arguments passed to setup_revisions().\n>> When --thin is set, the --objects-edge flag is passed instead of\n>> --objects.  Now see what effect this has on the third argument of\n>> mark_edges_uninteresting().\n>>\n>>> as I mentioned in the comment and above, it's an easy fix, but even\n>>> then I wasn't sure what to do with commit grafts.  as use_thin_pack\n>>> seemed to be predominantly set on shallow interactions, I just didn't\n>>> bother seperating the cases 'normal but thick pack' and 'shallow\n>>> stuff'.\n>>\n>> Please do separate them.  In theory you could use thin packs with a\n>> relative deepening of a shallow clone.  In other words, !thin and\n>> shallow is a wrong association to make.\n>>\n>>\n>> Nicolas\n>>\n>\n"},{"id":"115734","messageId":"1244407713.5870.7.camel@maia.lan","threadId":"19672","inReplyTo":"c77435a80906071155g5530ccdel286907b7c6022838@mail.gmail.com","subject":"Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects","fromName":"Sam Vilain","fromEmail":"sam@vilain.net","sentAt":"2009-06-07T20:48:33Z","receivedAt":"2009-06-07T20:48:33Z","isPatch":false,"sender":{"key":"sam@vilain.net","avatar":"https://gravatar.com/avatar/8fc840ca854dbf6f7065b4335e3b934951c1dca3b11db688e95e471901f8f4a8?d=mp&s=160"},"body":"On Sun, 2009-06-07 at 20:55 +0200, Nick Edelen wrote:\n> how does this look?\n> \n> Signed-off-by: Nick Edelen \n> \n> ---\n\nComments like that belong after the '---'; the change description is\npart of the work that you are developing, too.\n\nSam.\n"},{"id":"115735","messageId":"alpine.LFD.2.00.0906071637530.3906@xanadu.home","threadId":"19672","inReplyTo":"c77435a80906071155g5530ccdel286907b7c6022838@mail.gmail.com","subject":"Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-06-07T20:48:48Z","receivedAt":"2009-06-07T20:48:48Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 7 Jun 2009, Nick Edelen wrote:\n\n> how does this look?\n\nComments below.\n\n> Signed-off-by: Nick Edelen \n> \n> ---\n> t/t5530-upload-pack-error.sh | 2 +-\n> upload-pack.c | 50 +++++++++++++++++++++++++++++++++--------\n> 2 files changed, 41 insertions(+), 11 deletions(-)\n> \n> diff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\n> index f5102b9..26bcd1e 100755\n> --- a/t/t5530-upload-pack-error.sh\n> +++ b/t/t5530-upload-pack-error.sh\n> @@ -51,7 +51,7 @@ test_expect_success 'fsck fails' '\n> test_expect_success 'upload-pack fails due to error in rev-list' '\n> \n> \t! echo \"0032want $(git rev-parse HEAD)\n> -00000009done\n> +0034shallow $(git rev-parse HEAD^)00000009done\n\nWhy did you modify this?\n\n> 0000\" | git upload-pack . > /dev/null 2> output.err &&\n> \tgrep \"waitpid (async) failed\" output.err\n> '\n> diff --git a/upload-pack.c b/upload-pack.c\n> index edc7861..c8f2dca 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -29,6 +29,7 @@ static unsigned long oldest_have;\n> static int multi_ack, nr_our_refs;\n> static int use_thin_pack, use_ofs_delta, use_include_tag;\n> static int no_progress;\n> +static int shallow_nr;\n> static struct object_array have_obj;\n> static struct object_array want_obj;\n> static unsigned int timeout;\n> @@ -107,8 +108,6 @@ static int do_rev_list(int fd, void *create_full_pack)\n> \tstruct rev_info revs;\n> \n> \tpack_pipe = fdopen(fd, \"w\");\n> -\tif (create_full_pack)\n> -\t\tuse_thin_pack = 0; /* no point doing it */\n> \tinit_revisions(&revs, NULL);\n> \trevs.tag_objects = 1;\n> \trevs.tree_objects = 1;\n> @@ -155,13 +154,22 @@ static void create_pack_file(void)\n> \tconst char *argv[10];\n> \tint arg = 0;\n> \n> -\trev_list.proc = do_rev_list;\n> -\t/* .data is just a boolean: any non-NULL value will do */\n> -\trev_list.data = create_full_pack ? &rev_list : NULL;\n\nI'm glad you got rid of that.\n\n> -\tif (start_async(&rev_list))\n> -\t\tdie(\"git upload-pack: unable to fork git-rev-list\");\n> +\tif (shallow_nr) {\n> +\t\trev_list.proc = do_rev_list;\n> +\t\trev_list.data = 0;\n> +\t\tif (start_async(&rev_list))\n> +\t\t\tdie(\"git upload-pack: unable to fork git-rev-list\");\n> +\t\targv[arg++] = \"pack-objects\";\n> +\t} else {\n> +\t\targv[arg++] = \"pack-objects\";\n> +\t\targv[arg++] = \"--revs\";\n> +\t\targv[arg++] = \"--include-tag\";\n\nWhy this unconditional --include-tags here? Isn't it handled already a \ncouple lines down already?\n\n> +\t\tif (create_full_pack)\n> +\t\t\targv[arg++] = \"--all\";\n> +\t\tif (use_thin_pack)\n> +\t\t\targv[arg++] = \"--thin\";\n\nPlease turn this \"if (use_thin_pack)\" into an \"else if (use_thin_pack)\" \ninstead. No point using --thin for a full pack.\n\nThe rest looks fine to me.\n\n\nNicolas\n"},{"id":"115745","messageId":"c77435a80906071504u16a46e03w6dfde9a3e694a5ec@mail.gmail.com","threadId":"19672","inReplyTo":"alpine.LFD.2.00.0906071637530.3906@xanadu.home","subject":"Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects","fromName":"Nick Edelen","fromEmail":"sirnot@gmail.com","sentAt":"2009-06-07T22:04:57Z","receivedAt":"2009-06-07T22:04:57Z","isPatch":false,"sender":{"key":"sirnot@gmail.com","avatar":null},"body":"man I can sure be blind at times... alright fixed the latter two\ncomments. I changed the test file because that particular test wanted\nupload-pack to fail through the revision walker, which it could only\nnow do if shallow objects were involved.\n\n@sam: oops, sorry about that.\n\nFrom: Nick Edelen \nSubject: [PATCH] Shift object enumeration out of upload-pack\n\nOffload object enumeration in upload-pack to pack-objects, but fall\nback on internal revision walker for shallow interaction. Test t5530\nupdated to reflect mechanism change.\n\nSigned-off-by: Nick Edelen \n\n---\n t/t5530-upload-pack-error.sh | 2 +-\n upload-pack.c | 49 +++++++++++++++++++++++++++++++++--------\n 2 files changed, 40 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\nindex f5102b9..26bcd1e 100755\n--- a/t/t5530-upload-pack-error.sh\n+++ b/t/t5530-upload-pack-error.sh\n@@ -51,7 +51,7 @@ test_expect_success 'fsck fails' '\n test_expect_success 'upload-pack fails due to error in rev-list' '\n\n \t! echo \"0032want $(git rev-parse HEAD)\n-00000009done\n+0034shallow $(git rev-parse HEAD^)00000009done\n 0000\" | git upload-pack . > /dev/null 2> output.err &&\n \tgrep \"waitpid (async) failed\" output.err\n '\ndiff --git a/upload-pack.c b/upload-pack.c\nindex edc7861..397cada 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -29,6 +29,7 @@ static unsigned long oldest_have;\n static int multi_ack, nr_our_refs;\n static int use_thin_pack, use_ofs_delta, use_include_tag;\n static int no_progress;\n+static int shallow_nr;\n static struct object_array have_obj;\n static struct object_array want_obj;\n static unsigned int timeout;\n@@ -107,8 +108,6 @@ static int do_rev_list(int fd, void *create_full_pack)\n \tstruct rev_info revs;\n\n \tpack_pipe = fdopen(fd, \"w\");\n-\tif (create_full_pack)\n-\t\tuse_thin_pack = 0; /* no point doing it */\n \tinit_revisions(&revs, NULL);\n \trevs.tag_objects = 1;\n \trevs.tree_objects = 1;\n@@ -155,13 +154,21 @@ static void create_pack_file(void)\n \tconst char *argv[10];\n \tint arg = 0;\n\n-\trev_list.proc = do_rev_list;\n-\t/* .data is just a boolean: any non-NULL value will do */\n-\trev_list.data = create_full_pack ? &rev_list : NULL;\n-\tif (start_async(&rev_list))\n-\t\tdie(\"git upload-pack: unable to fork git-rev-list\");\n+\tif (shallow_nr) {\n+\t\trev_list.proc = do_rev_list;\n+\t\trev_list.data = 0;\n+\t\tif (start_async(&rev_list))\n+\t\t\tdie(\"git upload-pack: unable to fork git-rev-list\");\n+\t\targv[arg++] = \"pack-objects\";\n+\t} else {\n+\t\targv[arg++] = \"pack-objects\";\n+\t\targv[arg++] = \"--revs\";\n+\t\tif (create_full_pack)\n+\t\t\targv[arg++] = \"--all\";\n+\t\telse if (use_thin_pack)\n+\t\t\targv[arg++] = \"--thin\";\n+\t}\n\n-\targv[arg++] = \"pack-objects\";\n \targv[arg++] = \"--stdout\";\n \tif (!no_progress)\n \t\targv[arg++] = \"--progress\";\n@@ -172,7 +179,7 @@ static void create_pack_file(void)\n \targv[arg++] = NULL;\n\n \tmemset(&pack_objects, 0, sizeof(pack_objects));\n-\tpack_objects.in = rev_list.out;\t/* start_command closes it */\n+\tpack_objects.in = shallow_nr ? rev_list.out : -1;\n \tpack_objects.out = -1;\n \tpack_objects.err = -1;\n \tpack_objects.git_cmd = 1;\n@@ -181,6 +188,24 @@ static void create_pack_file(void)\n \tif (start_command(&pack_objects))\n \t\tdie(\"git upload-pack: unable to fork git-pack-objects\");\n\n+\t/* pass on revisions we (don't) want */\n+\tif (!shallow_nr) {\n+\t\tFILE *pipe_fd = fdopen(pack_objects.in, \"w\");\n+\t\tif (!create_full_pack) {\n+\t\t\tint i;\n+\t\t\tfor (i = 0; i < want_obj.nr; i++)\n+\t\t\t\tfprintf(pipe_fd, \"%s\\n\", sha1_to_hex(want_obj.objects[i].item->sha1));\n+\t\t\tfprintf(pipe_fd, \"--not\\n\");\n+\t\t\tfor (i = 0; i < have_obj.nr; i++)\n+\t\t\t\tfprintf(pipe_fd, \"%s\\n\", sha1_to_hex(have_obj.objects[i].item->sha1));\n+\t\t}\n+\n+\t\tfprintf(pipe_fd, \"\\n\");\n+\t\tfflush(pipe_fd);\n+\t\tfclose(pipe_fd);\n+\t}\n+\n+\n \t/* We read from pack_objects.err to capture stderr output for\n \t * progress bar, and pack_objects.out to capture the pack data.\n \t */\n@@ -276,7 +301,7 @@ static void create_pack_file(void)\n \t\terror(\"git upload-pack: git-pack-objects died with error.\");\n \t\tgoto fail;\n \t}\n-\tif (finish_async(&rev_list))\n+\tif (shallow_nr && finish_async(&rev_list))\n \t\tgoto fail;\t/* error was already reported */\n\n \t/* flush the data */\n@@ -451,6 +476,7 @@ static void receive_needs(void)\n \tstatic char line[1000];\n \tint len, depth = 0;\n\n+\tshallow_nr = 0;\n \tif (debug_fd)\n \t\twrite_in_full(debug_fd, \"#S\\n\", 3);\n \tfor (;;) {\n@@ -534,6 +560,7 @@ static void receive_needs(void)\n \t\t\t\tpacket_write(1, \"shallow %s\",\n \t\t\t\t\t\tsha1_to_hex(object->sha1));\n \t\t\t\tregister_shallow(object->sha1);\n+\t\t\t\tshallow_nr++;\n \t\t\t}\n \t\t\tresult = result->next;\n \t\t}\n@@ -567,6 +594,8 @@ static void receive_needs(void)\n \t\t\tfor (i = 0; i < shallows.nr; i++)\n \t\t\t\tregister_shallow(shallows.objects[i].item->sha1);\n \t\t}\n+\n+\tshallow_nr += shallows.nr;\n \tfree(shallows.objects);\n }\n\n-- \ntg: (503f464..) t/revcache/upload-pack-dont-enumerate (depends on: master)\n\n\nOn Sun, Jun 7, 2009 at 10:48 PM, Nicolas Pitre wrote:\n> On Sun, 7 Jun 2009, Nick Edelen wrote:\n>\n>> how does this look?\n>\n> Comments below.\n>\n>> Signed-off-by: Nick Edelen \n>>\n>> ---\n>>  t/t5530-upload-pack-error.sh |    2 +-\n>>  upload-pack.c                |   50 +++++++++++++++++++++++++++++++++--------\n>>  2 files changed, 41 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\n>> index f5102b9..26bcd1e 100755\n>> --- a/t/t5530-upload-pack-error.sh\n>> +++ b/t/t5530-upload-pack-error.sh\n>> @@ -51,7 +51,7 @@ test_expect_success 'fsck fails' '\n>>  test_expect_success 'upload-pack fails due to error in rev-list' '\n>>\n>>       ! echo \"0032want $(git rev-parse HEAD)\n>> -00000009done\n>> +0034shallow $(git rev-parse HEAD^)00000009done\n>\n> Why did you modify this?\n>\n>>  0000\" | git upload-pack . > /dev/null 2> output.err &&\n>>       grep \"waitpid (async) failed\" output.err\n>>  '\n>> diff --git a/upload-pack.c b/upload-pack.c\n>> index edc7861..c8f2dca 100644\n>> --- a/upload-pack.c\n>> +++ b/upload-pack.c\n>> @@ -29,6 +29,7 @@ static unsigned long oldest_have;\n>>  static int multi_ack, nr_our_refs;\n>>  static int use_thin_pack, use_ofs_delta, use_include_tag;\n>>  static int no_progress;\n>> +static int shallow_nr;\n>>  static struct object_array have_obj;\n>>  static struct object_array want_obj;\n>>  static unsigned int timeout;\n>> @@ -107,8 +108,6 @@ static int do_rev_list(int fd, void *create_full_pack)\n>>       struct rev_info revs;\n>>\n>>       pack_pipe = fdopen(fd, \"w\");\n>> -     if (create_full_pack)\n>> -             use_thin_pack = 0; /* no point doing it */\n>>       init_revisions(&revs, NULL);\n>>       revs.tag_objects = 1;\n>>       revs.tree_objects = 1;\n>> @@ -155,13 +154,22 @@ static void create_pack_file(void)\n>>       const char *argv[10];\n>>       int arg = 0;\n>>\n>> -     rev_list.proc = do_rev_list;\n>> -     /* .data is just a boolean: any non-NULL value will do */\n>> -     rev_list.data = create_full_pack ? &rev_list : NULL;\n>\n> I'm glad you got rid of that.\n>\n>> -     if (start_async(&rev_list))\n>> -             die(\"git upload-pack: unable to fork git-rev-list\");\n>> +     if (shallow_nr) {\n>> +             rev_list.proc = do_rev_list;\n>> +             rev_list.data = 0;\n>> +             if (start_async(&rev_list))\n>> +                     die(\"git upload-pack: unable to fork git-rev-list\");\n>> +             argv[arg++] = \"pack-objects\";\n>> +     } else {\n>> +             argv[arg++] = \"pack-objects\";\n>> +             argv[arg++] = \"--revs\";\n>> +             argv[arg++] = \"--include-tag\";\n>\n> Why this unconditional --include-tags here?  Isn't it handled already a\n> couple lines down already?\n>\n>> +             if (create_full_pack)\n>> +                     argv[arg++] = \"--all\";\n>> +             if (use_thin_pack)\n>> +                     argv[arg++] = \"--thin\";\n>\n> Please turn this \"if (use_thin_pack)\" into an \"else if (use_thin_pack)\"\n> instead.  No point using --thin for a full pack.\n>\n> The rest looks fine to me.\n>\n>\n> Nicolas\n>\n"},{"id":"115758","messageId":"alpine.LFD.2.00.0906072043400.3906@xanadu.home","threadId":"19672","inReplyTo":"c77435a80906071504u16a46e03w6dfde9a3e694a5ec@mail.gmail.com","subject":"Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-06-08T00:50:24Z","receivedAt":"2009-06-08T00:50:24Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 8 Jun 2009, Nick Edelen wrote:\n\n> man I can sure be blind at times... alright fixed the latter two\n> comments. I changed the test file because that particular test wanted\n> upload-pack to fail through the revision walker, which it could only\n> now do if shallow objects were involved.\n\nOK. However, since now we have 2 different paths for creating object \nlist instead of only one (the existing one in upload-pack and the one in \npack-objects), I'd prefer if you could add another test case to make \nsure the pack-objects path also fails appropriately.\n\nThe rest looks fine to me, and with the test issue fixed you could add \nmy:\n\n Acked-by: Nicolas Pitre writes:\n\n> On Mon, 8 Jun 2009, Nick Edelen wrote:\n>\n>> man I can sure be blind at times... alright fixed the latter two\n>> comments. I changed the test file because that particular test wanted\n>> upload-pack to fail through the revision walker, which it could only\n>> now do if shallow objects were involved.\n>\n> OK. However, since now we have 2 different paths for creating object \n> list instead of only one (the existing one in upload-pack and the one in \n> pack-objects), I'd prefer if you could add another test case to make \n> sure the pack-objects path also fails appropriately.\n>\n> The rest looks fine to me, and with the test issue fixed you could add \n> my:\n>\n> Acked-by: Nicolas Pitre \n> to your next submission. Oh and you might change the subject prefix to \n> [PATCH] as well since I think this won't be WIP anymore.\n\nTrue and true. Thanks.\n"},{"id":"115780","messageId":"4A2CD10A.3000703@viscovery.net","threadId":"19672","inReplyTo":"4A28D2EF.8040704@viscovery.net","subject":"[PATCH] fetch-pack: close output channel after sideband demultiplexer terminates","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-06-08T08:51:22Z","receivedAt":"2009-06-08T08:51:22Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Johannes Sixt \n\nfetch-pack runs the sideband demultiplexer using start_async(). This\nfacility requires that the asynchronously executed function closes the\noutput file descriptor (see Documentation/technical/api-run-command.txt).\nBut the sideband demultiplexer did not do that. This fixes it.\n\nIn certain error situations this could lock up a fetch operation on\nWindows because the asynchronous function is run in a thread; by not\nclosing the output fd the reading end never got EOF and waited for more\ndata indefinitely. On Unix this is not a problem because the asynchronous\nfunction is run in a separate process, which exits after the function ends\nand so implicitly closes the output.\n\nSince the pack that is sent over the wire encodes the number of objects in\nthe stream, during normal operation the reading end knows when the stream\nends and terminates by itself, and does not lock up.\n\nSigned-off-by: Johannes Sixt \n---\n\n Johannes Sixt schrieb:\n > FYI, with this patch MinGW git hangs in t5530.8; the test-case exercises\n > the new code path.\n\n This fixes the lockup.\n\n -- Hannes\n\n builtin-fetch-pack.c | 4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c\nindex 2eb7a22..b11d799 100644\n--- a/builtin-fetch-pack.c\n+++ b/builtin-fetch-pack.c\n@@ -487,7 +487,9 @@ static int sideband_demux(int fd, void *data)\n {\n \tint *xd = data;\n\n-\treturn recv_sideband(\"fetch-pack\", xd[0], fd);\n+\tint ret = recv_sideband(\"fetch-pack\", xd[0], fd);\n+\tclose(fd);\n+\treturn ret;\n }\n\n static int get_pack(int xd[2], char **pack_lockfile)\n-- \n1.6.3.1.1141.gcf6b0.dirty\n"}]}