{"thread":{"id":"13307","subject":"Hmm.. Try harder to find the common commits in git protocol?","startedAt":"2008-04-28T16:41:09Z","lastAt":"2008-04-28T23:20:08Z","messageCount":4,"participants":["Linus Torvalds","Daniel Barkalow","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"75402","messageId":"alpine.LFD.1.10.0804280936080.3119@woody.linux-foundation.org","threadId":"13307","inReplyTo":null,"subject":"Hmm.. Try harder to find the common commits in git protocol?","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-04-28T16:41:09Z","receivedAt":"2008-04-28T16:41:09Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nSo we've had a huge number of merges in this last kernel merge window, and \nas a result it seems that a number of the pulls I do get way too many \nobjects, because the git protocol doesn't try hard enough to find the \nminimal set of common commits. \n\nExample from today:\n\n\tremote: Counting objects: 50084, done.\n\tremote: Compressing objects: 100% (6489/6489), done.\n\tremote: Total 43343 (delta 37381), reused 42777 (delta 36836)\n\tReceiving objects: 100% (43343/43343), 8.85 MiB | 144 KiB/s, done.\n\tResolving deltas: 100% (37381/37381), completed with 5097 local objects.\n\tMerge made by recursive.\n\t kernel/hrtimer.c |   15 +++++++++++++--\n\t 1 files changed, 13 insertions(+), 2 deletions(-)\n\n\tgit rev-list --objects ORIG_HEAD.. | wc -l\n\t7\n\nie it downloaded 43343 objects totalling almost 9MB of data, even though \nit actually only needed seven objects. In fact, I guess three of the seven \nnew objects were due to the merge (commit, top-level tree, and 'kernel' \ntree), so only four objects actually came from the source repository.\n\nI haven't thought about the why's so much (because I'm busy merging), but \nI thought I'd mention this in case somebody else has already looked at it.\n\n\t\tLinus\n"},{"id":"75448","messageId":"alpine.LFD.1.10.0804281310320.3119@woody.linux-foundation.org","threadId":"13307","inReplyTo":"alpine.LFD.1.10.0804280936080.3119@woody.linux-foundation.org","subject":"Re: Hmm.. Try harder to find the common commits in git protocol?","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-04-28T20:15:51Z","receivedAt":"2008-04-28T20:15:51Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nHmm. Trying to debug this, I find the behavior hard to reproduce. But I \n*also* find that there seems to be something wrong in builtin-fetch-pack.\n\nLook at commit f3ec549481827b10609a43bf504517a0e8063a12 (\"fetch-pack: \ncheck parse_commit/object results\"), and tell me that the \"parents\" \nhandling isn't totally broken. In particular, get_rev() does:\n\n\tstruct commit_list *parents = NULL;\n\t..\n\tcommit = rev_list->item;\n\tif (!(commit->object.parsed))\n\t\tif (!parse_commit(commit))\n\t\t\tparents = commit->parents;\n\t..\n\nwhich means that \"parents\" will never even get set if the commit was \nalready parsed!\n\nAnd whether it got parsed or not depends on how we got there etc, so this \nmay explain the occasionally odd behaviour I saw.\n\nBasically, I don't think that code can be right, and with a cut-down repo, \nI get \"no commits in common\" due to this, because the whole \"get_rev()\" \nthing doesn't work.\n\nThis patch should fix that problem, but I wonder why it got rewritten that \nway in the first place?\n\n\t\t\tLinus\n\n---\n builtin-fetch-pack.c |    8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c\nindex 65350ca..c97a427 100644\n--- a/builtin-fetch-pack.c\n+++ b/builtin-fetch-pack.c\n@@ -117,15 +117,15 @@ static const unsigned char* get_rev(void)\n \n \twhile (commit == NULL) {\n \t\tunsigned int mark;\n-\t\tstruct commit_list *parents = NULL;\n+\t\tstruct commit_list *parents;\n \n \t\tif (rev_list == NULL || non_common_revs == 0)\n \t\t\treturn NULL;\n \n \t\tcommit = rev_list->item;\n-\t\tif (!(commit->object.parsed))\n-\t\t\tif (!parse_commit(commit))\n-\t\t\t\tparents = commit->parents;\n+\t\tif (!commit->object.parsed)\n+\t\t\tparse_commit(commit);\n+\t\tparents = commit->parents;\n \n \t\tcommit->object.flags |= POPPED;\n \t\tif (!(commit->object.flags & COMMON))\n"},{"id":"75463","messageId":"alpine.LNX.1.00.0804281716310.19665@iabervon.org","threadId":"13307","inReplyTo":"alpine.LFD.1.10.0804281310320.3119@woody.linux-foundation.org","subject":"Re: Hmm.. Try harder to find the common commits in git protocol?","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-04-28T21:27:22Z","receivedAt":"2008-04-28T21:27:22Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 28 Apr 2008, Linus Torvalds wrote:\n\n> Hmm. Trying to debug this, I find the behavior hard to reproduce. But I \n> *also* find that there seems to be something wrong in builtin-fetch-pack.\n> \n> Look at commit f3ec549481827b10609a43bf504517a0e8063a12 (\"fetch-pack: \n> check parse_commit/object results\"), and tell me that the \"parents\" \n> handling isn't totally broken. In particular, get_rev() does:\n> \n> \tstruct commit_list *parents = NULL;\n> \t..\n> \tcommit = rev_list->item;\n> \tif (!(commit->object.parsed))\n> \t\tif (!parse_commit(commit))\n> \t\t\tparents = commit->parents;\n> \t..\n> \n> which means that \"parents\" will never even get set if the commit was \n> already parsed!\n> \n> And whether it got parsed or not depends on how we got there etc, so this \n> may explain the occasionally odd behaviour I saw.\n> \n> Basically, I don't think that code can be right, and with a cut-down repo, \n> I get \"no commits in common\" due to this, because the whole \"get_rev()\" \n> thing doesn't work.\n> \n> This patch should fix that problem, but I wonder why it got rewritten that \n> way in the first place?\n\nLooks like f3ec549481827b10609a43bf504517a0e8063a12 was trying to do \nsomething clever when parse_commit returns non-zero, and accidentally did \nthe same thing with the case where it isn't needed.\n\n> \n> \t\t\tLinus\n> \n> ---\n>  builtin-fetch-pack.c |    8 ++++----\n>  1 files changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c\n> index 65350ca..c97a427 100644\n> --- a/builtin-fetch-pack.c\n> +++ b/builtin-fetch-pack.c\n> @@ -117,15 +117,15 @@ static const unsigned char* get_rev(void)\n>  \n>  \twhile (commit == NULL) {\n>  \t\tunsigned int mark;\n> -\t\tstruct commit_list *parents = NULL;\n> +\t\tstruct commit_list *parents;\n>  \n>  \t\tif (rev_list == NULL || non_common_revs == 0)\n>  \t\t\treturn NULL;\n>  \n>  \t\tcommit = rev_list->item;\n> -\t\tif (!(commit->object.parsed))\n> -\t\t\tif (!parse_commit(commit))\n> -\t\t\t\tparents = commit->parents;\n> +\t\tif (!commit->object.parsed)\n> +\t\t\tparse_commit(commit);\n> +\t\tparents = commit->parents;\n\nI think it's supposed to be:\n\n\t\tif (commit->object.parsed ||\n\t\t    !parse_commit(commit)))\n\t\t\tparents = commit->parents;\n\nJust looking at what the other changes in the same commit do.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"75476","messageId":"7vabjdbm8n.fsf@gitster.siamese.dyndns.org","threadId":"13307","inReplyTo":"alpine.LFD.1.10.0804281310320.3119@woody.linux-foundation.org","subject":"Re: Hmm.. Try harder to find the common commits in git protocol?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-04-28T23:20:08Z","receivedAt":"2008-04-28T23:20:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> Hmm. Trying to debug this, I find the behavior hard to reproduce. But I \n> *also* find that there seems to be something wrong in builtin-fetch-pack.\n>\n> Look at commit f3ec549481827b10609a43bf504517a0e8063a12 (\"fetch-pack: \n> check parse_commit/object results\"), and tell me that the \"parents\" \n> handling isn't totally broken. In particular, get_rev() does:\n>\n> \tstruct commit_list *parents = NULL;\n> \t..\n> \tcommit = rev_list->item;\n> \tif (!(commit->object.parsed))\n> \t\tif (!parse_commit(commit))\n> \t\t\tparents = commit->parents;\n> \t..\n>\n> which means that \"parents\" will never even get set if the commit was \n> already parsed!\n>\n> And whether it got parsed or not depends on how we got there etc, so this \n> may explain the occasionally odd behaviour I saw.\n>\n> Basically, I don't think that code can be right,...\n\n\n> This patch should fix that problem, but I wonder why it got rewritten that \n> way in the first place?\n\nSorry, my fault while reviewing the series.  The intent of the update was\ntoo focused on not touching possibly bad data (i.e. when parse_commit()\ndoes not return zero to indicate success) and failed to make sure that it\nacts identically on good data --- very bad.\n"}]}