revision: propagate flag bits from tags to pointees

[Junio C Hamano]

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 gits...@pobox.com
---

 * Comes directly on top of the faulty commit, so that we could
   backport it to 1.8.4.x series.

 revision.c   |  2 +-
 t/t6000-rev-list-misc.sh | 11 +++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 7010aff..6d1c8f9 100644
--- a/revision.c
+++ b/revision.c
@@ -265,6 +265,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;
}
 
/*
@@ -276,7 +277,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;
}
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 15e3d64..b84d6b0 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -56,4 +56,15 @@ test_expect_success 'rev-list A..B and rev-list ^A B are the 
same' '
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: revision: propagate flag bits from tags to pointees

[Jeff King]

On Wed, Jan 15, 2014 at 12:26:13PM -0800, Junio C Hamano wrote:

 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 gits...@pobox.com
 ---

Looks good to me. As per my previous mail, I _think_ you could squash
in:

diff --git a/revision.c b/revision.c
index f786b51..2db906c 100644
--- a/revision.c
+++ b/revision.c
@@ -316,13 +316,10 @@ static struct commit *handle_commit(struct rev_info *revs,
 * 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) {
-   mark_blob_uninteresting(blob);
+   if (flags  UNINTERESTING)
return NULL;
-   }
add_pending_object(revs, object, );
return NULL;
}

but that is not very much code reduction (and mark_blob_uninteresting is
very cheap). So it may not be worth the risk that my analysis is wrong.
:)

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: revision: propagate flag bits from tags to pointees

[Junio C Hamano]

Jeff King p...@peff.net writes:

 Looks good to me. As per my previous mail, I _think_ you could squash
 in:

 diff --git a/revision.c b/revision.c
 index f786b51..2db906c 100644
 --- a/revision.c
 +++ b/revision.c
 @@ -316,13 +316,10 @@ static struct commit *handle_commit(struct rev_info 
 *revs,
* 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) {
 - mark_blob_uninteresting(blob);
 + if (flags  UNINTERESTING)
   return NULL;
 - }
   add_pending_object(revs, object, );
   return NULL;
   }

 but that is not very much code reduction (and mark_blob_uninteresting is
 very cheap). So it may not be worth the risk that my analysis is wrong.
 :)

Your analysis is correct, but I think the pros-and-cons of the your
squashable change boils down to the choice between:

 - leaving it in will keep similarity between tree and blob
   codepaths (both have mark_X_uninteresting(); and

 - reducing cycles by taking advantage of the explicit knowledge
   that mark_X_uninteresting() recurses for a tree while it does not
   for a blob.

But I have a suspicion that my patch may break if any codepath looks
at the current flag on the object and decides ah, it already is
marked and punts.

It indeed looks like mark_tree_uninteresting() does have that
property.  When an uninteresting tag directly points at a tree, if
we propagate the UNINTERESTING bit to the pointee while peeling,
wouldn't we end up calling mark_tree_uninteresting() on a tree,
whose flags already have UNINTERESTING bit set, causing it not to
recurse?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: revision: propagate flag bits from tags to pointees

[Junio C Hamano]

Junio C Hamano gits...@pobox.com writes:

 But I have a suspicion that my patch may break if any codepath looks
 at the current flag on the object and decides ah, it already is
 marked and punts.

 It indeed looks like mark_tree_uninteresting() does have that
 property.  When an uninteresting tag directly points at a tree, if
 we propagate the UNINTERESTING bit to the pointee while peeling,
 wouldn't we end up calling mark_tree_uninteresting() on a tree,
 whose flags already have UNINTERESTING bit set, causing it not to
 recurse?

Extending that line of thought further, what should this do?

git rev-list --objects ^HEAD^^{tree} HEAD^{tree} |
git pack-object --stdin pack

It says I am interested in the objects that is used in the tree of
HEAD, but I do not need those that already appear in HEAD^.

With the current code (with or without the fix under discussion, or
even without the faulty do not peel tags used in range notation),
the tree of the HEAD^ is marked in handle_revision_arg() as
UNINTERESTING when it is placed in revs-pending.objects[], and the
handle_commit() --- we should rename it to handle_pending_object()
or something, by the way --- will call mark_tree_uninteresting() on
that tree, which then would say It is already uninteresting and
return without marking the objects common to these two trees
uninteresting, no?

I think that is a related but separate bug that dates back to
prehistoric times, and the asymmetry between handle_commit() deals
with commits and trees should have been a clear clue that tells us
something is fishy.  It calls mark PARENTS uninteresting, leaving
the responsibility of marking the commit itself to the caller, but
it calls mark_tree_uninteresting() whose caller is not supposed to
mark the tree itself.

Which suggest me that a right fix for this separate bug would be to
introduce mark_tree_contents_uninteresting() or something, which has
the parallel semantics to mark_parents_uninteresting().  Then
mark_blob_uninteresting() call in the function can clearly go.  Such
a change will make it clear that handle_commit() is responsible for
handling the flags for the given object, and any helper functions
called by it should not peek and stop the flag of the object itself
when deciding to recurse into the objects linked to it.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

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 gits...@pobox.com
---
 revision.c   |  8 ++--
 t/t6000-rev-list-misc.sh | 11 +++
 2 files changed, 13 insertions(+), 6 deletions(-)

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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html