Re: Possible --remove-empty bug

From: Junio C Hamano <junkio@cox.net>
Date: Mar 12, 2006, 21:31 UTC
Message-ID: <7vk6azz6xx.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <e5bfff550603120612k555fc7f3v9d8d17b1bd0b9e41@mail.gmail.com>

"Marco Costalba" <mcostalba@gmail.com> writes:

Show 6 quoted lines

>>>From git-rev-list documentation:
>
> --remove-empty::
> 	Stop when a given path disappears from the tree.
>
> But isn't it to be intended *after* a path disapperas from the tree?

To be honest, I do not know how --remove-empty is intended to work. What revision traversal code does and what the above says are different.

The traversal code goes like this:

	* Start from given commits (both interesting and
          uninteresting), look at still-to-be-procesed commit
          one by one, by calling add_parents_to_list().

          * add_parents_to_list() grows still-to-be-processed
            list; if the current commit is uninteresting, mark its
            parents also uninteresting, and if no interesting
            commit remains in the still-to-be-processed list, we
            are done.  On the other hand, if the current commit is
            interesting, place it to the list of results.

        * After the above traversal is done, the consumer calls
	  get_revision() to retrieve commits from the list of
	  results one-by-one.  We return only interesting ones.

And in add_parents_to_list()

	* if the commit is interesting, and when we are limiting
          by paths, we call try_to_simplify_commit().  This
          checks if the tree associated with the current commit
          is the same as one of its parents' with respect to
          specified paths, and if so pretend that the current
          commit has only that parent and no other.  This can
          make a merge commit to lose other parents that we do
          not inherit the specified paths from.

        * try_to_simplify_commit() looks at each parent, and:

          - if we find a parent that has the same tree (wrt the
            paths we are interested in), we pretend it is the
            sole parent of this commit.

	  - if we find a parent that does not have any of the
            specified paths, we pretend we do not have that
            parent under --remove-empty.

	  - otherwise we do not munge the list of parents.

My understanding of what the code is doing from the above reading is to lose that empty parent, and it does not have much to do with stop traversing the ancestry chain at such commit. I am not sure that is what was intended...

Maybe something like this is closer to what the documentation says.

-- >8 --

diff --git a/revision.c b/revision.c
index c8d93ff..03085ff 100644
--- a/revision.c
+++ b/revision.c
@@ -315,9 +315,14 @@ static void try_to_simplify_commit(struc
 			return;
 
 		case TREE_NEW:
-			if (revs->remove_empty_trees && same_tree_as_empty(p->tree)) {
-				*pp = parent->next;
-				continue;
+			if (revs->remove_empty_trees &&
+			    same_tree_as_empty(p->tree)) {
+				/* We are adding all the specified paths from
+				 * this parent, so the parents of it is
+				 * not interesting, but the difference between
+				 * this parent and us still is interesting.
+				 */
+				p->object.flags |= UNINTERESTING;
 			}
 		/* fallthrough */
 		case TREE_DIFFERENT: