{"thread":{"id":"35669","subject":"[PATCH v2 0/2] Propagating flags carefully from the command line","startedAt":"2014-01-15T23:59:42Z","lastAt":"2014-01-21T05:27:09Z","messageCount":6,"participants":["Junio C Hamano","Jonathan Nieder","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"233182","messageId":"1389830384-22851-1-git-send-email-gitster@pobox.com","threadId":"35669","inReplyTo":null,"subject":"[PATCH v2 0/2] Propagating flags carefully from the command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-15T23:59:42Z","receivedAt":"2014-01-15T23:59:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"So this is my second try.  The second one now gets rid of the call\nto mark_blob_uninteresting() as Peff suggested, because the first\npatch makes the function very well aware that it only should mark\nthe objects that are reachable from the object, and by definition\nblobs do not reach anything.\n\nJunio C Hamano (2):\n  revision: mark contents of an uninteresting tree uninteresting\n  revision: propagate flag bits from tags to pointees\n\n revision.c               | 29 +++++++++++++++++------------\n t/t6000-rev-list-misc.sh | 17 +++++++++++++++++\n 2 files changed, 34 insertions(+), 12 deletions(-)\n\n-- \n1.8.5.3-493-gb139ac2\n"},{"id":"233183","messageId":"1389830384-22851-2-git-send-email-gitster@pobox.com","threadId":"35669","inReplyTo":"1389830384-22851-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 1/2] revision: mark contents of an uninteresting tree uninteresting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-15T23:59:43Z","receivedAt":"2014-01-15T23:59:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git rev-list --objects ^A^{tree} B^{tree}\" ought to mean \"I want a\nlist of objects inside B's tree, but please exclude the objects that\nappear inside A's tree\".\n\nwe see the top-level tree marked as uninteresting (i.e. ^A^{tree} in\nthe above example) and call mark_tree_uninteresting() on it; this\nunfortunately prevents us from recursing into the tree and marking\nthe objects in the tree as uninteresting.\n\nThe reason why \"git log ^A A\" yields an empty set of commits,\ni.e. we do not have a similar issue for commits, is because we call\nmark_parents_uninteresting() after seeing an uninteresting commit.\nThe uninteresting-ness of the commit itself does not prevent its\nparents from being marked as uninteresting.\n\nIntroduce mark_tree_contents_uninteresting() and structure the code\nin handle_commit() in such a way that it makes it the responsibility\nof the callchain leading to this function to mark commits, trees and\nblobs as uninteresting, and also make it the responsibility of the\nhelpers called from this function to mark objects that are reachable\nfrom them.\n\nNote that this is a very old bug that probably dates back to the day\nwhen \"rev-list --objects\" was introduced.  The line to clear\ntree->object.parsed at the end of mark_tree_contents_uninteresting()\ncan be removed when this fix is merged to the codebase after\n6e454b9a (clear parsed flag when we free tree buffers, 2013-06-05).\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n revision.c               | 25 +++++++++++++++++--------\n t/t6000-rev-list-misc.sh |  6 ++++++\n 2 files changed, 23 insertions(+), 8 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 7010aff..28449c5 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -98,17 +98,12 @@ static void mark_blob_uninteresting(struct blob *blob)\n \tblob->object.flags |= UNINTERESTING;\n }\n \n-void mark_tree_uninteresting(struct tree *tree)\n+static void mark_tree_contents_uninteresting(struct tree *tree)\n {\n \tstruct tree_desc desc;\n \tstruct name_entry entry;\n \tstruct object *obj = &tree->object;\n \n-\tif (!tree)\n-\t\treturn;\n-\tif (obj->flags & UNINTERESTING)\n-\t\treturn;\n-\tobj->flags |= UNINTERESTING;\n \tif (!has_sha1_file(obj->sha1))\n \t\treturn;\n \tif (parse_tree(tree) < 0)\n@@ -135,6 +130,19 @@ void mark_tree_uninteresting(struct tree *tree)\n \t */\n \tfree(tree->buffer);\n \ttree->buffer = NULL;\n+\ttree->object.parsed = 0;\n+}\n+\n+void mark_tree_uninteresting(struct tree *tree)\n+{\n+\tstruct object *obj = &tree->object;\n+\n+\tif (!tree)\n+\t\treturn;\n+\tif (obj->flags & UNINTERESTING)\n+\t\treturn;\n+\tobj->flags |= UNINTERESTING;\n+\tmark_tree_contents_uninteresting(tree);\n }\n \n void mark_parents_uninteresting(struct commit *commit)\n@@ -294,7 +302,8 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object\n \t\tif (!revs->tree_objects)\n \t\t\treturn NULL;\n \t\tif (flags & UNINTERESTING) {\n-\t\t\tmark_tree_uninteresting(tree);\n+\t\t\ttree->object.flags |= UNINTERESTING;\n+\t\t\tmark_tree_contents_uninteresting(tree);\n \t\t\treturn NULL;\n \t\t}\n \t\tadd_pending_object(revs, object, \"\");\n@@ -309,7 +318,7 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object\n \t\tif (!revs->blob_objects)\n \t\t\treturn NULL;\n \t\tif (flags & UNINTERESTING) {\n-\t\t\tmark_blob_uninteresting(blob);\n+\t\t\tblob->object.flags |= UNINTERESTING;\n \t\t\treturn NULL;\n \t\t}\n \t\tadd_pending_object(revs, object, \"\");\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 15e3d64..9ad4971 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -56,4 +56,10 @@ test_expect_success 'rev-list A..B and rev-list ^A B are the same' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'propagate uninteresting flag down correctly' '\n+\tgit rev-list --objects ^HEAD^{tree} HEAD^{tree} >actual &&\n+\t>expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.5.3-493-gb139ac2\n"},{"id":"233184","messageId":"1389830384-22851-3-git-send-email-gitster@pobox.com","threadId":"35669","inReplyTo":"1389830384-22851-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 2/2] revision: propagate flag bits from tags to pointees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-15T23:59:44Z","receivedAt":"2014-01-15T23:59:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"With the previous fix 895c5ba3 (revision: do not peel tags used in\nrange notation, 2013-09-19), handle_revision_arg() that processes\ncommand line arguments for the \"git log\" family of commands no\nlonger directly places the object pointed by the tag in the pending\nobject array when it sees a tag object.  We used to place pointee\nthere after copying the flag bits like UNINTERESTING and\nSYMMETRIC_LEFT.\n\nThis change meant that any flag that is relevant to later history\ntraversal must now be propagated to the pointed objects (most often\nthese are commits) while starting the traversal, which is partly\ndone by handle_commit() that is called from prepare_revision_walk().\nWe did propagate UNINTERESTING, but did not do so for others, most\nnotably SYMMETRIC_LEFT.  This caused \"git log --left-right v1.0...\"\n(where \"v1.0\" is a tag) to start losing the \"leftness\" from the\ncommit the tag points at.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n revision.c               |  8 ++------\n t/t6000-rev-list-misc.sh | 11 +++++++++++\n 2 files changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 28449c5..aec0333 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -273,6 +273,7 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object\n \t\t\t\treturn NULL;\n \t\t\tdie(\"bad object %s\", sha1_to_hex(tag->tagged->sha1));\n \t\t}\n+\t\tobject->flags |= flags;\n \t}\n \n \t/*\n@@ -284,7 +285,6 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object\n \t\tif (parse_commit(commit) < 0)\n \t\t\tdie(\"unable to parse commit %s\", name);\n \t\tif (flags & UNINTERESTING) {\n-\t\t\tcommit->object.flags |= UNINTERESTING;\n \t\t\tmark_parents_uninteresting(commit);\n \t\t\trevs->limited = 1;\n \t\t}\n@@ -302,7 +302,6 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object\n \t\tif (!revs->tree_objects)\n \t\t\treturn NULL;\n \t\tif (flags & UNINTERESTING) {\n-\t\t\ttree->object.flags |= UNINTERESTING;\n \t\t\tmark_tree_contents_uninteresting(tree);\n \t\t\treturn NULL;\n \t\t}\n@@ -314,13 +313,10 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object\n \t * Blob object? You know the drill by now..\n \t */\n \tif (object->type == OBJ_BLOB) {\n-\t\tstruct blob *blob = (struct blob *)object;\n \t\tif (!revs->blob_objects)\n \t\t\treturn NULL;\n-\t\tif (flags & UNINTERESTING) {\n-\t\t\tblob->object.flags |= UNINTERESTING;\n+\t\tif (flags & UNINTERESTING)\n \t\t\treturn NULL;\n-\t\t}\n \t\tadd_pending_object(revs, object, \"\");\n \t\treturn NULL;\n \t}\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 9ad4971..3794e4c 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -62,4 +62,15 @@ test_expect_success 'propagate uninteresting flag down correctly' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'symleft flag bit is propagated down from tag' '\n+\tgit log --format=\"%m %s\" --left-right v1.0...master >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t> two\n+\t> one\n+\t< another\n+\t< that\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.5.3-493-gb139ac2\n"},{"id":"233185","messageId":"20140116001607.GN18964@google.com","threadId":"35669","inReplyTo":"1389830384-22851-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v2 1/2] revision: mark contents of an uninteresting tree uninteresting","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-01-16T00:16:07Z","receivedAt":"2014-01-16T00:16:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> we see the top-level tree marked as uninteresting (i.e. ^A^{tree} in\n> the above example) and call mark_tree_uninteresting() on it; this\n> unfortunately prevents us from recursing into the tree and marking\n> the objects in the tree as uninteresting.\n\nSo the tree is marked uninteresting twice --- once by setting in the\nUNINTERESTING flag in handle_revision_arg() and a second attempted\ntime in mark_tree_uninteresting()?   Makes sense.\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"233187","messageId":"xmqq61pk227v.fsf@gitster.dls.corp.google.com","threadId":"35669","inReplyTo":"20140116001607.GN18964@google.com","subject":"Re: [PATCH v2 1/2] revision: mark contents of an uninteresting tree uninteresting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-16T00:35:16Z","receivedAt":"2014-01-16T00:35:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>\n>> we see the top-level tree marked as uninteresting (i.e. ^A^{tree} in\n>> the above example) and call mark_tree_uninteresting() on it; this\n>> unfortunately prevents us from recursing into the tree and marking\n>> the objects in the tree as uninteresting.\n>\n> So the tree is marked uninteresting twice --- once by setting in the\n> UNINTERESTING flag in handle_revision_arg() and a second attempted\n> time in mark_tree_uninteresting()?   Makes sense.\n\nIt is that the original code, the setting of the mark on the object\nitself was inconsistent.  For commits, we did that ourselves; for\ntrees, we let the mark_tree_uninteresting() do so.\n\nAnd mark_tree_uninteresting() has this quirk that it refuses to\nrecurse into the given tree, if the tree is already marked as\nuninteresting by the caller.\n\nWe did not have the same problem on commits, because we make a\nsimilar call to mark-parents-uninteresting but the function does not\ncare if the commit itself is already marked as uninteresting.\n\nThe distinction does not matter when tags are not involved.  But\nonce we start propagating the flags from a tag to objects that the\ntag points at, it starts to matter.  The caller will mark the object\nuninteresting in the loop that peels the tag, and the resulting\nobject is uninteresting.  It is not a problem for commits.  It was a\nproblem for trees, which used mark_tree_uninteresting() to mark all\nthe objects inside the tree uninteresting, including the tree itself.\n"},{"id":"233430","messageId":"20140121052708.GB5878@sigill.intra.peff.net","threadId":"35669","inReplyTo":"1389830384-22851-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v2 0/2] Propagating flags carefully from the command line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-21T05:27:09Z","receivedAt":"2014-01-21T05:27:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 15, 2014 at 03:59:42PM -0800, Junio C Hamano wrote:\n\n> So this is my second try.  The second one now gets rid of the call\n> to mark_blob_uninteresting() as Peff suggested, because the first\n> patch makes the function very well aware that it only should mark\n> the objects that are reachable from the object, and by definition\n> blobs do not reach anything.\n> \n> Junio C Hamano (2):\n>   revision: mark contents of an uninteresting tree uninteresting\n>   revision: propagate flag bits from tags to pointees\n\nSorry for a slow review, but I just read through your earlier comments\nand this series. I agree there was definitely a bug in what we were\ndiscussing earlier, and this looks like the right way to fix it. The end\nresult splits the flag-setting responsibility much more sensibly.\n\nThanks.\n\n-Peff\n"}]}