{"thread":{"id":"34407","subject":"[RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","startedAt":"2013-07-11T22:01:27Z","lastAt":"2013-10-26T10:49:18Z","messageCount":30,"participants":["Matthijs Kooijman","Junio C Hamano","Duy Nguyen","Nguyễn Thái Ngọc Duy","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"223100","messageId":"20130711220127.GK10217@login.drsnuggles.stderr.nl","threadId":"34407","inReplyTo":null,"subject":"[RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","fromName":"Matthijs Kooijman","fromEmail":"matthijs@stdin.nl","sentAt":"2013-07-11T22:01:27Z","receivedAt":"2013-07-11T22:01:27Z","isPatch":true,"sender":{"key":"matthijs@stdin.nl","avatar":"https://avatars.githubusercontent.com/u/194491?v=4"},"body":"Hi folks,\n\nwhile playing with shallow fetches, I've found that in some\ncircumstances running git fetch with --depth can return too many objects\n(in particular, _all_ the objects for the requested revisions are\nreturned, even when some of those objects are already known to the\nclient).\n\nThis happens when a client issues a fetch with a depth bigger or equal\nto the number of commits the server is ahead of the client. In this\ncase, the revisions to be sent over will be completely detached from any\nrevisions the client already has (history-wise), causing the server to\neffectively ignore all objects the client has (as advertised using its\nhave lines) and just send over _all_ objects (needed for the revisions\nit is sending over).\n\nI've traced this down to the way do_rev_list in upload-pack.c works. If\nI've poured over the code enough to understand it, this is what happens:\n - The new shallow roots are made into graft points without parents.\n - The \"want\" commits are added to the pending list (revs->pending)\n - The \"have\" commits are marked uninteresting and added to the pending list\n - prepare_revision_walk is called, which adds everything from the\n   pending list into the commmit list (revs->commits)\n - limit_list is called, which traverses the history of each interesting\n   commit in the commit list (i.e., all want revisions), up to excluding\n   the first uninteresting commit (i.e. a have revision). The result of\n   this is the new commit list.\n\n   This means the commit list now contains all commits that the client\n   wants, up to (excluding) any commits he already has or up to\n   (including) any (new) shallow roots.\n - mark_edges_uninteresting is called, which marks the tree of every\n   parent of each edge in the commit list as uninteresting (in practice,\n   this marks the tree of each uninteresting parent, since those are by\n   definition the only kinds of revisions that can be beyond the edge).\n - All trees and blobs that are referenced by trees in the commit list\n   but are not marked as uninteresting, are passed to git-pack-objects\n   to put into the pack.\n\nNormally, the list of commits to send over is connected to the\nclient's existing commits (which are marked as uninteresting). This\nmeans that only the trees of those uninteresting (\"have\") commits that\nare actually (direct) predecessors of the commits to send over are\nmarked as uninteresting. This is probably useful, since it prevents\nhaving to go over all trees the client has (for other branches, for\nexample) and instead limits to the trees that are the most likely to\ncontain duplicate (or similar, for delta-ing) objects.\n\nHowever, in the \"detached shallow fetch\" case, this assumption is no\nlonger valid. There will be no uninteresting commits as parents for\nthe commit list, since all edge commits will be shallow roots (hence\nhave no parents).  Ideally, one would find out which of the \"detached\"\n\"have\" revisions are the closest to the new shallow roots, but with the\ncurrent code these shallow roots have their parents cut off long before\nthis code even runs, so this is probably not feasible.\n\nInstead, what we can do in this case, is simply mark the trees of all\n\"have\" commits as uninteresting. This prevents all objects that are\ncontained in the \"have\" commits themselves from being sent to the\nclient, which can be a big win for bigger repositories. Marking them all\nis is probably more work than strictly needed, but is easy to implement.\n\nI have created a mockup patch which does this, and also adds a test case\ndemonstrating the problem. Right now, the above fix is applied always,\neven in cases where it isn't needed.\n\nLooking at the code, I think it would be good to let\nmark_edges_uninteresting look for shallow roots in the commit list (or\nperhaps just add another loop over the commit list inside do_rev_list)\nand only apply the fix if any shallow roots are in the commit list\n(meaning at least a part of the history to send over is detached from\nthe clients current history). I haven't implemented this yet, wanting to\nget some feedback first.\n\nAlso, I'm not quite sure how this fits in with the concept of \"thin\npacks\". There might be some opportunities missing here as well, though\ngit-pack-objects is called without --thin when shallow roots are\ninvolved. I think this is related to the \"-\" prefixed commit sha's that\nare sent to git-pack-objects, but I couldn't found any documentation on\nwhat the - prefix is supposed to mean.\n\n(On a somewhat related note, show_commit in upload-pack.c checks the\nBOUNDARY flag, but AFAICS the revs->boundary flag is never set, so\nBOUNDARY cannot ever be set in this case either?)\n\nHow does this patch look?\n\nGr.\n\nMatthijs\n\n---\n t/t5500-fetch-pack.sh | 11 +++++++++++\n upload-pack.c         |  8 ++++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex fd2598e..a022d65 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -393,6 +393,17 @@ test_expect_success 'fetch in shallow repo unreachable shallow objects' '\n \t\tgit fsck --no-dangling\n \t)\n '\n+test_expect_success 'fetch creating new shallow root' '\n+\t(\n+\t\tgit clone \"file://$(pwd)/.\" shallow10 &&\n+\t\tgit commit --allow-empty -m empty &&\n+\t\tcd shallow10 &&\n+\t\tgit fetch --depth=1 --progress 2> actual &&\n+\t\t# This should fetch only the empty commit, no tree or\n+\t\t# blob objects\n+\t\tgrep \"remote: Total 1\" actual\n+\t)\n+'\n \n test_expect_success 'setup tests for the --stdin parameter' '\n \tfor head in C D E F\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 59f43d1..5885f33 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -122,6 +122,14 @@ static int do_rev_list(int in, int out, void *user_data)\n \tif (prepare_revision_walk(&revs))\n \t\tdie(\"revision walk setup failed\");\n \tmark_edges_uninteresting(revs.commits, &revs, show_edge);\n+\t/* In case we create a new shallow root, make sure that all\n+\t * we don't send over objects that the client already has just\n+\t * because their \"have\" revisions are no longer reachable from\n+\t * the shallow root. */\n+\tfor (i = 0; i < have_obj.nr; i++) {\n+\t\tstruct commit *commit = (struct commit *)have_obj.objects[i].item;\n+\t\tmark_tree_uninteresting(commit->tree);\n+\t}\n \tif (use_thin_pack)\n \t\tfor (i = 0; i < extra_edge_obj.nr; i++)\n \t\t\tfprintf(pack_pipe, \"-%s\\n\", sha1_to_hex(\n-- \n1.8.3.2.736.ge92bb95.dirty\n"},{"id":"223115","messageId":"7vsizkpv21.fsf@alter.siamese.dyndns.org","threadId":"34407","inReplyTo":"20130711220127.GK10217@login.drsnuggles.stderr.nl","subject":"Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-11T22:53:58Z","receivedAt":"2013-07-11T22:53:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthijs Kooijman <matthijs@stdin.nl> writes:\n\n[administrivia: you seem to have mail-followup-to that points at you\nand the list; is that really needed???]\n\n> This happens when a client issues a fetch with a depth bigger or equal\n> to the number of commits the server is ahead of the client.\n\nDo you mean \"smaller\" (not \"bigger\")?\n\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 59f43d1..5885f33 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -122,6 +122,14 @@ static int do_rev_list(int in, int out, void *user_data)\n>  \tif (prepare_revision_walk(&revs))\n>  \t\tdie(\"revision walk setup failed\");\n>  \tmark_edges_uninteresting(revs.commits, &revs, show_edge);\n> +\t/* In case we create a new shallow root, make sure that all\n> +\t * we don't send over objects that the client already has just\n> +\t * because their \"have\" revisions are no longer reachable from\n> +\t * the shallow root. */\n> +\tfor (i = 0; i < have_obj.nr; i++) {\n> +\t\tstruct commit *commit = (struct commit *)have_obj.objects[i].item;\n> +\t\tmark_tree_uninteresting(commit->tree);\n> +\t}\n\nHmph.\n\nIn your discussion (including the comment), you talk about \"shallow\nroot\" (I think that is the same as what we call \"shallow boundary\"),\nbut in this added block, there is nothing that checks CLIENT_SHALLOW\nor SHALLOW flags to special case that.\n\nIs it a good idea to unconditionally do this for all \"have\"\nrevisions?\n\nAlso there is another loop that iterates over \"have\" revisions just\nabove the precontext.  I wonder if this added code belongs in that\nloop.\n"},{"id":"223149","messageId":"20130712071157.GL10217@login.drsnuggles.stderr.nl","threadId":"34407","inReplyTo":"7vsizkpv21.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","fromName":"Matthijs Kooijman","fromEmail":"matthijs@stdin.nl","sentAt":"2013-07-12T07:11:57Z","receivedAt":"2013-07-12T07:11:57Z","isPatch":true,"sender":{"key":"matthijs@stdin.nl","avatar":"https://avatars.githubusercontent.com/u/194491?v=4"},"body":"Hi Junio,\n\n> [administrivia: you seem to have mail-followup-to that points at you\n> and the list; is that really needed???]\nI'm not subscribed to the list, so yes :-)\n\n> > This happens when a client issues a fetch with a depth bigger or equal\n> > to the number of commits the server is ahead of the client.\n> \n> Do you mean \"smaller\" (not \"bigger\")?\nYes, I meant smaller (reworded this first sentence a few times and then messed\nup :-)\n\n> > diff --git a/upload-pack.c b/upload-pack.c\n> > index 59f43d1..5885f33 100644\n> > --- a/upload-pack.c\n> > +++ b/upload-pack.c\n> > @@ -122,6 +122,14 @@ static int do_rev_list(int in, int out, void *user_data)\n> >  \tif (prepare_revision_walk(&revs))\n> >  \t\tdie(\"revision walk setup failed\");\n> >  \tmark_edges_uninteresting(revs.commits, &revs, show_edge);\n> > +\t/* In case we create a new shallow root, make sure that all\n> > +\t * we don't send over objects that the client already has just\n> > +\t * because their \"have\" revisions are no longer reachable from\n> > +\t * the shallow root. */\n> > +\tfor (i = 0; i < have_obj.nr; i++) {\n> > +\t\tstruct commit *commit = (struct commit *)have_obj.objects[i].item;\n> > +\t\tmark_tree_uninteresting(commit->tree);\n> > +\t}\n> \n> Hmph.\n> \n> In your discussion (including the comment), you talk about \"shallow\n> root\" (I think that is the same as what we call \"shallow boundary\"),\nI think so, yes. I mean to refer to the commits referenced in\n.git/shallow, that have their parents \"hidden\".\n\n> but in this added block, there is nothing that checks CLIENT_SHALLOW\n> or SHALLOW flags to special case that.\n>\n> Is it a good idea to unconditionally do this for all \"have\"\n> revisions?\nThat's what I meant in my mail with \"applying the fix unconditionally\" -\nthere is probably some check needed (I discussed a few options in the\nmail as well).\n\nNote that this entire do_rev_list function is only called when there are\nshallow revisions involved, so there is also a basic \"only when shallow\"\ncheck in place.\n\n> Also there is another loop that iterates over \"have\" revisions just\n> above the precontext.  I wonder if this added code belongs in that\n> loop.\nI think we could add it there, yes. On the other hand, if we only want\nto execute this code when there are shallow boundaries in the list of\nrevisions to send (as I suggested in my previous mail), then we can't\nmove this code up.\n\nGr.\n\nMatthijs\n"},{"id":"224743","messageId":"20130807102716.GA10217@login.drsnuggles.stderr.nl","threadId":"34407","inReplyTo":"20130712071157.GL10217@login.drsnuggles.stderr.nl","subject":"Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","fromName":"Matthijs Kooijman","fromEmail":"matthijs@stdin.nl","sentAt":"2013-08-07T10:27:16Z","receivedAt":"2013-08-07T10:27:16Z","isPatch":true,"sender":{"key":"matthijs@stdin.nl","avatar":"https://avatars.githubusercontent.com/u/194491?v=4"},"body":"Hi Junio,\n\nI haven't got a reply to my mail yet. Could you have a look, so I can\nupdate and resubmit my patch?\n\nOn Fri, Jul 12, 2013 at 09:11:57AM +0200, Matthijs Kooijman wrote:\n> > [administrivia: you seem to have mail-followup-to that points at you\n> > and the list; is that really needed???]\n> > In your discussion (including the comment), you talk about \"shallow\n> > root\" (I think that is the same as what we call \"shallow boundary\"),\n> I think so, yes. I mean to refer to the commits referenced in\n> .git/shallow, that have their parents \"hidden\".\nCould you confirm that I got the terms right here (or is the shallow\nboundary the first hidden commit?)\n\n> > but in this added block, there is nothing that checks CLIENT_SHALLOW\n> > or SHALLOW flags to special case that.\n> >\n> > Is it a good idea to unconditionally do this for all \"have\"\n> > revisions?\n> That's what I meant in my mail with \"applying the fix unconditionally\" -\n> there is probably some check needed (I discussed a few options in the\n> mail as well).\n>\n> Note that this entire do_rev_list function is only called when there are\n> shallow revisions involved, so there is also a basic \"only when shallow\"\n> check in place.\n\nMy proposal was to only apply the fix for all have revisions when the\nprevious history traversal came across some shallow boundary commits. If\nthis happens, then that shallow boundary commit will be a \"new\" one and\nit will have prevented the history traversal from finding the full list\nof relevant \"have\" commits. In this case, we should just use all \"have\"\ncommits instead.\n\nNow, looking at the code, I see a few options for detecting this case:\n\n 1 Modify mark_edges_uninteresting to return a boolean (or have an\n   output argument) if any of the commits in the list of commits to find\n   (not the edges) is a shallow boundary.\n 2 Modify mark_edges_uninteresting to have a \"show_shallow\" argument\n   that gets called for every shallow boundary. The show_shallow\n   function passed would then simply keep a boolean if it is passed at\n   least once.\n 3 Add another loop over the commits _after_ the call to\n   mark_edges_uninteresting, that simply looks for any shallow boundary\n   commit.\n\nThe last option seems sensible to me, since it prevents modifying the\nsomewhat generic mark_edges_uninteresting function for this specific\nusecase. On the other hand, it does mean that the list of commits is\nlooped twice, not sure what that means for performance.\n\nBefore I go and implement one of these, which option seems best to you?\n\nGr.\n\nMatthijs\n"},{"id":"224778","messageId":"7v61vhc7wn.fsf@alter.siamese.dyndns.org","threadId":"34407","inReplyTo":"20130807102716.GA10217@login.drsnuggles.stderr.nl","subject":"Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-08T01:01:44Z","receivedAt":"2013-08-08T01:01:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthijs Kooijman <matthijs@stdin.nl> writes:\n\n>> > In your discussion (including the comment), you talk about \"shallow\n>> > root\" (I think that is the same as what we call \"shallow boundary\"),\n>> I think so, yes. I mean to refer to the commits referenced in\n>> .git/shallow, that have their parents \"hidden\".\n> Could you confirm that I got the terms right here (or is the shallow\n> boundary the first hidden commit?)\n\nAs long as you are consistent it is fine. I _think_ boundary refers\nto what is recorded in the .git/shallow file, so they are commits\nthat are missing from our repository, and their immediate children\nare available.\n\n> My proposal was to only apply the fix for all have revisions when the\n> previous history traversal came across some shallow boundary commits. If\n> this happens, then that shallow boundary commit will be a \"new\" one and\n> it will have prevented the history traversal from finding the full list\n> of relevant \"have\" commits. In this case, we should just use all \"have\"\n> commits instead.\n>\n> Now, looking at the code, I see a few options for detecting this case:\n>\n>  1 Modify mark_edges_uninteresting to return a boolean (or have an\n>    output argument) if any of the commits in the list of commits to find\n>    (not the edges) is a shallow boundary.\n>  2 Modify mark_edges_uninteresting to have a \"show_shallow\" argument\n>    that gets called for every shallow boundary. The show_shallow\n>    function passed would then simply keep a boolean if it is passed at\n>    least once.\n>  3 Add another loop over the commits _after_ the call to\n>    mark_edges_uninteresting, that simply looks for any shallow boundary\n>    commit.\n>\n> The last option seems sensible to me, since it prevents modifying the\n> somewhat generic mark_edges_uninteresting function for this specific\n> usecase. On the other hand, it does mean that the list of commits is\n> looped twice, not sure what that means for performance.\n>\n> Before I go and implement one of these, which option seems best to you?\n\nMy gut feeling without looking at any patch is that the simplest\n(i.e. 3.) would be the best among these three.\n\nBut I suspect, with any of these approaches, you would need to be\nvery careful futzing with the edge ones.  It may have an interesting\ninteractions with --thin transfer.\n"},{"id":"224779","messageId":"CACsJy8AUrrMuW9TgT=gCfFVNq8H0zNjCsZwBY_0Ty-tdEgUYyQ@mail.gmail.com","threadId":"34407","inReplyTo":"7v61vhc7wn.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-08T01:09:49Z","receivedAt":"2013-08-08T01:09:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 8, 2013 at 8:01 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Matthijs Kooijman <matthijs@stdin.nl> writes:\n>\n>>> > In your discussion (including the comment), you talk about \"shallow\n>>> > root\" (I think that is the same as what we call \"shallow boundary\"),\n>>> I think so, yes. I mean to refer to the commits referenced in\n>>> .git/shallow, that have their parents \"hidden\".\n>> Could you confirm that I got the terms right here (or is the shallow\n>> boundary the first hidden commit?)\n>\n> As long as you are consistent it is fine. I _think_ boundary refers\n> to what is recorded in the .git/shallow file, so they are commits\n> that are missing from our repository, and their immediate children\n> are available.\n\nHaven't found time to read the rest yet, but this I can answer.\n.git/shallow records graft points. If a commit is in .git/shallow and\nit exists in the repository, the commit is considered to have no\nparents regardless of what's recorded in repository. So .git/shallow\nrefers to the new roots, not the missing bits.\n-- \nDuy\n"},{"id":"224786","messageId":"CACsJy8CP6pGRwEn6H=cbKxTMuOjzAF3=Qh8qsLbJaw6feK3NMw@mail.gmail.com","threadId":"34407","inReplyTo":"20130711220127.GK10217@login.drsnuggles.stderr.nl","subject":"Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-08T04:50:55Z","receivedAt":"2013-08-08T04:50:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jul 12, 2013 at 5:01 AM, Matthijs Kooijman <matthijs@stdin.nl> wrote:\n> Hi folks,\n>\n> while playing with shallow fetches, I've found that in some\n> circumstances running git fetch with --depth can return too many objects\n> (in particular, _all_ the objects for the requested revisions are\n> returned, even when some of those objects are already known to the\n> client).\n>\n> This happens when a client issues a fetch with a depth bigger or equal\n> to the number of commits the server is ahead of the client. In this\n> case, the revisions to be sent over will be completely detached from any\n> revisions the client already has (history-wise), causing the server to\n> effectively ignore all objects the client has (as advertised using its\n> have lines) and just send over _all_ objects (needed for the revisions\n> it is sending over).\n>\n> I've traced this down to the way do_rev_list in upload-pack.c works. If\n> I've poured over the code enough to understand it, this is what happens:\n>  - The new shallow roots are made into graft points without parents.\n>  - The \"want\" commits are added to the pending list (revs->pending)\n>  - The \"have\" commits are marked uninteresting and added to the pending list\n>  - prepare_revision_walk is called, which adds everything from the\n>    pending list into the commmit list (revs->commits)\n>  - limit_list is called, which traverses the history of each interesting\n>    commit in the commit list (i.e., all want revisions), up to excluding\n>    the first uninteresting commit (i.e. a have revision). The result of\n>    this is the new commit list.\n>\n>    This means the commit list now contains all commits that the client\n>    wants, up to (excluding) any commits he already has or up to\n>    (including) any (new) shallow roots.\n>  - mark_edges_uninteresting is called, which marks the tree of every\n>    parent of each edge in the commit list as uninteresting (in practice,\n>    this marks the tree of each uninteresting parent, since those are by\n>    definition the only kinds of revisions that can be beyond the edge).\n>  - All trees and blobs that are referenced by trees in the commit list\n>    but are not marked as uninteresting, are passed to git-pack-objects\n>    to put into the pack.\n>\n> Normally, the list of commits to send over is connected to the\n> client's existing commits (which are marked as uninteresting). This\n> means that only the trees of those uninteresting (\"have\") commits that\n> are actually (direct) predecessors of the commits to send over are\n> marked as uninteresting. This is probably useful, since it prevents\n> having to go over all trees the client has (for other branches, for\n> example) and instead limits to the trees that are the most likely to\n> contain duplicate (or similar, for delta-ing) objects.\n>\n> However, in the \"detached shallow fetch\" case, this assumption is no\n> longer valid. There will be no uninteresting commits as parents for\n> the commit list, since all edge commits will be shallow roots (hence\n> have no parents).  Ideally, one would find out which of the \"detached\"\n> \"have\" revisions are the closest to the new shallow roots, but with the\n> current code these shallow roots have their parents cut off long before\n> this code even runs, so this is probably not feasible.\n\nI think this applies to general case as well, not just shallow.\nImagine I have a disconnected commit that points to the latest tree\n(i.e. it contains most of latest changes). Because it's disconnected,\nit'll be ignored by the server side. But if the servide side does\nmark_tree_interesting on this commit, a bunch of blobs might be\nexcluded from sending. I used to (ab)use git and store a bunch of tags\npoint to trees. These trees share a lot. Still, fetching a new tag\nmeans pulling all objects of the new tree even though it only needs a\nfew new blobs and trees. So perhaps we could go over have_obj list\nagain, if it's not processed and is\n\n - a tree-ish, mark_tree_uninteresting\n - a blob, just mark unintesting\n\nand this does regardless of shallow state or edges. The only downside\nis mark_tree_uninteresting is recursive so in unpacks lots of trees if\nhave_obj is long, or the worktree is really big. Commit bitmap should\nhelp reduce the cost if have_obj is a committish, at least.\n-- \nDuy\n"},{"id":"224789","messageId":"7vli4cbs91.fsf@alter.siamese.dyndns.org","threadId":"34407","inReplyTo":"CACsJy8AUrrMuW9TgT=gCfFVNq8H0zNjCsZwBY_0Ty-tdEgUYyQ@mail.gmail.com","subject":"Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-08T06:39:54Z","receivedAt":"2013-08-08T06:39:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> Haven't found time to read the rest yet, but this I can answer.\n> .git/shallow records graft points. If a commit is in .git/shallow and\n> it exists in the repository, the commit is considered to have no\n> parents regardless of what's recorded in repository. So .git/shallow\n> refers to the new roots, not the missing bits.\n\nThanks.\n"},{"id":"224791","messageId":"7vfvukbrqh.fsf@alter.siamese.dyndns.org","threadId":"34407","inReplyTo":"CACsJy8CP6pGRwEn6H=cbKxTMuOjzAF3=Qh8qsLbJaw6feK3NMw@mail.gmail.com","subject":"Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-08T06:51:02Z","receivedAt":"2013-08-08T06:51:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> I think this applies to general case as well, not just shallow.\n> Imagine I have a disconnected commit that points to the latest tree\n> (i.e. it contains most of latest changes). Because it's disconnected,\n> it'll be ignored by the server side. But if the servide side does\n> mark_tree_interesting on this commit, a bunch of blobs might be\n> excluded from sending.\n\nI think you meant mark_tree_UNinteresting.\n\n> ... So perhaps we could go over have_obj list\n> again, if it's not processed and is\n>\n>  - a tree-ish, mark_tree_uninteresting\n>  - a blob, just mark unintesting\n>\n> and this does regardless of shallow state or edges.\n\nAs a general idea, I agree it may be worth trying out to see if your\nconcern that the \"have\" list may be so big that this approach may be\nmore costly than it is worth.\n\nIf the recipient is known to have something, we do not have to send\nit.\n\nThe things that we decide not to send are not necessarily what the\nrecipient has, which introduces a twist you need to watch out for if\nwe want to go that route.\n\nIf the recipient is known to have something, a thin transfer can\nsend a delta against it.  You do not want to send the commits before\nthe shallow boundary (i.e. the parents of the commits listed in\n.git/shallow) because the recipient does not want them, and that\nmeans you may have to use a different mark to record that fact.  The\nrecipient does not have them, we do not want to send them, and they\ncannot be used as a delta base for what we do send.  Which is quite\ndifferent from the ordinary \"uninteresting\" objects, those we decide\nnot to send because the recipient has them.\n"},{"id":"224794","messageId":"CACsJy8BahoGcDcLjSaHA-62_KQE2wD-p5oeJOOA4nk8ZRfXrEA@mail.gmail.com","threadId":"34407","inReplyTo":"7vfvukbrqh.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-08T07:21:33Z","receivedAt":"2013-08-08T07:21:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 8, 2013 at 1:51 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>> I think this applies to general case as well, not just shallow.\n>> Imagine I have a disconnected commit that points to the latest tree\n>> (i.e. it contains most of latest changes). Because it's disconnected,\n>> it'll be ignored by the server side. But if the servide side does\n>> mark_tree_interesting on this commit, a bunch of blobs might be\n>> excluded from sending.\n>\n> I think you meant mark_tree_UNinteresting.\n\nYes, thanks for correcting.\n\n>> ... So perhaps we could go over have_obj list\n>> again, if it's not processed and is\n>>\n>>  - a tree-ish, mark_tree_uninteresting\n>>  - a blob, just mark unintesting\n>>\n>> and this does regardless of shallow state or edges.\n>\n> As a general idea, I agree it may be worth trying out to see if your\n> concern that the \"have\" list may be so big that this approach may be\n> more costly than it is worth.\n>\n> If the recipient is known to have something, we do not have to send\n> it.\n\nOK. Mathijs, do you want make a patch for it?\n\n> The things that we decide not to send are not necessarily what the\n> recipient has, which introduces a twist you need to watch out for if\n> we want to go that route.\n>\n> If the recipient is known to have something, a thin transfer can\n> send a delta against it.  You do not want to send the commits before\n> the shallow boundary (i.e. the parents of the commits listed in\n> .git/shallow) because the recipient does not want them, and that\n> means you may have to use a different mark to record that fact.  The\n> recipient does not have them, we do not want to send them, and they\n> cannot be used as a delta base for what we do send.  Which is quite\n> different from the ordinary \"uninteresting\" objects, those we decide\n> not to send because the recipient has them.\n\nI fail to see the point here. There are two different things: what we\nwant to send, and what we can make deltas against. Shallow boundary\naffects the former. What the recipient has affects latter. What is the\ntwist about?\n\nAs for considering objects before shallow boundary uninteresting, I\nhave a plan for it: kill upload-pack.c:do_rev_list(). The function is\ncreated to make a cut at shallow boundary, but we already have a tool\nfor that: grafting. In my ongoing shallow series I will create a\ntemporary shallow file that contains new roots and pass the file to\npack-objects with --shallow-file. pack-objects will never see anything\noutside what the recipient may want (i.e. commits before shallow\nboundary) to receive and pack-objects' rev-list should do what\nupload-pack.c:do_rev_list() currently does.\n-- \nDuy\n"},{"id":"224827","messageId":"7v1u64az18.fsf@alter.siamese.dyndns.org","threadId":"34407","inReplyTo":"CACsJy8BahoGcDcLjSaHA-62_KQE2wD-p5oeJOOA4nk8ZRfXrEA@mail.gmail.com","subject":"Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-08T17:10:59Z","receivedAt":"2013-08-08T17:10:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> I fail to see the point here. There are two different things: what we\n> want to send, and what we can make deltas against. Shallow boundary\n> affects the former. What the recipient has affects latter. What is the\n> twist about?\n\ndo_rev_list() --> mark_edges_uninteresting() --> show_edge() callchain\nthat eventually does this:\n\nstatic void show_edge(struct commit *commit)\n{\n\tfprintf(pack_pipe, \"-%s\\n\", sha1_to_hex(commit->object.sha1));\n}\n\nwas what I had in mind.\n\nFor a non-shallow transfer, feeding \"-<boundary commit>\" is done for\ncommits that we do not send (we do not do so for all of them) and\nthose that we know the recipient does have.  Two different things\nused to be the same, but with your suggestion they are not.  Which\nis a good thing but we need to be careful to make sure existing\ncodepaths do not conflate them and untangle ones that do if there\nare any, that's all.\n\n> As for considering objects before shallow boundary uninteresting, I\n> have a plan for it: kill upload-pack.c:do_rev_list(). The function is\n> created to make a cut at shallow boundary,...\n\nHmph, that function is not primarily about shallow boundary but does\nall packing in general.\n\nThe edge hinting in there is for thin transfer where the sender\nsends deltas against base objects that are known to be present in\nthe receiving repository, without sending the base objects.\n"},{"id":"224902","messageId":"CACsJy8A5VkyMcvtqu2J0COh5+pqKTkmCstLzDpPS5_0T+XFtsA@mail.gmail.com","threadId":"34407","inReplyTo":"7v1u64az18.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-09T13:13:09Z","receivedAt":"2013-08-09T13:13:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Aug 9, 2013 at 12:10 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>> I fail to see the point here. There are two different things: what we\n>> want to send, and what we can make deltas against. Shallow boundary\n>> affects the former. What the recipient has affects latter. What is the\n>> twist about?\n>\n> do_rev_list() --> mark_edges_uninteresting() --> show_edge() callchain\n> that eventually does this:\n>\n> static void show_edge(struct commit *commit)\n> {\n>         fprintf(pack_pipe, \"-%s\\n\", sha1_to_hex(commit->object.sha1));\n> }\n>\n> was what I had in mind.\n\nNow I see. Thanks.\n\nmark_edges_uninteresting() actually calls\nmark_edge_parents_uninteresting(), which calls show_edge(). The middle\nfunction is important because after calculating new depth, upload-pack\ncalls register_shallow() for all both old and new shallow roots and\nthose commits will have their 'parents' pointer set to NULL, which\nrenders mark_edge_parents_uninteresting() no-op. So show_edge() is\nnever called on shallow points' parents.\n\n>> As for considering objects before shallow boundary uninteresting, I\n>> have a plan for it: kill upload-pack.c:do_rev_list(). The function is\n>> created to make a cut at shallow boundary,...\n>\n> Hmph, that function is not primarily about shallow boundary but does\n> all packing in general.\n>\n> The edge hinting in there is for thin transfer where the sender\n> sends deltas against base objects that are known to be present in\n> the receiving repository, without sending the base objects.\n\nOK but edge hinting is the same in pack-objects.c:get_object_list() so\nthe plan might still work, right? I still need to study about\nextra_edge_obj in upload-pack.c though. That's something knowledge\nthat pack-objects won't have.\n-- \nDuy\n"},{"id":"225091","messageId":"20130812080203.GK10217@login.drsnuggles.stderr.nl","threadId":"34407","inReplyTo":"CACsJy8BahoGcDcLjSaHA-62_KQE2wD-p5oeJOOA4nk8ZRfXrEA@mail.gmail.com","subject":"Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","fromName":"Matthijs Kooijman","fromEmail":"matthijs@stdin.nl","sentAt":"2013-08-12T08:02:03Z","receivedAt":"2013-08-12T08:02:03Z","isPatch":true,"sender":{"key":"matthijs@stdin.nl","avatar":"https://avatars.githubusercontent.com/u/194491?v=4"},"body":"Hi Duy,\n\n> OK. Mathijs, do you want make a patch for it?\nI'm willing, but:\n - I don't understand the code and all of your comments well enough yet\n   to start coding right away (though I haven't actually invested enough\n   time in this yet, either).\n - I'll be on vacation for the next two weeks.\n\nWhen I get back, I'll re-read this thread properly and reply where I\ndon't follow it. Feel free to continue discussing the plan until then,\nof course :-)\n\nGr.\n\nMatthijs\n"},{"id":"225321","messageId":"CACsJy8CDGgKftp0iBB8MYjMawKhxZ1JQ+xAYb0itpaCOjFHWxg@mail.gmail.com","threadId":"34407","inReplyTo":"20130812080203.GK10217@login.drsnuggles.stderr.nl","subject":"Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-16T09:51:07Z","receivedAt":"2013-08-16T09:51:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Aug 12, 2013 at 3:02 PM, Matthijs Kooijman <matthijs@stdin.nl> wrote:\n> Hi Duy,\n>\n>> OK. Mathijs, do you want make a patch for it?\n> I'm willing, but:\n>  - I don't understand the code and all of your comments well enough yet\n>    to start coding right away (though I haven't actually invested enough\n>    time in this yet, either).\n>  - I'll be on vacation for the next two weeks.\n>\n> When I get back, I'll re-read this thread properly and reply where I\n> don't follow it. Feel free to continue discussing the plan until then,\n> of course :-)\n\nI thought a bit but my thoughts often get stuck if I don't write them\ndown in form of code :-) so this is what I got so far. 4/6 is a good\nthing in my opinion, but I might overlook something 6/6  is about this\nthread. I'm likely offline this weekend, so all is good :-D\n-- \nDuy\n"},{"id":"225322","messageId":"1376646727-22318-1-git-send-email-pclouds@gmail.com","threadId":"34407","inReplyTo":"CACsJy8CDGgKftp0iBB8MYjMawKhxZ1JQ+xAYb0itpaCOjFHWxg@mail.gmail.com","subject":"[PATCH 1/6] Move setup_alternate_shallow and write_shallow_commits to shallow.c","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-16T09:52:02Z","receivedAt":"2013-08-16T09:52:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n commit.h     |  3 +++\n fetch-pack.c | 53 +----------------------------------------------------\n shallow.c    | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 58 insertions(+), 52 deletions(-)\n\ndiff --git a/commit.h b/commit.h\nindex d912a9d..790e31b 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -198,6 +198,9 @@ extern struct commit_list *get_shallow_commits(struct object_array *heads,\n \t\tint depth, int shallow_flag, int not_shallow_flag);\n extern void check_shallow_file_for_update(void);\n extern void set_alternate_shallow_file(const char *path);\n+extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol);\n+extern void setup_alternate_shallow(struct lock_file *shallow_lock,\n+\t\t\t\t    const char **alternate_shallow_file);\n \n int is_descendant_of(struct commit *, struct commit_list *);\n int in_merge_bases(struct commit *, struct commit *);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 6684348..28195ed 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -184,36 +184,6 @@ static void consume_shallow_list(struct fetch_pack_args *args, int fd)\n \t}\n }\n \n-struct write_shallow_data {\n-\tstruct strbuf *out;\n-\tint use_pack_protocol;\n-\tint count;\n-};\n-\n-static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n-{\n-\tstruct write_shallow_data *data = cb_data;\n-\tconst char *hex = sha1_to_hex(graft->sha1);\n-\tdata->count++;\n-\tif (data->use_pack_protocol)\n-\t\tpacket_buf_write(data->out, \"shallow %s\", hex);\n-\telse {\n-\t\tstrbuf_addstr(data->out, hex);\n-\t\tstrbuf_addch(data->out, '\\n');\n-\t}\n-\treturn 0;\n-}\n-\n-static int write_shallow_commits(struct strbuf *out, int use_pack_protocol)\n-{\n-\tstruct write_shallow_data data;\n-\tdata.out = out;\n-\tdata.use_pack_protocol = use_pack_protocol;\n-\tdata.count = 0;\n-\tfor_each_commit_graft(write_one_shallow, &data);\n-\treturn data.count;\n-}\n-\n static enum ack_type get_ack(int fd, unsigned char *result_sha1)\n {\n \tint len;\n@@ -795,27 +765,6 @@ static int cmp_ref_by_name(const void *a_, const void *b_)\n \treturn strcmp(a->name, b->name);\n }\n \n-static void setup_alternate_shallow(void)\n-{\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tint fd;\n-\n-\tcheck_shallow_file_for_update();\n-\tfd = hold_lock_file_for_update(&shallow_lock, git_path(\"shallow\"),\n-\t\t\t\t       LOCK_DIE_ON_ERROR);\n-\tif (write_shallow_commits(&sb, 0)) {\n-\t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n-\t\t\tdie_errno(\"failed to write to %s\", shallow_lock.filename);\n-\t\talternate_shallow_file = shallow_lock.filename;\n-\t} else\n-\t\t/*\n-\t\t * is_repository_shallow() sees empty string as \"no\n-\t\t * shallow file\".\n-\t\t */\n-\t\talternate_shallow_file = \"\";\n-\tstrbuf_release(&sb);\n-}\n-\n static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\t\t\t int fd[2],\n \t\t\t\t const struct ref *orig_ref,\n@@ -896,7 +845,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tif (args->stateless_rpc)\n \t\tpacket_flush(fd[1]);\n \tif (args->depth > 0)\n-\t\tsetup_alternate_shallow();\n+\t\tsetup_alternate_shallow(&shallow_lock, &alternate_shallow_file);\n \tif (get_pack(args, fd, pack_lockfile))\n \t\tdie(\"git fetch-pack: fetch failed.\");\n \ndiff --git a/shallow.c b/shallow.c\nindex 8a9c96d..68dd106 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"commit.h\"\n #include \"tag.h\"\n+#include \"pkt-line.h\"\n \n static int is_shallow = -1;\n static struct stat shallow_stat;\n@@ -141,3 +142,56 @@ void check_shallow_file_for_update(void)\n \t\t   )\n \t\tdie(\"shallow file was changed during fetch\");\n }\n+\n+struct write_shallow_data {\n+\tstruct strbuf *out;\n+\tint use_pack_protocol;\n+\tint count;\n+};\n+\n+static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n+{\n+\tstruct write_shallow_data *data = cb_data;\n+\tconst char *hex = sha1_to_hex(graft->sha1);\n+\tdata->count++;\n+\tif (data->use_pack_protocol)\n+\t\tpacket_buf_write(data->out, \"shallow %s\", hex);\n+\telse {\n+\t\tstrbuf_addstr(data->out, hex);\n+\t\tstrbuf_addch(data->out, '\\n');\n+\t}\n+\treturn 0;\n+}\n+\n+int write_shallow_commits(struct strbuf *out, int use_pack_protocol)\n+{\n+\tstruct write_shallow_data data;\n+\tdata.out = out;\n+\tdata.use_pack_protocol = use_pack_protocol;\n+\tdata.count = 0;\n+\tfor_each_commit_graft(write_one_shallow, &data);\n+\treturn data.count;\n+}\n+\n+void setup_alternate_shallow(struct lock_file *shallow_lock,\n+\t\t\t     const char **alternate_shallow_file)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint fd;\n+\n+\tcheck_shallow_file_for_update();\n+\tfd = hold_lock_file_for_update(shallow_lock, git_path(\"shallow\"),\n+\t\t\t\t       LOCK_DIE_ON_ERROR);\n+\tif (write_shallow_commits(&sb, 0)) {\n+\t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n+\t\t\tdie_errno(\"failed to write to %s\",\n+\t\t\t\t  shallow_lock->filename);\n+\t\t*alternate_shallow_file = shallow_lock->filename;\n+\t} else\n+\t\t/*\n+\t\t * is_repository_shallow() sees empty string as \"no\n+\t\t * shallow file\".\n+\t\t */\n+\t\t*alternate_shallow_file = \"\";\n+\tstrbuf_release(&sb);\n+}\n-- \n1.8.2.82.gc24b958\n"},{"id":"225323","messageId":"1376646727-22318-2-git-send-email-pclouds@gmail.com","threadId":"34407","inReplyTo":"1376646727-22318-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/6] shallow: only add shallow graft points to new shallow file","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-16T09:52:03Z","receivedAt":"2013-08-16T09:52:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"for_each_commit_graft() goes through all graft points and shallow\nboudaries are just one special kind of grafting. If $GIT_DIR/shallow\nand $GIT_DIR/info/grafts are both present, write_shallow_commits may\ncatch both sets, accidentally turning some graft points to shallow\nboundaries. Don't do that.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n shallow.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/shallow.c b/shallow.c\nindex 68dd106..5f626c0 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -153,6 +153,8 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n {\n \tstruct write_shallow_data *data = cb_data;\n \tconst char *hex = sha1_to_hex(graft->sha1);\n+\tif (graft->nr_parent != -1)\n+\t\treturn 0;\n \tdata->count++;\n \tif (data->use_pack_protocol)\n \t\tpacket_buf_write(data->out, \"shallow %s\", hex);\n-- \n1.8.2.82.gc24b958\n"},{"id":"225324","messageId":"1376646727-22318-3-git-send-email-pclouds@gmail.com","threadId":"34407","inReplyTo":"1376646727-22318-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/6] shallow: add setup_temporary_shallow()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-16T09:52:04Z","receivedAt":"2013-08-16T09:52:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This function is like setup_alternate_shallow() except that it does\nnot lock $GIT_DIR/shallow. It's supposed to be used when a program\ngenerates temporary shallow for for use by another program, then throw\nthe shallow file away.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n commit.h  |  1 +\n shallow.c | 23 +++++++++++++++++++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/commit.h b/commit.h\nindex 790e31b..c4d324c 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -201,6 +201,7 @@ extern void set_alternate_shallow_file(const char *path);\n extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol);\n extern void setup_alternate_shallow(struct lock_file *shallow_lock,\n \t\t\t\t    const char **alternate_shallow_file);\n+extern char *setup_temporary_shallow(void);\n \n int is_descendant_of(struct commit *, struct commit_list *);\n int in_merge_bases(struct commit *, struct commit *);\ndiff --git a/shallow.c b/shallow.c\nindex 5f626c0..cdf37d6 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -175,6 +175,29 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol)\n \treturn data.count;\n }\n \n+char *setup_temporary_shallow(void)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint fd;\n+\n+\tif (write_shallow_commits(&sb, 0)) {\n+\t\tstruct strbuf path = STRBUF_INIT;\n+\t\tstrbuf_addstr(&path, git_path(\"shallow_XXXXXX\"));\n+\t\tfd = xmkstemp(path.buf);\n+\t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n+\t\t\tdie_errno(\"failed to write to %s\",\n+\t\t\t\t  path.buf);\n+\t\tclose(fd);\n+\t\tstrbuf_release(&sb);\n+\t\treturn strbuf_detach(&path, NULL);\n+\t}\n+\t/*\n+\t * is_repository_shallow() sees empty string as \"no shallow\n+\t * file\".\n+\t */\n+\treturn xstrdup(\"\");\n+}\n+\n void setup_alternate_shallow(struct lock_file *shallow_lock,\n \t\t\t     const char **alternate_shallow_file)\n {\n-- \n1.8.2.82.gc24b958\n"},{"id":"225325","messageId":"1376646727-22318-4-git-send-email-pclouds@gmail.com","threadId":"34407","inReplyTo":"1376646727-22318-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 4/6] upload-pack: delegate rev walking in shallow fetch to pack-objects","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-16T09:52:05Z","receivedAt":"2013-08-16T09:52:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"upload-pack has a special rev walking code for shallow recipients. It\nworks almost like the similar code in pack-objects except:\n\n1. in upload-pack, graft points could be added for deepening\n\n2. also when the repository is deepened, the shallow point will be\n   moved further away from the tip, but the old shallow point will be\n   marked as edge to produce more efficient packs. See 6523078 (make\n   shallow repository deepening more network efficient - 2009-09-03)\n\npass the file to pack-objects via --shallow-file. This will override\n$GIT_DIR/shallow and give pack-objects the exact repository shape that\nupload-pack has.\n\nmark edge commits by revision command arguments. Even if old shallow\npoints are passed as \"--not\" revisions as in this patch, they will not\nbe picked up by mark_edges_uninteresting() because this function looks\nup to parents for edges, while in this case the edge is the children,\nin the opposite direction. This will be fixed in the next patch when\nall given uninteresting commits are marked as edges.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n t/t5530-upload-pack-error.sh |   3 -\n upload-pack.c                | 128 +++++++++++--------------------------------\n 2 files changed, 32 insertions(+), 99 deletions(-)\n\ndiff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\nindex c983d36..3932e79 100755\n--- a/t/t5530-upload-pack-error.sh\n+++ b/t/t5530-upload-pack-error.sh\n@@ -54,9 +54,6 @@ test_expect_success 'upload-pack fails due to error in rev-list' '\n \tprintf \"0032want %s\\n0034shallow %s00000009done\\n0000\" \\\n \t\t$(git rev-parse HEAD) $(git rev-parse HEAD^) >input &&\n \ttest_must_fail git upload-pack . <input >/dev/null 2>output.err &&\n-\t# pack-objects survived\n-\tgrep \"Total.*, reused\" output.err &&\n-\t# but there was an error, which must have been in rev-list\n \tgrep \"bad tree object\" output.err\n '\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 127e59a..d5a003a 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -68,87 +68,28 @@ static ssize_t send_client_data(int fd, const char *data, ssize_t sz)\n \treturn sz;\n }\n \n-static FILE *pack_pipe = NULL;\n-static void show_commit(struct commit *commit, void *data)\n-{\n-\tif (commit->object.flags & BOUNDARY)\n-\t\tfputc('-', pack_pipe);\n-\tif (fputs(sha1_to_hex(commit->object.sha1), pack_pipe) < 0)\n-\t\tdie(\"broken output pipe\");\n-\tfputc('\\n', pack_pipe);\n-\tfflush(pack_pipe);\n-\tfree(commit->buffer);\n-\tcommit->buffer = NULL;\n-}\n-\n-static void show_object(struct object *obj,\n-\t\t\tconst struct name_path *path, const char *component,\n-\t\t\tvoid *cb_data)\n-{\n-\tshow_object_with_name(pack_pipe, obj, path, component);\n-}\n-\n-static void show_edge(struct commit *commit)\n-{\n-\tfprintf(pack_pipe, \"-%s\\n\", sha1_to_hex(commit->object.sha1));\n-}\n-\n-static int do_rev_list(int in, int out, void *user_data)\n-{\n-\tint i;\n-\tstruct rev_info revs;\n-\n-\tpack_pipe = xfdopen(out, \"w\");\n-\tinit_revisions(&revs, NULL);\n-\trevs.tag_objects = 1;\n-\trevs.tree_objects = 1;\n-\trevs.blob_objects = 1;\n-\tif (use_thin_pack)\n-\t\trevs.edge_hint = 1;\n-\n-\tfor (i = 0; i < want_obj.nr; i++) {\n-\t\tstruct object *o = want_obj.objects[i].item;\n-\t\t/* why??? */\n-\t\to->flags &= ~UNINTERESTING;\n-\t\tadd_pending_object(&revs, o, NULL);\n-\t}\n-\tfor (i = 0; i < have_obj.nr; i++) {\n-\t\tstruct object *o = have_obj.objects[i].item;\n-\t\to->flags |= UNINTERESTING;\n-\t\tadd_pending_object(&revs, o, NULL);\n-\t}\n-\tsetup_revisions(0, NULL, &revs, NULL);\n-\tif (prepare_revision_walk(&revs))\n-\t\tdie(\"revision walk setup failed\");\n-\tmark_edges_uninteresting(revs.commits, &revs, show_edge);\n-\tif (use_thin_pack)\n-\t\tfor (i = 0; i < extra_edge_obj.nr; i++)\n-\t\t\tfprintf(pack_pipe, \"-%s\\n\", sha1_to_hex(\n-\t\t\t\t\textra_edge_obj.objects[i].item->sha1));\n-\ttraverse_commit_list(&revs, show_commit, show_object, NULL);\n-\tfflush(pack_pipe);\n-\tfclose(pack_pipe);\n-\treturn 0;\n-}\n-\n static void create_pack_file(void)\n {\n-\tstruct async rev_list;\n \tstruct child_process pack_objects;\n \tchar data[8193], progress[128];\n \tchar abort_msg[] = \"aborting due to possible repository \"\n \t\t\"corruption on the remote side.\";\n \tint buffered = -1;\n \tssize_t sz;\n-\tconst char *argv[10];\n-\tint arg = 0;\n+\tconst char *argv[12];\n+\tint i, arg = 0;\n+\tFILE *pipe_fd;\n+\tchar *shallow_file = NULL;\n \n-\targv[arg++] = \"pack-objects\";\n-\tif (!shallow_nr) {\n-\t\targv[arg++] = \"--revs\";\n-\t\tif (use_thin_pack)\n-\t\t\targv[arg++] = \"--thin\";\n+\tif (shallow_nr) {\n+\t\tshallow_file = setup_temporary_shallow();\n+\t\targv[arg++] = \"--shallow-file\";\n+\t\targv[arg++] = shallow_file;\n \t}\n+\targv[arg++] = \"pack-objects\";\n+\targv[arg++] = \"--revs\";\n+\tif (use_thin_pack)\n+\t\targv[arg++] = \"--thin\";\n \n \targv[arg++] = \"--stdout\";\n \tif (!no_progress)\n@@ -169,29 +110,21 @@ 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-\tif (shallow_nr) {\n-\t\tmemset(&rev_list, 0, sizeof(rev_list));\n-\t\trev_list.proc = do_rev_list;\n-\t\trev_list.out = pack_objects.in;\n-\t\tif (start_async(&rev_list))\n-\t\t\tdie(\"git upload-pack: unable to fork git-rev-list\");\n-\t}\n-\telse {\n-\t\tFILE *pipe_fd = xfdopen(pack_objects.in, \"w\");\n-\t\tint i;\n-\n-\t\tfor (i = 0; i < want_obj.nr; i++)\n-\t\t\tfprintf(pipe_fd, \"%s\\n\",\n-\t\t\t\tsha1_to_hex(want_obj.objects[i].item->sha1));\n-\t\tfprintf(pipe_fd, \"--not\\n\");\n-\t\tfor (i = 0; i < have_obj.nr; i++)\n-\t\t\tfprintf(pipe_fd, \"%s\\n\",\n-\t\t\t\tsha1_to_hex(have_obj.objects[i].item->sha1));\n-\t\tfprintf(pipe_fd, \"\\n\");\n-\t\tfflush(pipe_fd);\n-\t\tfclose(pipe_fd);\n-\t}\n-\n+\tpipe_fd = xfdopen(pack_objects.in, \"w\");\n+\n+\tfor (i = 0; i < want_obj.nr; i++)\n+\t\tfprintf(pipe_fd, \"%s\\n\",\n+\t\t\tsha1_to_hex(want_obj.objects[i].item->sha1));\n+\tfprintf(pipe_fd, \"--not\\n\");\n+\tfor (i = 0; i < have_obj.nr; i++)\n+\t\tfprintf(pipe_fd, \"%s\\n\",\n+\t\t\tsha1_to_hex(have_obj.objects[i].item->sha1));\n+\tfor (i = 0; i < extra_edge_obj.nr; i++)\n+\t\tfprintf(pipe_fd, \"%s\\n\",\n+\t\t\tsha1_to_hex(extra_edge_obj.objects[i].item->sha1));\n+\tfprintf(pipe_fd, \"\\n\");\n+\tfflush(pipe_fd);\n+\tfclose(pipe_fd);\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@@ -290,8 +223,11 @@ 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 (shallow_nr && finish_async(&rev_list))\n-\t\tgoto fail;\t/* error was already reported */\n+\tif (shallow_file) {\n+\t\tif (*shallow_file)\n+\t\t\tunlink(shallow_file);\n+\t\tfree(shallow_file);\n+\t}\n \n \t/* flush the data */\n \tif (0 <= buffered) {\n-- \n1.8.2.82.gc24b958\n"},{"id":"225326","messageId":"1376646727-22318-5-git-send-email-pclouds@gmail.com","threadId":"34407","inReplyTo":"1376646727-22318-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 5/6] list-objects: reduce one argument in mark_edges_uninteresting","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-16T09:52:06Z","receivedAt":"2013-08-16T09:52:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"mark_edges_uninteresting() is always called with this form\n\n  mark_edges_uninteresting(revs->commits, revs, ...);\n\nRemove the first argument and let mark_edges_uninteresting figure that\nout by itself. It helps answer the question \"are this commit list and\nrevs related in any way?\" when looking at mark_edges_uninteresting\nimplementation.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n bisect.c               | 2 +-\n builtin/pack-objects.c | 2 +-\n builtin/rev-list.c     | 2 +-\n http-push.c            | 2 +-\n list-objects.c         | 7 +++----\n list-objects.h         | 2 +-\n 6 files changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 71c1958..1e46a4f 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -624,7 +624,7 @@ static void bisect_common(struct rev_info *revs)\n \tif (prepare_revision_walk(revs))\n \t\tdie(\"revision walk setup failed\");\n \tif (revs->tree_objects)\n-\t\tmark_edges_uninteresting(revs->commits, revs, NULL);\n+\t\tmark_edges_uninteresting(revs, NULL);\n }\n \n static void exit_if_skipped_commits(struct commit_list *tried,\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f069462..dd117b3 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2378,7 +2378,7 @@ static void get_object_list(int ac, const char **av)\n \n \tif (prepare_revision_walk(&revs))\n \t\tdie(\"revision walk setup failed\");\n-\tmark_edges_uninteresting(revs.commits, &revs, show_edge);\n+\tmark_edges_uninteresting(&revs, show_edge);\n \ttraverse_commit_list(&revs, show_commit, show_object, NULL);\n \n \tif (keep_unreachable)\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex a5ec30d..4fc1616 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -336,7 +336,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tif (prepare_revision_walk(&revs))\n \t\tdie(\"revision walk setup failed\");\n \tif (revs.tree_objects)\n-\t\tmark_edges_uninteresting(revs.commits, &revs, show_edge);\n+\t\tmark_edges_uninteresting(&revs, show_edge);\n \n \tif (bisect_list) {\n \t\tint reaches = reaches, all = all;\ndiff --git a/http-push.c b/http-push.c\nindex 6dad188..cde6416 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1976,7 +1976,7 @@ int main(int argc, char **argv)\n \t\tpushing = 0;\n \t\tif (prepare_revision_walk(&revs))\n \t\t\tdie(\"revision walk setup failed\");\n-\t\tmark_edges_uninteresting(revs.commits, &revs, NULL);\n+\t\tmark_edges_uninteresting(&revs, NULL);\n \t\tobjects_to_send = get_delta(&revs, ref_lock);\n \t\tfinish_all_active_slots();\n \ndiff --git a/list-objects.c b/list-objects.c\nindex 3dd4a96..db8ee4f 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -145,11 +145,10 @@ static void mark_edge_parents_uninteresting(struct commit *commit,\n \t}\n }\n \n-void mark_edges_uninteresting(struct commit_list *list,\n-\t\t\t      struct rev_info *revs,\n-\t\t\t      show_edge_fn show_edge)\n+void mark_edges_uninteresting(struct rev_info *revs, show_edge_fn show_edge)\n {\n-\tfor ( ; list; list = list->next) {\n+\tstruct commit_list *list;\n+\tfor (list = revs->commits; list; list = list->next) {\n \t\tstruct commit *commit = list->item;\n \n \t\tif (commit->object.flags & UNINTERESTING) {\ndiff --git a/list-objects.h b/list-objects.h\nindex 3db7bb6..136a1da 100644\n--- a/list-objects.h\n+++ b/list-objects.h\n@@ -6,6 +6,6 @@ typedef void (*show_object_fn)(struct object *, const struct name_path *, const\n void traverse_commit_list(struct rev_info *, show_commit_fn, show_object_fn, void *);\n \n typedef void (*show_edge_fn)(struct commit *);\n-void mark_edges_uninteresting(struct commit_list *, struct rev_info *, show_edge_fn);\n+void mark_edges_uninteresting(struct rev_info *, show_edge_fn);\n \n #endif\n-- \n1.8.2.82.gc24b958\n"},{"id":"225327","messageId":"1376646727-22318-6-git-send-email-pclouds@gmail.com","threadId":"34407","inReplyTo":"1376646727-22318-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 6/6] list-objects: mark more commits as edges in mark_edges_uninteresting","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-16T09:52:07Z","receivedAt":"2013-08-16T09:52:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The purpose of edge commits is to let pack-objects know what objects\nit can use as base, but does not need to include in the thin pack\nbecause the other side is supposed to already have them. So far we\nmark uninteresting parents of interesting commits as edges. But even\nan unrelated uninteresting commit (that the other side has) may become\na good base for pack-objects and help produce more efficient packs.\n\nThis is especially true for shallow clone, when the client issues a\nfetch with a depth smaller or equal to the number of commits the\nserver is ahead of the client. For example, in this commit history the\nclient has up to \"A\" and the server has up to \"B\":\n\n    -------A---B\n     have--^   ^\n              /\n       want--+\n\nIf depth 1 is requested, the commit list to send to the client\nincludes only B. The way m_e_u is working, it checks if parent commits\nof B are uninteresting, if so mark them as edges. Due to shallow\neffect, commit B is grafted to have no parents and the revision walker\nnever sees A as the parent of B. In fact it marks no edges at all in\nthis simple case and sends everything B has to the client even if it\ncould have excluded what A and also the client already have. In a\nslightly different case where A is not a direct parent of B (iow there\nare commits in between A and B), marking A as an edge can still save\nsome because B may still have stuff from the far ancestor A.\n\nThere is another case from the previous patch, when we deepen a ref\nfrom C->E to A->E:\n\n    ---A---B   C---D---E\n     want--^   ^       ^\n       shallow-+      /\n          have-------+\n\nIn this case we need to send A and B to the client, and C (i.e. the\ncurrent shallow point that the client informs the server) is a very\ngood base because it's closet to A and B. Normal m_e_u won't recognize\nC as an edge because it only looks back to parents (i.e. A<-B) not the\nopposite way B->C even if C is already marked as uninteresting commit\nby the previous patch.\n\nThis patch includes all uninteresting commits from command line as\nedges and lets pack-objects decide what's best to do. The upside is we\nhave better chance of producing better packs in certain cases. The\ndownside is we may need to process some extra objects on the server\nside.\n\nFor the shallow case on git.git, when the client is 5 commits behind\nand does \"fetch --depth=3\", the result pack is 99.26 KiB instead of\n4.92 MiB.\n\nReported-and-analyzed-by: Matthijs Kooijman <matthijs@stdin.nl>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n list-objects.c | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex db8ee4f..05c8c5c 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -148,15 +148,32 @@ static void mark_edge_parents_uninteresting(struct commit *commit,\n void mark_edges_uninteresting(struct rev_info *revs, show_edge_fn show_edge)\n {\n \tstruct commit_list *list;\n+\tint i;\n+\n \tfor (list = revs->commits; list; list = list->next) {\n \t\tstruct commit *commit = list->item;\n \n \t\tif (commit->object.flags & UNINTERESTING) {\n \t\t\tmark_tree_uninteresting(commit->tree);\n+\t\t\tif (revs->edge_hint && !(commit->object.flags & SHOWN)) {\n+\t\t\t\tcommit->object.flags |= SHOWN;\n+\t\t\t\tshow_edge(commit);\n+\t\t\t}\n \t\t\tcontinue;\n \t\t}\n \t\tmark_edge_parents_uninteresting(commit, revs, show_edge);\n \t}\n+\tfor (i = 0; i < revs->cmdline.nr; i++) {\n+\t\tstruct object *obj = revs->cmdline.rev[i].item;\n+\t\tstruct commit *commit = (struct commit *)obj;\n+\t\tif (obj->type != OBJ_COMMIT || !(obj->flags & UNINTERESTING))\n+\t\t\tcontinue;\n+\t\tmark_tree_uninteresting(commit->tree);\n+\t\tif (revs->edge_hint && !(obj->flags & SHOWN)) {\n+\t\t\tobj->flags |= SHOWN;\n+\t\t\tshow_edge(commit);\n+\t\t}\n+\t}\n }\n \n static void add_pending_tree(struct rev_info *revs, struct tree *tree)\n-- \n1.8.2.82.gc24b958\n"},{"id":"225359","messageId":"CAPig+cS1y5cuM6zg0k=rKF3O28krdoNe4ghKtttZt74DENJB+g@mail.gmail.com","threadId":"34407","inReplyTo":"1376646727-22318-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/6] shallow: only add shallow graft points to new shallow file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-08-16T23:50:50Z","receivedAt":"2013-08-16T23:50:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 16, 2013 at 5:52 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:\n> for_each_commit_graft() goes through all graft points and shallow\n> boudaries are just one special kind of grafting. If $GIT_DIR/shallow\n\ns/boudaries/boundaries/\n\n> and $GIT_DIR/info/grafts are both present, write_shallow_commits may\n> catch both sets, accidentally turning some graft points to shallow\n> boundaries. Don't do that.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n"},{"id":"225360","messageId":"CAPig+cQxq2B-zJeFP8p=8yb8Po7LX4_ZWsAZy=jJdHF7f5PN8A@mail.gmail.com","threadId":"34407","inReplyTo":"1376646727-22318-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 3/6] shallow: add setup_temporary_shallow()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-08-16T23:52:04Z","receivedAt":"2013-08-16T23:52:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 16, 2013 at 5:52 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:\n> This function is like setup_alternate_shallow() except that it does\n> not lock $GIT_DIR/shallow. It's supposed to be used when a program\n> generates temporary shallow for for use by another program, then throw\n\ns/for for/for/\n\n> the shallow file away.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n"},{"id":"226089","messageId":"20130828145225.GE10217@login.drsnuggles.stderr.nl","threadId":"34407","inReplyTo":"1376646727-22318-4-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 4/6] upload-pack: delegate rev walking in shallow fetch to pack-objects","fromName":"Matthijs Kooijman","fromEmail":"matthijs@stdin.nl","sentAt":"2013-08-28T14:52:25Z","receivedAt":"2013-08-28T14:52:25Z","isPatch":true,"sender":{"key":"matthijs@stdin.nl","avatar":"https://avatars.githubusercontent.com/u/194491?v=4"},"body":"Hi Nguy,\n\nOn Fri, Aug 16, 2013 at 04:52:05PM +0700, Nguyễn Thái Ngọc Duy wrote:\n> upload-pack has a special rev walking code for shallow recipients. It\n> works almost like the similar code in pack-objects except:\n> \n> 1. in upload-pack, graft points could be added for deepening\n> \n> 2. also when the repository is deepened, the shallow point will be\n>    moved further away from the tip, but the old shallow point will be\n>    marked as edge to produce more efficient packs. See 6523078 (make\n>    shallow repository deepening more network efficient - 2009-09-03)\n> \n> pass the file to pack-objects via --shallow-file. This will override\n> $GIT_DIR/shallow and give pack-objects the exact repository shape that\n> upload-pack has.\n> \n> mark edge commits by revision command arguments. Even if old shallow\n> points are passed as \"--not\" revisions as in this patch, they will not\n> be picked up by mark_edges_uninteresting() because this function looks\n> up to parents for edges, while in this case the edge is the children,\n> in the opposite direction. This will be fixed in the next patch when\n> all given uninteresting commits are marked as edges.\nThis says \"the next patch\" but it really refers to 6/6, not 5/6. Patch\n6/6 has the same problem (it says \"previous patch\"). Perhaps patches 4\nand 5 should just be swapped?\n\nGr.\n\nMatthijs\n"},{"id":"226090","messageId":"20130828153638.GF10217@login.drsnuggles.stderr.nl","threadId":"34407","inReplyTo":"CACsJy8CDGgKftp0iBB8MYjMawKhxZ1JQ+xAYb0itpaCOjFHWxg@mail.gmail.com","subject":"Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","fromName":"Matthijs Kooijman","fromEmail":"matthijs@stdin.nl","sentAt":"2013-08-28T15:36:38Z","receivedAt":"2013-08-28T15:36:38Z","isPatch":true,"sender":{"key":"matthijs@stdin.nl","avatar":"https://avatars.githubusercontent.com/u/194491?v=4"},"body":"Hi Duy,\n\n> I thought a bit but my thoughts often get stuck if I don't write them\n> down in form of code :-) so this is what I got so far. 4/6 is a good\n> thing in my opinion, but I might overlook something 6/6  is about this\n> thread.\n\nThe series looks good to me, though I don't know enough about the code\nto do detailed analysis.\n\nIn any case, I agree that 4/6 is a good change, it removes a bunch of\nsimilar code for the shallow special case (which is now no longer a\ncompletely separate special case).\n\nThe total series also seems to actually fix the problem I reported. I'll\nresend the testcase from my original patch as well, which now passes\nwith your series applied.\n\nThanks for diving into this!\n\nGr.\n\nMatthijs\n"},{"id":"226092","messageId":"1377705722-17053-1-git-send-email-matthijs@stdin.nl","threadId":"34407","inReplyTo":"20130828153638.GF10217@login.drsnuggles.stderr.nl","subject":"[PATCH] Add testcase for needless objects during a shallow fetch","fromName":"Matthijs Kooijman","fromEmail":"matthijs@stdin.nl","sentAt":"2013-08-28T16:02:02Z","receivedAt":"2013-08-28T16:02:02Z","isPatch":true,"sender":{"key":"matthijs@stdin.nl","avatar":"https://avatars.githubusercontent.com/u/194491?v=4"},"body":"This is a testcase that checks for a problem where, during a specific\nshallow fetch where the client does not have any commits that are a\nsuccessor of the new shallow root (i.e., the fetch creates a new\ndetached piece of history), the server would simply send over _all_\nobjects, instead of taking into account the objects already present in\nthe client.\n\nThe actual problem was fixed by a recent patch series by Nguyễn Thái\nNgọc Duy already.\n\nSigned-off-by: Matthijs Kooijman <matthijs@stdin.nl>\n---\n t/t5500-fetch-pack.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex fd2598e..a022d65 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -393,6 +393,17 @@ test_expect_success 'fetch in shallow repo unreachable shallow objects' '\n \t\tgit fsck --no-dangling\n \t)\n '\n+test_expect_success 'fetch creating new shallow root' '\n+\t(\n+\t\tgit clone \"file://$(pwd)/.\" shallow10 &&\n+\t\tgit commit --allow-empty -m empty &&\n+\t\tcd shallow10 &&\n+\t\tgit fetch --depth=1 --progress 2> actual &&\n+\t\t# This should fetch only the empty commit, no tree or\n+\t\t# blob objects\n+\t\tgrep \"remote: Total 1\" actual\n+\t)\n+'\n \n test_expect_success 'setup tests for the --stdin parameter' '\n \tfor head in C D E F\n-- \n1.8.4.rc1\n"},{"id":"226147","messageId":"CACsJy8BMQ=k_W12OhJH8Pod6g-eynVOuTULauBqeERbT1uXdSA@mail.gmail.com","threadId":"34407","inReplyTo":"20130828145225.GE10217@login.drsnuggles.stderr.nl","subject":"Re: [PATCH 4/6] upload-pack: delegate rev walking in shallow fetch to pack-objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-29T09:48:56Z","receivedAt":"2013-08-29T09:48:56Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 28, 2013 at 9:52 PM, Matthijs Kooijman <matthijs@stdin.nl> wrote:\n> Hi Nguy,\n>\n> On Fri, Aug 16, 2013 at 04:52:05PM +0700, Nguyễn Thái Ngọc Duy wrote:\n>> upload-pack has a special rev walking code for shallow recipients. It\n>> works almost like the similar code in pack-objects except:\n>>\n>> 1. in upload-pack, graft points could be added for deepening\n>>\n>> 2. also when the repository is deepened, the shallow point will be\n>>    moved further away from the tip, but the old shallow point will be\n>>    marked as edge to produce more efficient packs. See 6523078 (make\n>>    shallow repository deepening more network efficient - 2009-09-03)\n>>\n>> pass the file to pack-objects via --shallow-file. This will override\n>> $GIT_DIR/shallow and give pack-objects the exact repository shape that\n>> upload-pack has.\n>>\n>> mark edge commits by revision command arguments. Even if old shallow\n>> points are passed as \"--not\" revisions as in this patch, they will not\n>> be picked up by mark_edges_uninteresting() because this function looks\n>> up to parents for edges, while in this case the edge is the children,\n>> in the opposite direction. This will be fixed in the next patch when\n>> all given uninteresting commits are marked as edges.\n> This says \"the next patch\" but it really refers to 6/6, not 5/6. Patch\n> 6/6 has the same problem (it says \"previous patch\"). Perhaps patches 4\n> and 5 should just be swapped?\n\nYeah. I guess I reordered the patches before sending out and forgot\nthat the commit message needs a special order. Wil do.\n-- \nDuy\n"},{"id":"226148","messageId":"CACsJy8BDxkpFG=nfVENeAHMyhdokwvbpxu26m0RtHou_WK2Mkw@mail.gmail.com","threadId":"34407","inReplyTo":"1377705722-17053-1-git-send-email-matthijs@stdin.nl","subject":"Re: [PATCH] Add testcase for needless objects during a shallow fetch","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-29T09:50:40Z","receivedAt":"2013-08-29T09:50:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 28, 2013 at 11:02 PM, Matthijs Kooijman <matthijs@stdin.nl> wrote:\n> This is a testcase that checks for a problem where, during a specific\n> shallow fetch where the client does not have any commits that are a\n> successor of the new shallow root (i.e., the fetch creates a new\n> detached piece of history), the server would simply send over _all_\n> objects, instead of taking into account the objects already present in\n> the client.\n\nThanks. This reminds me I should add a test case in the 4/6 to\ndemonstrate the regression and let it verify again in 6/6 that the\ntemporary regression is gone. Will reroll the series with your patch\nincluded.\n\n>\n> The actual problem was fixed by a recent patch series by Nguyễn Thái\n> Ngọc Duy already.\n>\n> Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>\n> ---\n>  t/t5500-fetch-pack.sh | 11 +++++++++++\n>  1 file changed, 11 insertions(+)\n>\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index fd2598e..a022d65 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -393,6 +393,17 @@ test_expect_success 'fetch in shallow repo unreachable shallow objects' '\n>                 git fsck --no-dangling\n>         )\n>  '\n> +test_expect_success 'fetch creating new shallow root' '\n> +       (\n> +               git clone \"file://$(pwd)/.\" shallow10 &&\n> +               git commit --allow-empty -m empty &&\n> +               cd shallow10 &&\n> +               git fetch --depth=1 --progress 2> actual &&\n> +               # This should fetch only the empty commit, no tree or\n> +               # blob objects\n> +               grep \"remote: Total 1\" actual\n> +       )\n> +'\n>\n>  test_expect_success 'setup tests for the --stdin parameter' '\n>         for head in C D E F\n> --\n> 1.8.4.rc1\n>\n\n\n\n-- \nDuy\n"},{"id":"226390","messageId":"CACsJy8Dv7tVG_oWcPvNTy-zxD7axZxoXHcE1=TFwTv9+wGFCOQ@mail.gmail.com","threadId":"34407","inReplyTo":"CACsJy8BDxkpFG=nfVENeAHMyhdokwvbpxu26m0RtHou_WK2Mkw@mail.gmail.com","subject":"Re: [PATCH] Add testcase for needless objects during a shallow fetch","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-31T01:25:42Z","receivedAt":"2013-08-31T01:25:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 29, 2013 at 4:50 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n> On Wed, Aug 28, 2013 at 11:02 PM, Matthijs Kooijman <matthijs@stdin.nl> wrote:\n>> This is a testcase that checks for a problem where, during a specific\n>> shallow fetch where the client does not have any commits that are a\n>> successor of the new shallow root (i.e., the fetch creates a new\n>> detached piece of history), the server would simply send over _all_\n>> objects, instead of taking into account the objects already present in\n>> the client.\n>\n> Thanks. This reminds me I should add a test case in the 4/6 to\n> demonstrate the regression and let it verify again in 6/6 that the\n> temporary regression is gone. Will reroll the series with your patch\n> included.\n\nNo. It's too hard. The difference is what base a delta object use and\nchecking that might not be entirely reliable because the algorithm in\npack-objects might change some day.\n-- \nDuy\n"},{"id":"229243","messageId":"20131021075139.GA15425@login.drsnuggles.stderr.nl","threadId":"34407","inReplyTo":"CACsJy8CDGgKftp0iBB8MYjMawKhxZ1JQ+xAYb0itpaCOjFHWxg@mail.gmail.com","subject":"Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","fromName":"Matthijs Kooijman","fromEmail":"matthijs@stdin.nl","sentAt":"2013-10-21T07:51:39Z","receivedAt":"2013-10-21T07:51:39Z","isPatch":true,"sender":{"key":"matthijs@stdin.nl","avatar":"https://avatars.githubusercontent.com/u/194491?v=4"},"body":"Hi Duy,\n\nI saw your patch series got accepted in git master a while back, great!\nSince I hope to be using the fixed behaviour soon, what was the plan for\nincluding it? Am I correct in thinking that git master will become 1.8.5\nin a while? Would this series perhaps be considered for backporting to\n1.8.4.x?\n\nGr.\n\nMatthijs\n"},{"id":"229563","messageId":"CACsJy8DXH2verOjq670wzT+wkhQPZaqf68-rM91JJnVt2G=pBg@mail.gmail.com","threadId":"34407","inReplyTo":"20131021075139.GA15425@login.drsnuggles.stderr.nl","subject":"Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-10-26T10:49:18Z","receivedAt":"2013-10-26T10:49:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Oct 21, 2013 at 2:51 PM, Matthijs Kooijman <matthijs@stdin.nl> wrote:\n> Hi Duy,\n>\n> I saw your patch series got accepted in git master a while back, great!\n> Since I hope to be using the fixed behaviour soon, what was the plan for\n> including it? Am I correct in thinking that git master will become 1.8.5\n> in a while? Would this series perhaps be considered for backporting to\n> 1.8.4.x?\n\nI was waiting for Junio to answer this as I rarely run released\nversions and do not care much about releases. I think normally master\nwill be cut for the next release (1.8.5?), maint branches have\nbackported bug fixes. I consider this an improvement rather than bug\nfix. So my guess is it will not be back ported to 1.8.4.x.\n\n>\n> Gr.\n>\n> Matthijs\n>\n> -----BEGIN PGP SIGNATURE-----\n> Version: GnuPG v1.4.9 (GNU/Linux)\n>\n> iEYEARECAAYFAlJk3QsACgkQz0nQ5oovr7wVOwCgvQCmB4IJ6X86727/5Kslg83G\n> A4UAoI8fBIXGnE1PwtwqFk/Od697dgNM\n> =rjMT\n> -----END PGP SIGNATURE-----\n>\n\n\n\n-- \nDuy\n"}]}