{"thread":{"id":"51252","subject":"Is --filter-print-omitted correct/used/needed?","startedAt":"2019-06-06T19:02:30Z","lastAt":"2019-06-07T21:10:09Z","messageCount":4,"participants":["Emily Shaffer","Christian Couder","Jeff Hostetler"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"376778","messageId":"CAJoAoZktk-w40MEsRZesvkX23Xn2uMUPiO_cMHa=-xfdyVHCWg@mail.gmail.com","threadId":"51252","inReplyTo":null,"subject":"Is --filter-print-omitted correct/used/needed?","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-06-06T19:02:17Z","receivedAt":"2019-06-06T19:02:30Z","isPatch":false,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Yesterday while down a rabbit hole, I discovered an interesting thing\nabout the 'omitted' object in many filters in\nlist-objects-filter-options.h.  It appears that when we call\nlist-objects.h:traverse_commit_list_filtered() with a non-NULL\n'omitted' argument, we still perform a walk of all objects - that is,\nthe filtered walk is no more efficient than the unfiltered walk from\nthe same place via 'traverse_commit_list()'. I verified by calling\neach and counting the objects:\n\n161         if (0) {\n162                 /* Unfiltered: */\n163                 printf(_(\"Unfiltered object walk.\\n\"));\n164                 traverse_commit_list(rev, walken_show_commit,\n165                                 walken_show_object, NULL);\n166         } else {\n167                 printf(_(\"Filtered object walk with filterspec\n'tree:1'.\\n\"));\n168                 /*\n169                  * We can parse a tree depth of 1 to demonstrate\nthe kind of\n170                  * filtering that could occur eg during shallow\ncloning.\n171                  */\n172                 parse_list_objects_filter(&filter_options,\n\"tree:1\");\n173\n174                 traverse_commit_list_filtered(&filter_options,\nrev,\n175                         walken_show_commit, walken_show_object,\nNULL, &omitted);\n176         }\n177\n178         /* Count the omitted objects. */\n179         oidset_iter_init(&omitted, &oit);\n180\n181         while ((oid = oidset_iter_next(&oit)))\n182                 omitted_count++;\n183\n184         printf(_(\"Object walk completed. Found %d commits, %d\nblobs, %d tags, \"\n185                \"and %d trees; %d omitted objects.\\n\"),\ncommit_count,\n186                blob_count, tag_count, tree_count, omitted_count);\n\nI found that omitted_count was always equal to the difference between\nsum(blob_count, tag_count, tree_count, commit_count) in the unfiltered\nand filtered walks. I also found that the length of time required to\nperform the unfiltered walk and the filtered-with-non-NULL-omitted\nwalk was the same, while the time required to perform the filtered\nwalk with NULL omitted was significantly shorter. (The walk in\nquestion was over the latest release of Git master, plus the ten or so\ncommits in my feature branch.)\n\nI was surprised! I figured that with filter \"tree:1\" that \"omitted\"\nwould contain only the objects on the \"border\" of the filter - that\nis, I assumed it would contain the blobs and trees in the root git\ndir, but none of those trees' blobs and trees. After talking with\njrnieder at length, it sounds like neither of us were clear on why\nthis \"omitted\" list would be needed beyond the initial development\nstage of a new filter... Jonathan's impression was also that if we do\nneed the \"omitted\" list, it may be a bug that we're still traversing\nobjects which are only reachable from objects already omitted.\n\nI grepped the Git source and found that we only provide a non-NULL\n\"omitted\" when someone calls \"git rev-list --filter-print-omitted\",\nwhich we verify with a simple test case for \"blobs:none\", in which\ncase the \"border\" objects which were omitted must be the same as all\nobjects which were omitted (since blobs aren't pointing to anything\nelse). I think if we had written a similar test case with some trees\nwe expect to omit we might have noticed sooner.\n\nSince I was already in the rabbit hole, out of curiosity I did a\nsearch on Github and only found one project referencing\n--filter-print-omitted which wasn't a mirror of Git:\nhttps://github.com/search?l=Python&q=%22--filter-print-omitted%22&type=Code\n\nSo, what do we use --filter-print-omitted for? Is anybody needing it?\nOr do we just use it to verify this one test case? Should we fix it,\nor get rid of it, or neither?\n\n - Emily\n"},{"id":"376812","messageId":"CAP8UFD2Kyq9nrAnoj3AxQyLgiGHycLEdJbJOQx_stsepsqmX4g@mail.gmail.com","threadId":"51252","inReplyTo":"CAJoAoZktk-w40MEsRZesvkX23Xn2uMUPiO_cMHa=-xfdyVHCWg@mail.gmail.com","subject":"Re: Is --filter-print-omitted correct/used/needed?","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-06-07T06:38:34Z","receivedAt":"2019-06-07T06:38:49Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Jun 6, 2019 at 10:18 PM Emily Shaffer <emilyshaffer@google.com> wrote:\n\n\n\n> I grepped the Git source and found that we only provide a non-NULL\n> \"omitted\" when someone calls \"git rev-list --filter-print-omitted\",\n> which we verify with a simple test case for \"blobs:none\", in which\n> case the \"border\" objects which were omitted must be the same as all\n> objects which were omitted (since blobs aren't pointing to anything\n> else). I think if we had written a similar test case with some trees\n> we expect to omit we might have noticed sooner.\n\nIt seems that --filter-print-omitted was introduced in caf3827e2f\n(rev-list: add list-objects filtering support, 2017-11-21) so I cc'ed\nJeff.\n\n[...]\n\n> So, what do we use --filter-print-omitted for? Is anybody needing it?\n> Or do we just use it to verify this one test case? Should we fix it,\n> or get rid of it, or neither?\n\nIn caf3827e2f there is:\n\n    This patch introduces handling of missing objects to help\n    debugging and development of the \"partial clone\" mechanism,\n    and once the mechanism is implemented, for a power user to\n    perform operations that are missing-object aware without\n    incurring the cost of checking if a missing link is expected.\n\nSo I would say that if you think that --filter-print-omitted doesn't\nhelp in debugging or development, and can even be confusing, and that\nit also doesn't help performance for power users or anyone else, then\nit would make sense to remove it, unless you find a way to make it\nfulfill its original goals, or maybe other worthwhile goals.\n"},{"id":"376847","messageId":"e9017ba5-5b52-9e50-96ff-743d7e2ff4df@jeffhostetler.com","threadId":"51252","inReplyTo":"CAP8UFD2Kyq9nrAnoj3AxQyLgiGHycLEdJbJOQx_stsepsqmX4g@mail.gmail.com","subject":"Re: Is --filter-print-omitted correct/used/needed?","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2019-06-07T18:57:01Z","receivedAt":"2019-06-07T18:57:05Z","isPatch":false,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 6/7/2019 2:38 AM, Christian Couder wrote:\n> On Thu, Jun 6, 2019 at 10:18 PM Emily Shaffer <emilyshaffer@google.com> wrote:\n> \n> \n> \n>> I grepped the Git source and found that we only provide a non-NULL\n>> \"omitted\" when someone calls \"git rev-list --filter-print-omitted\",\n>> which we verify with a simple test case for \"blobs:none\", in which\n>> case the \"border\" objects which were omitted must be the same as all\n>> objects which were omitted (since blobs aren't pointing to anything\n>> else). I think if we had written a similar test case with some trees\n>> we expect to omit we might have noticed sooner.\n> \n> It seems that --filter-print-omitted was introduced in caf3827e2f\n> (rev-list: add list-objects filtering support, 2017-11-21) so I cc'ed\n> Jeff.\n> \n> [...]\n\nThe --filter-print-omitted was intended to print the complete list\nof omitted objects.  For example, a packfile built from a filtered\ncommand and a packfile build from the unfiltered command would differ\nby exactly that set of objects.\n\nSo the discrepancy reported by the tree:1 example is incorrect.\nThe omitted set is the full set, not the frontier.  So when\n--filter-print-omitted is used, we still have to do the full tree walk.\nWhen not specified, we do get the perf boost because we can terminate\nthe tree walk early.\n\n\n>> So, what do we use --filter-print-omitted for? Is anybody needing it?\n>> Or do we just use it to verify this one test case? Should we fix it,\n>> or get rid of it, or neither?\n>\n> In caf3827e2f there is:\n> \n>      This patch introduces handling of missing objects to help\n>      debugging and development of the \"partial clone\" mechanism,\n>      and once the mechanism is implemented, for a power user to\n>      perform operations that are missing-object aware without\n>      incurring the cost of checking if a missing link is expected.\n> \n> So I would say that if you think that --filter-print-omitted doesn't\n> help in debugging or development, and can even be confusing, and that\n> it also doesn't help performance for power users or anyone else, then\n> it would make sense to remove it, unless you find a way to make it\n> fulfill its original goals, or maybe other worthwhile goals.\n\nI don't currently have a use for that (other than the existing test\ncases), but we could use that in the future as a guide for the server\nto put the omitted objects on a CDN, for example.\n\nSo I'd say let's leave it as is for now.\n\n\nJeff\n\n\n\n"},{"id":"376849","messageId":"20190607211001.GA197548@google.com","threadId":"51252","inReplyTo":"e9017ba5-5b52-9e50-96ff-743d7e2ff4df@jeffhostetler.com","subject":"Re: Is --filter-print-omitted correct/used/needed?","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-06-07T21:10:01Z","receivedAt":"2019-06-07T21:10:09Z","isPatch":false,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, Jun 07, 2019 at 02:57:01PM -0400, Jeff Hostetler wrote:\n> \n> \n> On 6/7/2019 2:38 AM, Christian Couder wrote:\n> > On Thu, Jun 6, 2019 at 10:18 PM Emily Shaffer <emilyshaffer@google.com> wrote:\n> > \n> > \n> > \n> > > I grepped the Git source and found that we only provide a non-NULL\n> > > \"omitted\" when someone calls \"git rev-list --filter-print-omitted\",\n> > > which we verify with a simple test case for \"blobs:none\", in which\n> > > case the \"border\" objects which were omitted must be the same as all\n> > > objects which were omitted (since blobs aren't pointing to anything\n> > > else). I think if we had written a similar test case with some trees\n> > > we expect to omit we might have noticed sooner.\n> > \n> > It seems that --filter-print-omitted was introduced in caf3827e2f\n> > (rev-list: add list-objects filtering support, 2017-11-21) so I cc'ed\n> > Jeff.\n> > \n> > [...]\n> \n> The --filter-print-omitted was intended to print the complete list\n> of omitted objects.  For example, a packfile built from a filtered\n> command and a packfile build from the unfiltered command would differ\n> by exactly that set of objects.\n> \n> So the discrepancy reported by the tree:1 example is incorrect.\n> The omitted set is the full set, not the frontier.  So when\n> --filter-print-omitted is used, we still have to do the full tree walk.\n> When not specified, we do get the perf boost because we can terminate\n> the tree walk early.\n> \n> \n> > > So, what do we use --filter-print-omitted for? Is anybody needing it?\n> > > Or do we just use it to verify this one test case? Should we fix it,\n> > > or get rid of it, or neither?\n> > \n> > In caf3827e2f there is:\n> > \n> >      This patch introduces handling of missing objects to help\n> >      debugging and development of the \"partial clone\" mechanism,\n> >      and once the mechanism is implemented, for a power user to\n> >      perform operations that are missing-object aware without\n> >      incurring the cost of checking if a missing link is expected.\n> > \n> > So I would say that if you think that --filter-print-omitted doesn't\n> > help in debugging or development, and can even be confusing, and that\n> > it also doesn't help performance for power users or anyone else, then\n> > it would make sense to remove it, unless you find a way to make it\n> > fulfill its original goals, or maybe other worthwhile goals.\n> \n> I don't currently have a use for that (other than the existing test\n> cases), but we could use that in the future as a guide for the server\n> to put the omitted objects on a CDN, for example.\n> \n> So I'd say let's leave it as is for now.\n\nThanks for the input, Jeff. I wasn't sure from looking at it whether it\nwas intended behavior with a plan for use; looks like it is. I'll leave\nit alone.\n\n> \n> \n> Jeff\n> \n> \n> \n"}]}