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.

Sign up for GitHub

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

Jump to bottom

Repository.diff fails when treeish is "empty" #432

Closed
Inkane opened this issue Oct 6, 2014 · 8 comments
Closed

Repository.diff fails when treeish is "empty" #432

Inkane opened this issue Oct 6, 2014 · 8 comments

Comments

@Inkane
Copy link

Inkane commented Oct 6, 2014

Repository.diff contains https://github.com/libgit2/pygit2/blob/master/pygit2/repository.py#L390
This causes a subsequent failure when a is pygit2.Commit and len(a.tree) is 0, because a treeish_to_tree(a) evaluates to False in this case. Consequently, a will be the Commit object afterwards.
An example for such a commit is eb02887bc34130647a52f8152c13531d97f5a058 from https://github.com/twbs/bootstrap.git . https://stackoverflow.com/questions/9765453/gits-semi-secret-empty-tree might shed some light on the empty tree.
Inkane pushed a commit to RepoGrams/RepoGrams that referenced this issue Oct 6, 2014
workaround for libgit2/pygit2#432
385f0f5 
calling the low level method is faster anyway
@carlosmn
Copy link
Member

carlosmn commented Oct 6, 2014

So git is assuming that every other implementation is going to support the fake empty tree and it doesn't bother writing the actual tree it wants to use to disc? That is quite worrying.

carlosmn added a commit to carlosmn/pygit2 that referenced this issue Oct 6, 2014
@carlosmn
No object should ever be False
156a82e 
The representation of Tree as a sequence causes boolean coercion to
consider the empty tree to be False, which is nonsensical.

Override __nonzero__ for all object types to return True.

This fixes libgit2#432.
@carlosmn
Copy link
Member

carlosmn commented Oct 6, 2014

This behaviour has nothing to do with git-core's special handling of the empty tree in some situations, or really with the Git system at all, it's purely an unfortunate decision in Python to consider that an object with a sequence interface is falsy just because the length of that aspect is zero.

@carlosmn carlosmn mentioned this issue Oct 6, 2014
Closed
carlosmn added a commit to carlosmn/pygit2 that referenced this issue Oct 6, 2014
@carlosmn
No object should ever be False
2031136 
The representation of Tree as a sequence causes boolean coercion to
consider the empty tree to be False, which is nonsensical.

Override __nonzero__ for all object types to return True.

This fixes libgit2#432.
carlosmn added a commit to carlosmn/pygit2 that referenced this issue Oct 6, 2014
@carlosmn
No object should ever be False
947e409 
The representation of Tree as a sequence causes boolean coercion to
consider the empty tree to be False, which is nonsensical.

Override __nonzero__ for all object types to return True.

This fixes libgit2#432.
carlosmn added a commit to carlosmn/pygit2 that referenced this issue Oct 6, 2014
@carlosmn
No object should ever be False
d0aaa64 
The representation of Tree as a sequence causes boolean coercion to
consider the empty tree to be False, which is nonsensical.

Override __nonzero__ for all object types to return True.

This fixes libgit2#432.
@jdavid
Copy link
Member

jdavid commented Oct 7, 2014

I disagree with the solution. It is the code in Repository.diff which is wrong and should be spelled
differently. It should be explicit.

Regarding the if len == 0 then falsy debate, I have not yet stop to think about it, so I don't
have an opinion yet, anyway, that's to be discussed in PR #433

@jdavid
Copy link
Member

jdavid commented Oct 7, 2014

There is by the way at least one more bug, when the input is a reference.

@carlosmn
Copy link
Member

carlosmn commented Oct 7, 2014

There are more issues with this method, yes. I have a refactored version which should clear away the issue as well.

@carlosmn
Copy link
Member

carlosmn commented Oct 7, 2014

PR #434 would also fix this, as well as working for references.

jdavid added a commit that referenced this issue Oct 7, 2014
@jdavid
Fix Repository.diff for references an empty trees
510f617 
This should fix issue #432

By the way make the code a little more robust.
@jdavid
Copy link
Member

jdavid commented Oct 7, 2014

@carlosmn ouch, just pushed my version, feel free to improve it

@Inkane please check if it works for you

@Inkane
Copy link
Author

Inkane commented Oct 7, 2014

@jdavid : Your change does indeed fix my issue.

@jdavid jdavid closed this as completed Oct 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Object or the Index are never False carlosmn/pygit2
3 participants
@carlosmn @jdavid @Inkane