Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split TreeEntry's equality and sorting #488

Merged
merged 1 commit into from
Mar 16, 2015

Conversation

carlosmn
Copy link
Member

@carlosmn carlosmn commented Feb 7, 2015

The value equality for TreeEntry needs to take the id into account. The
function we were using git_tree_entry_cmp() is only meant for
git-compatible sorting in a tree and thus does not take that into
account. Change _richcompare() to do so.

We then leave the git-copmatible sorting method as TreeEntry.sortcmp()
which can be used to ask python to use that as the comparison function
in sorted() and friends.


It might just be good enough to leave out TreeEntry.sortcmp() and just rely on richcompare, since the extra checking only comes in when they're otherwise equal.

@jdavid
Copy link
Member

jdavid commented Feb 8, 2015

Tests failing with Python 3

@carlosmn
Copy link
Member Author

carlosmn commented Feb 8, 2015

Ah, they removed sort's 'cmp'. I guess I'll just go with just using richcompare.

@carlosmn
Copy link
Member Author

carlosmn commented Feb 9, 2015

I removed sortcmp() as we can just use the rich comparison method anyway, and that lets us use sort() in the expected way in each version.

@jdavid
Copy link
Member

jdavid commented Feb 10, 2015

Hello, you have fixed the equal and not-equal cases, but it is still using git_tree_entry_cmp for the other cases. I find it confusing that the same function uses different criteria, either the oid or the name. I would expose git_tree_entry_cmp through a separate method, what you first did with TreeEntry.sortcmp, still
available at commit 5c792e3

Then TreeEntry.richcompare would use only the oid, this is basically a copy of the Oid.richcompare

(Sorry for not reviewing the code before and just stop at the failing test.)

@carlosmn
Copy link
Member Author

I don't see what making TreeEntry.richcompare equal to Oid.richcomare would bring us. I would find it more confusing that way, as the id is just one aspect of the TreeEntry. Entries with names "a" and "aa" should go close to each other, regardless of what object they point to.

Two entries would also not be equal just because they point to the same object. The entries for file "a" and "b" are not equal just because the contents of the files they represent happent to be the same.

@jdavid
Copy link
Member

jdavid commented Mar 5, 2015

With this commit we will run into this situation:

>>> a == b
False
>>> a <= b
True
>>> a >= b
True

@jdavid
Copy link
Member

jdavid commented Mar 5, 2015

In other words this is breaking the order relation (the antisymmetry property to be specific).

The issue #458 which spawned this PR says the current behaviour as counter intuitive, or surprising. But breaking the order relation is just as surprising.

This commit is trying to do two different things in the same API. On the other hand I am not certain which would be the best solution. Maybe some use cases could help shed some light: @ssadler ?

The function we were using `git_tree_entry_cmp()` is only meant for
git-compatible sorting in a tree and thus does not take the id into
account. This is however important in order to keep value equality. In
order to avoid issues with assymetry, we compare the id any time when
two entries are equal according to their position in a tree.
@carlosmn
Copy link
Member Author

Ah yes, I hadn't thought about the <= and >= situation. It's unfortunate that sorting and equality are mixed together this way. If <= and >= is true for two entries, then the equals is true for sorting, but they may or may not be have value equality.

I've updated the commit to always compare the ids when they would sort as equal. As having duplicate entries would be a bug when you're creating a tree (and might thus care about comparing inequality for entries) it doesn't detract from the ability to sort, and it keeps the result consistent, which the previous version did not.

@jdavid jdavid merged commit c099655 into libgit2:master Mar 16, 2015
@jdavid
Copy link
Member

jdavid commented Mar 16, 2015

nice solution, sorting by name then by id

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants