libgit2 / rugged

ruby bindings to libgit2
MIT License
2.25k stars 277 forks source link

Is Rugged::Commit.diff right? #561

Open yehaha9876 opened 8 years ago

yehaha9876 commented 8 years ago

I use walker find a commit but when i get it`s diff, i think the result is wrong.

rugged/lib/rugged/commit.rb

    def diff(*args)
      args.unshift(parents.first) if args.size == 1 && args.first.is_a?(Hash)
      self.tree.diff(*args)
    end

rugged/lib/rugged/tree.rb

    def diff(other = nil, options = nil)
      Tree.diff(repo, self, other, options)
    end

but rugged/ext/rugged/rugged_tree.c

 *  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.

so all seams right, but as the comment say 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, is parent used as new side right?

ccutrer commented 5 years ago

Yup. I just ran into this. First of all, it was broken completely if you didn't pass either an other or an options. But it's still counter-intuitive that calling commit.diff will return a diff of that commit being reverted, not applied. Worse still, if you get the commit.diff of the first commit, it somehow automatically swaps things, and returns a forward diff (as you would expect). https://github.com/libgit2/rugged/pull/798 fixes both issues - you can call it with no args, while preserving the current behavior if you explicitly pass something to compare it to (especially since this is how Repository#diff is implemented)