Rugged::Diff#tree returns Rugged::Tree instead of its instance

[mmozuras]

Rugged::Diff#tree returns Rugged::Tree instead of its instance. How to reproduce:

irb(main):001:0> require 'rugged'
=> true
irb(main):002:0> repo = Rugged::Repository.new('.')
=> #<Rugged::Repository:70098604661600 {path: "/Users/mindaugasmozuras/Projects/rugged/.git/"}>
irb(main):003:0> diff = repo.diff('HEAD', 'HEAD~1')
=> #<Rugged::Diff:0x007f8234db3400 @owner=Rugged::Tree>
irb(main):004:0> diff.owner
=> Rugged::Tree
irb(main):005:0> diff.owner.class
=> Class

[arthurschreiber]

Yeah, this was caused by 93d2d78, specifically this line here:

rugged/ext/rugged/rugged_tree.c

Line 476 in c0bcaab

return rugged_diff_new(rb_cRuggedDiff, self, diff);

. I'll prep a fix and test cases asap.

Thanks for reporting this issue!

[arthurschreiber]

Can you check #392? Having a Rugged::Diff#owner method made no sense, as diffs are not owned by any object in the first place. So I removed them both.

[mmozuras]

@arthurschreiber

Having a Rugged::Diff#owner method made no sense, as diffs are not owned by any object in the first place. So I removed them both.

As a user of rugged, I'd prefer to have a way to access instances of Rugged::Tree or Rugged::Repository, while having only an instance of Rugged::Diff. There are, of course, other ways around it, it's just a convenience 😄

I wouldn't say that it makes "no sense". Current implementation of diff goes through Tree, so there is some connection there.

/*
 *  call-seq:
 *    Tree.diff(repo, tree, diffable[, options]) -> diff
 *
 *  Returns a diff between the `tree` and the diffable object that was given.
 *  +diffable+ can either be a +Rugged::Commit+, a +Rugged::Tree+, a +Rugged::Index+,
 *  or +nil+.
 *
 *  The +tree+ object will be used as the "old file" side of the diff, while the
 *  parent tree or the +diffable+ object will be used for the "new file" side.
 *
 *  If +tree+ or +diffable+ are nil, they will be treated as an empty tree. Passing
 *  both as `nil` will raise an exception.
 */

Another alternative is just to use different names. Here, in documentation, old file and new file are used. But they are also not ideal.

[arthurschreiber]

The following works, too:

Tree.diff(nil, nil)
Tree.diff(nil, other_tree)
Tree.diff(other_tree, nil)

In those cases, what would #owner return?

[arthurschreiber]

Adding accessors for the left and right side of a diff might be possible, but you have to keep in mind that a diff can also be made against the workdir. I'm not sure what those would return in that case.

[mmozuras]

@arthurschreiber

Adding accessors for the left and right side of a diff might be possible, but you have to keep in mind that a diff can also be made against the workdir. I'm not sure what those would return in that case.

Hm, the workdir point sounds like another reason to have only "old side" accessible 😄

/*
 *  call-seq:
 *    tree.diff_workdir([options]) -> diff
 *
 *  Returns a diff between a tree and the current workdir.
 *
 *  The +tree+ object will be used as the "old file" side of the diff, while the
 *  content of the current workdir will be used for the "new file" side.
 */

[arthurschreiber]

This method probably also needs to be converted into a class method on Tree, to support doing a diff between an empty tree and the workdir, like: Tree.diff_workdir(repo, nil)

I'm really not keen on only having an accessor for the "old side" of the diff. But maybe @carlosmn or @vmg feel otherwise?

[carlosmn]

A diff stands on its own, it doesn't belong to a particular tree any more than blob does. What it might need is a reference to the repository in order to stop a GC run from freeing memory it references, but there's no reason that needs to be public (git_diff has a pointer to the repo, but it doesn't necessarily even need it after it's been returned to the user).

If you need more information in your code, then I think passing that information along is bound to work better, as you wouldn't need to assume the diff was created just as you expected it to.

[arthurschreiber]

Thanks @carlosmn!

So yeah, the only thing I would find sensible would be to have #owner point to the repository instance, mainly to ensure it does not get GC'ed (that's also the reason we have all the other owner methods / instance variables).