threads / patch / 35669

v2, 2 partsPropagating flags carefully from the command line

Subject: [PATCH v2 0/2] Propagating flags carefully from the command line

## tl;dr

6 messages between Jan 15, 2014 and Jan 21, 2014. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Junio C Hamano· Jan 15, 2014, 23:59 UTC · lore

So this is my second try. The second one now gets rid of the call to mark_blob_uninteresting() as Peff suggested, because the first patch makes the function very well aware that it only should mark the objects that are reachable from the object, and by definition blobs do not reach anything.

Junio C Hamano (2):
  revision: mark contents of an uninteresting tree uninteresting
  revision: propagate flag bits from tags to pointees
 revision.c               | 29 +++++++++++++++++------------
 t/t6000-rev-list-misc.sh | 17 +++++++++++++++++
 2 files changed, 34 insertions(+), 12 deletions(-)
-- 
1.8.5.3-493-gb139ac2
Junio C Hamano· Jan 15, 2014, 23:59 UTC · re: Junio C Hamano · lore

[PATCH v2 1/2] revision: mark contents of an uninteresting tree uninteresting

"git rev-list --objects ^A^{tree} B^{tree}" ought to mean "I want a list of objects inside B's tree, but please exclude the objects that appear inside A's tree".

we see the top-level tree marked as uninteresting (i.e. ^A^{tree} in the above example) and call mark_tree_uninteresting() on it; this unfortunately prevents us from recursing into the tree and marking the objects in the tree as uninteresting.

The reason why "git log ^A A" yields an empty set of commits, i.e. we do not have a similar issue for commits, is because we call mark_parents_uninteresting() after seeing an uninteresting commit. The uninteresting-ness of the commit itself does not prevent its parents from being marked as uninteresting.

Introduce mark_tree_contents_uninteresting() and structure the code in handle_commit() in such a way that it makes it the responsibility of the callchain leading to this function to mark commits, trees and blobs as uninteresting, and also make it the responsibility of the helpers called from this function to mark objects that are reachable from them.

Note that this is a very old bug that probably dates back to the day when "rev-list --objects" was introduced. The line to clear tree->object.parsed at the end of mark_tree_contents_uninteresting() can be removed when this fix is merged to the codebase after 6e454b9a (clear parsed flag when we free tree buffers, 2013-06-05).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 revision.c               | 25 +++++++++++++++++--------
 t/t6000-rev-list-misc.sh |  6 ++++++
 2 files changed, 23 insertions(+), 8 deletions(-)
Show changes to 2 files +23 −8

revision.c, t/t6000-rev-list-misc.sh

diff --git a/revision.c b/revision.c
index 7010aff..28449c5 100644
--- a/revision.c
+++ b/revision.c
@@ -98,17 +98,12 @@ static void mark_blob_uninteresting(struct blob *blob)
 	blob->object.flags |= UNINTERESTING;
 }
 
-void mark_tree_uninteresting(struct tree *tree)
+static void mark_tree_contents_uninteresting(struct tree *tree)
 {
 	struct tree_desc desc;
 	struct name_entry entry;
 	struct object *obj = &tree->object;
 
-	if (!tree)
-		return;
-	if (obj->flags & UNINTERESTING)
-		return;
-	obj->flags |= UNINTERESTING;
 	if (!has_sha1_file(obj->sha1))
 		return;
 	if (parse_tree(tree) < 0)
@@ -135,6 +130,19 @@ void mark_tree_uninteresting(struct tree *tree)
 	 */
 	free(tree->buffer);
 	tree->buffer = NULL;
+	tree->object.parsed = 0;
+}
+
+void mark_tree_uninteresting(struct tree *tree)
+{
+	struct object *obj = &tree->object;
+
+	if (!tree)
+		return;
+	if (obj->flags & UNINTERESTING)
+		return;
+	obj->flags |= UNINTERESTING;
+	mark_tree_contents_uninteresting(tree);
 }
 
 void mark_parents_uninteresting(struct commit *commit)
@@ -294,7 +302,8 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object
 		if (!revs->tree_objects)
 			return NULL;
 		if (flags & UNINTERESTING) {
-			mark_tree_uninteresting(tree);
+			tree->object.flags |= UNINTERESTING;
+			mark_tree_contents_uninteresting(tree);
 			return NULL;
 		}
 		add_pending_object(revs, object, "");
@@ -309,7 +318,7 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object
 		if (!revs->blob_objects)
 			return NULL;
 		if (flags & UNINTERESTING) {
-			mark_blob_uninteresting(blob);
+			blob->object.flags |= UNINTERESTING;
 			return NULL;
 		}
 		add_pending_object(revs, object, "");
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 15e3d64..9ad4971 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -56,4 +56,10 @@ test_expect_success 'rev-list A..B and rev-list ^A B are the same' '
 	test_cmp expect actual
 '
 
+test_expect_success 'propagate uninteresting flag down correctly' '
+	git rev-list --objects ^HEAD^{tree} HEAD^{tree} >actual &&
+	>expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.5.3-493-gb139ac2
Jonathan Nieder· Jan 16, 2014, 00:16 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 1/2] revision: mark contents of an uninteresting tree uninteresting

Junio C Hamano wrote:
> we see the top-level tree marked as uninteresting (i.e. ^A^{tree} in
> the above example) and call mark_tree_uninteresting() on it; this
> unfortunately prevents us from recursing into the tree and marking
> the objects in the tree as uninteresting.

