Hi Robin,
Robin Rosenberg escreveu:
i.e. the tree 'a' is sorted as if there is a '/' at then end. For full path
names that is no problem and it is actually trivial, but when you order
things by partial names it is much messier and quite hard to test also. There
is a bug in the IndexTreeWalker and probably the differenser and you found it.
It seems most likely you fixed it for that particular case, but it could still
be broken for another.
Thanks for your explanation, it makes more sense now.
I didn't look in detail yet the parts where we need to interact with the
index and the workspace, but just thinking about it now. If we are using
an algorithm which requires that both trees are sorted using the same
order, and one of them can't be trusted to follow such order, them
sorting the trees the way we want looks like a simple solution...
I intend to go back to this after I finish some more patches I'm working
on to add more info into the revision detail viewer. So, lets go in
steps and look at one distinct thing at a time. My patch doesn't change
sorting or ordering of things and isn't very complicated either, it just
adds missing functionality.
So, if in the old tree we had:
file1
and in the new tree we have:
file1
dir1/file2
dir1/file3
The compare viewer must show:
+ dir1/file2
+ dir1/file3
The current code only notices that dir1 was added and shows:
+ dir1
which confuses people reviewing code because they can't see that file2
and file3 were added. What the patch does is take the next step and, if
the added or removed node is a directory and it has children, add all
the children to the compare viewer using the same Differencer from the
directory.
The Differencer I changed was actually a bug. If you say that the files
left in the left pane are Differencer.ADDITION, then the ones left on
the right pane _must_ be Differencer.DELETION. It just doesn't make
sense to say that files not on the old tree but on the new tree are
addition, and at the same time that files on the old tree but not on the
new tree are also addition.
There is another check that must be added. If in a commit I delete a
directory and add a file with the same name of the directory and
vice-versa. This case is still failing, it shows an alert with an empty
message. But as I said, I intend to deal with it later, it is a more
uncommon case and nobody here complained about it yet... :)
I think it should be like this for all cases, even when comparing with
the index or the workspace, so if there is something else I'm missing
please let me know.
I'd love to see a unit test for your code since, even if it works, it is very easy
to break again.
Ok, I'll get a look on the test cases and probably write something when
I get back into these compare issues. That if somebody else doesn't do
it before me, of course :)
[]s,
Roger.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
