{"thread":{"id":"5486","subject":"send-pack: limit on negative references","startedAt":"2006-09-05T12:45:03Z","lastAt":"2006-09-06T04:46:50Z","messageCount":8,"participants":["Andy Whitcroft","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"26359","messageId":"44FD714F.9040003@shadowen.org","threadId":"5486","inReplyTo":null,"subject":"send-pack: limit on negative references","fromName":"Andy Whitcroft","fromEmail":"apw@shadowen.org","sentAt":"2006-09-05T12:45:03Z","receivedAt":"2006-09-05T12:45:03Z","isPatch":false,"sender":{"key":"apw@shadowen.org","avatar":"https://gravatar.com/avatar/d3088262854661a913ef35cc40fedcc270142d4461791142bc1ea0b2a4e2e147?d=mp&s=160"},"body":"I've been having trouble with git push apparently resending the entire\ncommit trace for the branch each and every time I push; even with short\nbranch names.  This seems to be related to the changes made to handle\nthe case where the remote end has a large number of branches (>900).\n\nThe problem there seemed to be that we would fill list-revs' argument\nlist in reference order with negative and positive limits.  This could\nlead to us to exceed the limit 900 argument limit and bomb.  The changes\nmade in commit 797656e58ddbd82ac461a5142ed726db3a4d0ac0 split the\nparameter loading into two phases.  First we load the +ve and -ve\nreferences for the branches we are definatly trying to send; bailing if\nwe cannot fit those references onto the command line.  Second we load up\nthe remaining remote references as -ve references to limit duplicate\nobject sends.\n\nThis second phase loads up to 16 additional references onto the command\nline.  Now this effectively limits you to 16 branchs in your remote\nrepository if you wish to ensure you catch a parental head to copy from.\n This can lead us to send the entire commit stream for the branch even\nthough the branch may contain no new commits.\n\nIt seems to me that any and all negative references we have are helpful\nand as such we should be loading as many as we can into the list.  If we\ncannot fit them all we should _not_ error out, but we should not limit\nourselves to any less than as many as we can fit.\n\nCan anyone see a downside to the patch below?\n\n-apw\n\n\nsend-pack: supply as many negative references as we can fit\n\nIn commit 797656e58ddbd82ac461a5142ed726db3a4d0ac0 list-revs' command\nline was reordered to ensure the branches we are pushing are first on\nthe list, and then a subset of the remaining remote references are\npassed to limit our push.  Given that we now have the needed positive\nreferences on the list first, it seems reasonable to load as many\nnegative references onto the list as will fit without error.\n\nSigned-off-by: Andy Whitcroft <andyw@uk.ibm.com>\n---\ndiff --git a/send-pack.c b/send-pack.c\nindex 10bc8bc..5d55241 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -40,7 +40,7 @@ static void exec_rev_list(struct ref *re\n {\n \tstruct ref *ref;\n \tstatic const char *args[1000];\n-\tint i = 0, j;\n+\tint i = 0;\n \n \targs[i++] = \"rev-list\";\t/* 0 */\n \tif (use_thin_pack)\t/* 1 */\n@@ -69,8 +69,8 @@ static void exec_rev_list(struct ref *re\n \t/* Then a handful of the remainder\n \t * NEEDSWORK: we would be better off if used the newer ones first.\n \t */\n-\tfor (ref = refs, j = i + 16;\n-\t     i < 900 && i < j && ref;\n+\tfor (ref = refs;\n+\t     i < 900 && ref;\n \t     ref = ref->next) {\n \t\tif (is_zero_sha1(ref->new_sha1) &&\n \t\t    !is_zero_sha1(ref->old_sha1) &&\n"},{"id":"26362","messageId":"7vwt8igu4u.fsf@assigned-by-dhcp.cox.net","threadId":"5486","inReplyTo":"44FD714F.9040003@shadowen.org","subject":"Re: send-pack: limit on negative references","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-05T16:23:13Z","receivedAt":"2006-09-05T16:23:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Whitcroft <apw@shadowen.org> writes:\n\n> I've been having trouble with git push apparently resending the entire\n> commit trace for the branch each and every time I push; even with short\n> branch names.  This seems to be related to the changes made to handle\n> the case where the remote end has a large number of branches (>900).\n\nI think the right fix is to do one or both of the following, and\nlift that 900 cut-off entirely.\n\nOne is to teach rev-list to read the information it is taking\nfrom the command line instead from its standard input.\n\nAnother is to teach pack-object the same trick on top of my\npatch last night.  This has an added benefit that we save one\npipe+fork+exec there.\n\nThese are essentially suggestions from Linus made twice\nseparately in the past, so they must be on the right track ;-).\n\nIf nobody does, I would do it myself, but the list is welcome to\nbeat me to it.  Especially, the former (giving --stdin option to\nrev-list) should be trivial.\n"},{"id":"26373","messageId":"44FDECD1.2090909@shadowen.org","threadId":"5486","inReplyTo":"7vwt8igu4u.fsf@assigned-by-dhcp.cox.net","subject":"Re: send-pack: limit on negative references","fromName":"Andy Whitcroft","fromEmail":"apw@shadowen.org","sentAt":"2006-09-05T21:32:01Z","receivedAt":"2006-09-05T21:32:01Z","isPatch":false,"sender":{"key":"apw@shadowen.org","avatar":"https://gravatar.com/avatar/d3088262854661a913ef35cc40fedcc270142d4461791142bc1ea0b2a4e2e147?d=mp&s=160"},"body":"Junio C Hamano wrote:\n> Andy Whitcroft <apw@shadowen.org> writes:\n> \n>> I've been having trouble with git push apparently resending the entire\n>> commit trace for the branch each and every time I push; even with short\n>> branch names.  This seems to be related to the changes made to handle\n>> the case where the remote end has a large number of branches (>900).\n> \n> I think the right fix is to do one or both of the following, and\n> lift that 900 cut-off entirely.\n> \n> One is to teach rev-list to read the information it is taking\n> from the command line instead from its standard input.\n> \n> Another is to teach pack-object the same trick on top of my\n> patch last night.  This has an added benefit that we save one\n> pipe+fork+exec there.\n> \n> These are essentially suggestions from Linus made twice\n> separately in the past, so they must be on the right track ;-).\n> \n> If nobody does, I would do it myself, but the list is welcome to\n> beat me to it.  Especially, the former (giving --stdin option to\n> rev-list) should be trivial.\n\nOk, I'll have a go at this one.  As you say it looks pretty simple to\nreuse the command line revision parser on an incoming stream.\n\n-apw\n"},{"id":"26374","messageId":"20060905215157.GA29172@shadowen.org","threadId":"5486","inReplyTo":"44FDECD1.2090909@shadowen.org","subject":"[PATCH 1/2] rev list add option accepting revision constraints on standard input","fromName":"Andy Whitcroft","fromEmail":"apw@shadowen.org","sentAt":"2006-09-05T21:51:57Z","receivedAt":"2006-09-05T21:51:57Z","isPatch":true,"sender":{"key":"apw@shadowen.org","avatar":"https://gravatar.com/avatar/d3088262854661a913ef35cc40fedcc270142d4461791142bc1ea0b2a4e2e147?d=mp&s=160"},"body":"rev-list: add option accepting revision constraints on standard input\n\nWhen we are generating packs to update remote repositories we\nwant to supply as much information as possible about the revisions\nthat already exist to rev-list in order optimise the pack as much\nas possible.  We need to pass two revisions for each branch we are\nupdating in the remote repository and one for each additional branch.\nWhere the remote repository has numerous branches we can run out\nof command line space to pass them.\n\nAdd a --stdin flag which causes rev-list to additionally read\nits stdin stream and parse that for revision constraints.\n\nSigned-off-by: Andy Whitcroft <apw@shadowen.org>\n---\ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex 8437454..0303909 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -304,10 +304,30 @@ static void mark_edges_uninteresting(str\n \t}\n }\n \n+/*\n+ * Parse revision information, filling in the \"rev_info\" structure,\n+ * revisions are taken from stream.\n+ */\n+static void setup_revisions_stream(FILE *stream, struct rev_info *revs)\n+{\n+\tchar line[1000];\n+\tconst char *args[] = { 0, line, 0 };\n+\n+\twhile (fgets(line, sizeof(line), stream) != NULL) {\n+\t\tline[strlen(line) - 1] = 0;\n+\n+\t\tif (line[0] == '-')\n+\t\t\tdie(\"options not supported in --stdin mode\");\n+\n+\t\t(void)setup_revisions(2, args, revs, NULL);\n+\t}\n+}\n+\n int cmd_rev_list(int argc, const char **argv, const char *prefix)\n {\n \tstruct commit_list *list;\n \tint i;\n+\tint read_stdin = 0;\n \n \tinit_revisions(&revs, prefix);\n \trevs.abbrev = 0;\n@@ -329,9 +349,15 @@ int cmd_rev_list(int argc, const char **\n \t\t\tbisect_list = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"--stdin\")) {\n+\t\t\tread_stdin = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tusage(rev_list_usage);\n-\n \t}\n+\tif (read_stdin)\n+\t\tsetup_revisions_stream(stdin, &revs);\n+\n \tif (revs.commit_format != CMIT_FMT_UNSPECIFIED) {\n \t\t/* The command line has a --pretty  */\n \t\thdr_termination = '\\n';\n"},{"id":"26375","messageId":"20060905215212.GA29280@shadowen.org","threadId":"5486","inReplyTo":"44FDECD1.2090909@shadowen.org","subject":"[PATCH 2/2] send pack switch to using git rev list stdin","fromName":"Andy Whitcroft","fromEmail":"apw@shadowen.org","sentAt":"2006-09-05T21:52:12Z","receivedAt":"2006-09-05T21:52:12Z","isPatch":true,"sender":{"key":"apw@shadowen.org","avatar":"https://gravatar.com/avatar/d3088262854661a913ef35cc40fedcc270142d4461791142bc1ea0b2a4e2e147?d=mp&s=160"},"body":"send-pack: switch to using git-rev-list --stdin\n\nWhen we are generating packs to update remote repositories we\nwant to supply as much information as possible about the revisions\nthat already exist to rev-list in order optimise the pack as much\nas possible.  We need to pass two revisions for each branch we are\nupdating in the remote repository and one for each additional branch.\nWhere the remote repository has numerous branches we can run out\nof command line space to pass them.\n\nUtilise the git-rev-list --stdin mode to allow unlimited numbers\nof revision constraints.  This allows us to move back to the much\nsimpler unordered revision selection code.\n\nSigned-off-by: Andy Whitcroft <apw@shadowen.org>\n---\ndiff --git a/send-pack.c b/send-pack.c\nindex ac4501d..f6e5eed 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -38,9 +38,8 @@ static void exec_pack_objects(void)\n \n static void exec_rev_list(struct ref *refs)\n {\n-\tstruct ref *ref;\n-\tstatic const char *args[1000];\n-\tint i = 0, j;\n+\tstatic const char *args[4];\n+\tint i = 0;\n \n \targs[i++] = \"rev-list\";\t/* 0 */\n \tif (use_thin_pack)\t/* 1 */\n@@ -48,43 +47,29 @@ static void exec_rev_list(struct ref *re\n \telse\n \t\targs[i++] = \"--objects\";\n \n-\t/* First send the ones we care about most */\n-\tfor (ref = refs; ref; ref = ref->next) {\n-\t\tif (900 < i)\n-\t\t\tdie(\"git-rev-list environment overflow\");\n-\t\tif (!is_zero_sha1(ref->new_sha1)) {\n-\t\t\tchar *buf = xmalloc(100);\n-\t\t\targs[i++] = buf;\n-\t\t\tsnprintf(buf, 50, \"%s\", sha1_to_hex(ref->new_sha1));\n-\t\t\tbuf += 50;\n-\t\t\tif (!is_zero_sha1(ref->old_sha1) &&\n-\t\t\t    has_sha1_file(ref->old_sha1)) {\n-\t\t\t\targs[i++] = buf;\n-\t\t\t\tsnprintf(buf, 50, \"^%s\",\n-\t\t\t\t\t sha1_to_hex(ref->old_sha1));\n-\t\t\t}\n-\t\t}\n-\t}\n+\targs[i++] = \"--stdin\";\n \n-\t/* Then a handful of the remainder\n-\t * NEEDSWORK: we would be better off if used the newer ones first.\n-\t */\n-\tfor (ref = refs, j = i + 16;\n-\t     i < 900 && i < j && ref;\n-\t     ref = ref->next) {\n-\t\tif (is_zero_sha1(ref->new_sha1) &&\n-\t\t    !is_zero_sha1(ref->old_sha1) &&\n-\t\t    has_sha1_file(ref->old_sha1)) {\n-\t\t\tchar *buf = xmalloc(42);\n-\t\t\targs[i++] = buf;\n-\t\t\tsnprintf(buf, 42, \"^%s\", sha1_to_hex(ref->old_sha1));\n-\t\t}\n-\t}\n \targs[i] = NULL;\n \texecv_git_cmd(args);\n \tdie(\"git-rev-list exec failed (%s)\", strerror(errno));\n }\n \n+static void builtin_rev_list_generate(struct ref *refs)\n+{\n+        while (refs) {\n+                if (!is_zero_sha1(refs->old_sha1) &&\n+                    has_sha1_file(refs->old_sha1)) {\n+                        printf(\"^%s\", sha1_to_hex(refs->old_sha1));\n+                }\n+                if (!is_zero_sha1(refs->new_sha1)) {\n+                        printf(\"%s\", sha1_to_hex(refs->new_sha1));\n+                }\n+                refs = refs->next;\n+        }\n+\n+\texit(0);\n+}\n+\n static void rev_list(int fd, struct ref *refs)\n {\n \tint pipe_fd[2];\n@@ -111,13 +96,38 @@ static void rev_list(int fd, struct ref \n \texec_rev_list(refs);\n }\n \n+static void rev_list_generate(int fd, struct ref *refs)\n+{\n+\tint pipe_fd[2];\n+\tpid_t rev_list_generate_pid;\n+\n+\tif (pipe(pipe_fd) < 0)\n+\t\tdie(\"rev-list-generate setup: pipe failed\");\n+\trev_list_generate_pid = fork();\n+\tif (!rev_list_generate_pid) {\n+\t\tdup2(pipe_fd[0], 0);\n+\t\tdup2(fd, 1);\n+\t\tclose(pipe_fd[0]);\n+\t\tclose(pipe_fd[1]);\n+\t\tclose(fd);\n+\t\trev_list(fd, refs);\n+\t\tdie(\"rev-list setup failed\");\n+\t}\n+\tif (rev_list_generate_pid < 0)\n+\t\tdie(\"rev-list-generate fork failed\");\n+\tdup2(pipe_fd[1], 1);\n+\tclose(pipe_fd[0]);\n+\tclose(pipe_fd[1]);\n+\tclose(fd);\n+\tbuiltin_rev_list_generate(refs);\n+}\n static void pack_objects(int fd, struct ref *refs)\n {\n \tpid_t rev_list_pid;\n \n \trev_list_pid = fork();\n \tif (!rev_list_pid) {\n-\t\trev_list(fd, refs);\n+\t\trev_list_generate(fd, refs);\n \t\tdie(\"rev-list setup failed\");\n \t}\n \tif (rev_list_pid < 0)\n"},{"id":"26378","messageId":"7vpseaarrl.fsf@assigned-by-dhcp.cox.net","threadId":"5486","inReplyTo":"20060905215157.GA29172@shadowen.org","subject":"Re: [PATCH 1/2] rev list add option accepting revision constraints on standard input","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-05T22:10:54Z","receivedAt":"2006-09-05T22:10:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Whitcroft <apw@shadowen.org> writes:\n\n> Add a --stdin flag which causes rev-list to additionally read\n> its stdin stream and parse that for revision constraints.\n\n> +/*\n> + * Parse revision information, filling in the \"rev_info\" structure,\n> + * revisions are taken from stream.\n> + */\n> +static void setup_revisions_stream(FILE *stream, struct rev_info *revs)\n> +{\n> +\tchar line[1000];\n> +\tconst char *args[] = { 0, line, 0 };\n> +\n> +\twhile (fgets(line, sizeof(line), stream) != NULL) {\n> +\t\tline[strlen(line) - 1] = 0;\n> +\n> +\t\tif (line[0] == '-')\n> +\t\t\tdie(\"options not supported in --stdin mode\");\n> +\n> +\t\t(void)setup_revisions(2, args, revs, NULL);\n> +\t}\n> +}\n\nIs calling setup_revisions() on the same revs like this many\ntimes safe?  I do not think so, especially what is after the\nprimary \"for()\" loop in the function.\n\nI was sort-of expecting that you would instead replace that\nprimary for() loop in setup_revisions() with some sort of\ncallback...\n"},{"id":"26383","messageId":"44FE1DE8.1040200@shadowen.org","threadId":"5486","inReplyTo":"7vpseaarrl.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 1/2] rev list add option accepting revision constraints on standard input","fromName":"Andy Whitcroft","fromEmail":"apw@shadowen.org","sentAt":"2006-09-06T01:01:28Z","receivedAt":"2006-09-06T01:01:28Z","isPatch":true,"sender":{"key":"apw@shadowen.org","avatar":"https://gravatar.com/avatar/d3088262854661a913ef35cc40fedcc270142d4461791142bc1ea0b2a4e2e147?d=mp&s=160"},"body":"Junio C Hamano wrote:\n> Andy Whitcroft <apw@shadowen.org> writes:\n> \n>> Add a --stdin flag which causes rev-list to additionally read\n>> its stdin stream and parse that for revision constraints.\n> \n>> +/*\n>> + * Parse revision information, filling in the \"rev_info\" structure,\n>> + * revisions are taken from stream.\n>> + */\n>> +static void setup_revisions_stream(FILE *stream, struct rev_info *revs)\n>> +{\n>> +\tchar line[1000];\n>> +\tconst char *args[] = { 0, line, 0 };\n>> +\n>> +\twhile (fgets(line, sizeof(line), stream) != NULL) {\n>> +\t\tline[strlen(line) - 1] = 0;\n>> +\n>> +\t\tif (line[0] == '-')\n>> +\t\t\tdie(\"options not supported in --stdin mode\");\n>> +\n>> +\t\t(void)setup_revisions(2, args, revs, NULL);\n>> +\t}\n>> +}\n> \n> Is calling setup_revisions() on the same revs like this many\n> times safe?  I do not think so, especially what is after the\n> primary \"for()\" loop in the function.\n> \n> I was sort-of expecting that you would instead replace that\n> primary for() loop in setup_revisions() with some sort of\n> callback...\n\nHeh, well I'll give it another poke tommorrow my time ...\n\nThanks for the feedback.  Its hard to get any context on something new\nwithout trying and failing :).\n\n-apw\n"},{"id":"26384","messageId":"7vk64ha9fp.fsf@assigned-by-dhcp.cox.net","threadId":"5486","inReplyTo":"7vpseaarrl.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 1/2] rev list add option accepting revision constraints on standard input","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-06T04:46:50Z","receivedAt":"2006-09-06T04:46:50Z","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> Is calling setup_revisions() on the same revs like this many\n> times safe?  I do not think so, especially what is after the\n> primary \"for()\" loop in the function.\n>\n> I was sort-of expecting that you would instead replace that\n> primary for() loop in setup_revisions() with some sort of\n> callback...\n\nI take half of the above back.  Even after setup_revisions()\nreturns, adding more revisions via add_pending_object() is\nsafe.  However, the postprocessing done in setup_revisions()\nafter its main loop, while I do not think they would crash when\ncalled twice, would be very wasteful.\n\nAnd ``callback'' interface is usually very cumbersome to use, so\nwe probably would want to avoid it unless absolutely necessary.\n\nI've outlined an alternative implementation in two patches;\nI'll be sending them out shortly.\n"}]}