So the tree is marked uninteresting twice --- once by setting in the UNINTERESTING flag in handle_revision_arg() and a second attempted time in mark_tree_uninteresting()? Makes sense.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Junio C Hamano· Jan 16, 2014, 00:35 UTC · re: Jonathan Nieder · lore

Re: [PATCH v2 1/2] revision: mark contents of an uninteresting tree uninteresting

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 10 quoted lines
> Junio C Hamano wrote:
>
>> we see the top-level tree marked as uninteresting (i.e. ^A^{tree} in
>> the above example) and call mark_tree_uninteresting() on it; this
>> unfortunately prevents us from recursing into the tree and marking
>> the objects in the tree as uninteresting.
>
> So the tree is marked uninteresting twice --- once by setting in the
> UNINTERESTING flag in handle_revision_arg() and a second attempted
> time in mark_tree_uninteresting()?   Makes sense.

It is that the original code, the setting of the mark on the object itself was inconsistent. For commits, we did that ourselves; for trees, we let the mark_tree_uninteresting() do so.

And mark_tree_uninteresting() has this quirk that it refuses to recurse into the given tree, if the tree is already marked as uninteresting by the caller.

We did not have the same problem on commits, because we make a similar call to mark-parents-uninteresting but the function does not care if the commit itself is already marked as uninteresting.

The distinction does not matter when tags are not involved. But once we start propagating the flags from a tag to objects that the tag points at, it starts to matter. The caller will mark the object uninteresting in the loop that peels the tag, and the resulting object is uninteresting. It is not a problem for commits. It was a problem for trees, which used mark_tree_uninteresting() to mark all the objects inside the tree uninteresting, including the tree itself.

Junio C Hamano· Jan 15, 2014, 23:59 UTC · re: Junio C Hamano · lore

[PATCH v2 2/2] revision: propagate flag bits from tags to pointees

With the previous fix 895c5ba3 (revision: do not peel tags used in range notation, 2013-09-19), handle_revision_arg() that processes command line arguments for the "git log" family of commands no longer directly places the object pointed by the tag in the pending object array when it sees a tag object. We used to place pointee there after copying the flag bits like UNINTERESTING and SYMMETRIC_LEFT.

This change meant that any flag that is relevant to later history traversal must now be propagated to the pointed objects (most often these are commits) while starting the traversal, which is partly done by handle_commit() that is called from prepare_revision_walk(). We did propagate UNINTERESTING, but did not do so for others, most notably SYMMETRIC_LEFT. This caused "git log --left-right v1.0..." (where "v1.0" is a tag) to start losing the "leftness" from the commit the tag points at.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 revision.c               |  8 ++------
 t/t6000-rev-list-misc.sh | 11 +++++++++++
 2 files changed, 13 insertions(+), 6 deletions(-)
Show changes to 2 files +13 −6

revision.c, t/t6000-rev-list-misc.sh

diff --git a/revision.c b/revision.c
index 28449c5..aec0333 100644
--- a/revision.c
+++ b/revision.c
@@ -273,6 +273,7 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object
 				return NULL;
 			die("bad object %s", sha1_to_hex(tag->tagged->sha1));
 		}
+		object->flags |= flags;
 	}
 
 	/*
@@ -284,7 +285,6 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object
 		if (parse_commit(commit) < 0)
 			die("unable to parse commit %s", name);
 		if (flags & UNINTERESTING) {
-			commit->object.flags |= UNINTERESTING;
 			mark_parents_uninteresting(commit);
 			revs->limited = 1;
 		}
@@ -302,7 +302,6 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object
 		if (!revs->tree_objects)
 			return NULL;
 		if (flags & UNINTERESTING) {
-			tree->object.flags |= UNINTERESTING;
 			mark_tree_contents_uninteresting(tree);
 			return NULL;
 		}
@@ -314,13 +313,10 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object
 	 * Blob object? You know the drill by now..
 	 */
 	if (object->type == OBJ_BLOB) {
-		struct blob *blob = (struct blob *)object;
 		if (!revs->blob_objects)
 			return NULL;
-		if (flags & UNINTERESTING) {
-			blob->object.flags |= UNINTERESTING;
+		if (flags & UNINTERESTING)
 			return NULL;
-		}
 		add_pending_object(revs, object, "");
 		return NULL;
 	}
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 9ad4971..3794e4c 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -62,4 +62,15 @@ test_expect_success 'propagate uninteresting flag down correctly' '
 	test_cmp expect actual
 '
 
+test_expect_success 'symleft flag bit is propagated down from tag' '
+	git log --format="%m %s" --left-right v1.0...master >actual &&
+	cat >expect <<-\EOF &&
+	> two
+	> one
+	< another
+	< that
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.5.3-493-gb139ac2
Jeff King· Jan 21, 2014, 05:27 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 0/2] Propagating flags carefully from the command line

On Wed, Jan 15, 2014 at 03:59:42PM -0800, Junio C Hamano wrote:
Show 9 quoted lines
> So this is my second try.  The second one now gets rid of the call
> to mark_blob_uninteresting() as Peff suggested, because the first
> patch makes the function very well aware that it only should mark
> the objects that are reachable from the object, and by definition
> blobs do not reach anything.
> 
> Junio C Hamano (2):
>   revision: mark contents of an uninteresting tree uninteresting
>   revision: propagate flag bits from tags to pointees

Sorry for a slow review, but I just read through your earlier comments and this series. I agree there was definitely a bug in what we were discussing earlier, and this looks like the right way to fix it. The end result splits the flag-setting responsibility much more sensibly.

Thanks.
-Peff

← back to recent threads