Wrap diff iterator API #103

Closed
wants to merge 29 commits into
from

Projects

None yet

10 participants

@brianmario
Member

My initial attempt at wrapping the diff iterator API recently added to libgit2.

The various diff methods don't accept options yet, but I'll add that asap. Wanted to open this pull to get a discussion going around where I'm taking the API so far. And so we can all get a good chuckle from my crappy C code ;)

Quick API example:

r = Rugged::Repository.new('.')
diff = r.diff(commit1, commit2)

diff.deltas.each do |delta|
  # ...
  delta.hunks.each do |hunk|
    # ...
    hunk.lines.each do |line|
      # ...
    end
  end
end

New Objects

This introduces four new objects, Rugged::Diff, Rugged::Diff::Delta, Rugged::Diff::Hunk and Rugged::Diff::Line.

Rugged::Diff - The high-level object representing a diff, which is composed of a list of file deltas.

Rugged::Diff::Delta - A list of changes (hunks) from a specific file or rename.

Rugged::Diff::Hunk - A list of lines from a single change in a file.

Rugged::Diff::Line - A single line from the diff which can either be a change itself or a context line.

New Methods

Rugged::Tree#diff(other_tree) is the entry point into the libgit2 code to compute the diff, but I also added a couple of "convinience" methods: Rugged::Commit#diff(other_commit) and Rugged::Repository#diff(tree_or_commit1, tree_or_commit2).

As usual, this all needs tests and documentation. I held back because I have no idea where this API will end up.

Discuss! :)

@arthurschreiber
Member

I ❤️ ❤️ ❤️ this, very cool!

@vmg

What happened to rb_io_write? I thought we were going to roll with that. Regardless, the global id_write is very ugly. We should stick to calling rb_intern locally.

Member

Looking up id_write was just an optimization (the ruby-core code does this all over the place) but I can change it to looking it up each time.

As for rb_io_write, I decided to use rb_funcall because even though internally it's doing the same thing, that could change and end up doing something that will only work on IO objects. Waddya think?

Member

I'd go with rb_io_write for now, and maybe add some tests where you write into a StringIO to make sure that we get notified if this behaviour ever changes.

Member

@arthurschreiber that'll work :)

@egonSchiele

+1, awesome!

@arthurschreiber
Member

Any progress on this? What is missing to get this merged?

@vanakenm
vanakenm commented Feb 5, 2013

Same question as above, any change to have this (or more generally a possiblity to do "diff" using rugged) anytime soon ? Any way to test or contribute that could help ? (I probably cannot write C code, but I'll be happy to test or help on the Ruby side).

@brianmario
Member

Hey guys - this pull is outdated as the diff API has been almost completely redesigned since it was opened. I haven't had a chance to go back and refactor it to make it work against the current API but that's all that needs to be done. I'd like to keep the Ruby API the same as it is in this pull.

@arthurschreiber
Member

I'll see what I can do then. :)

@bootstraponline bootstraponline referenced this pull request in gollum/gollum Feb 12, 2013
Closed

Move to Rugged #617

0 of 5 tasks complete
@heelhook

@brianmario Do you know if anyone is working on this at Github? I wouldn't mind at all implementing the libgit2 current diff API in rugged using this PR API as a skeleton if no one else is working on it.

@brianmario
Member

@heelhook I haven't had a chance to work on it again and I don't think anyone else has started on it yet either. Feel free to get started

@jrsconfitto

i've been working on it in secret because i don't know C and haven't gotten anything to compile yet and didn't want to publicize my ineptitude. i'm learning C as we speak and slowly getting better, but i'm still not quite there yet.

Anyway, i'm willing to help out on this, in whatever capacity i can.

@NARKOZ
Contributor
NARKOZ commented Mar 19, 2013

:shipit:

@randx
randx commented Mar 30, 2013

+1 for this. really need a diff iterator

@thelucid
thelucid commented Apr 7, 2013

Any progress on this, it's super useful. I'm currently resorting to backticks:

`git diff --raw --name-only #{old_sha} #{new_sha}`.each_line do |path|
  path.chomp!
  puts "You changed: #{path}"
end

Or if I just want (A)dded, (C)opied, (M)odified etc. I use:

`git diff --raw --name-only --diff-filter=ACM #{old_sha} #{new_sha}`.each_line do |path|
  path.chomp!
  puts "You changed: #{path}"
end

Would love to see this merged.

@thelucid thelucid referenced this pull request Apr 7, 2013
Closed

Initial diff wrapping #98

@jrsconfitto

i've been slowly chipping away at updating the code underlying this proposed API in my fork. i've got a PR (heelhook/rugged#3) on another fork where i'm working on this. i would love help, especially in the form of code review because i'm extremely new to C.

This started as a PR on gollum as a fun way to learn Ruby and now i'm tunnelling deeper and deeper, knee-deep in C, and loving the ride 😄

@thelucid
thelucid commented Apr 8, 2013

@jugglingnutcase I would love to help with this but my C knowledge is at just about zero. Happy to help with any Ruby API decisions etc.

@arthurschreiber arthurschreiber referenced this pull request Apr 8, 2013
Merged

Updated Diff Iteration #181

5 of 5 tasks complete
@thelucid
thelucid commented Apr 9, 2013

Pardon my ignorance but I have a need to get a list of object ids that have changes (added, modified, deleted etc. etc.) between two commit sha's. Would this be possible with this patch, I'm thinking something like:

repo.object_diff(old_sha, new_sha, '/refs/heads/master').each do |type, oid|
  puts "type: #{type}, oid: #{oid}"
end

...this would output something like:

type: A, oid: 9651783511be5f3eca035b3291852e8772dbe0f7
type: M, oid: 1fb0788141a10bc39ad00b29d73580a15d22054f
type: D, oid: 9651783511be5f3eca035b3291852e8772dbe0f7
etc.

Even if someone knows how I would achieve this with git itself so I could then pass the oid's to a repo.lookup(oid) call. I may be looking at this from the wrong angle.

@jrsconfitto

Based on the little i know, that should be there.

@thelucid
thelucid commented Apr 9, 2013

@jugglingnutcase Probably the wrong kind of place for this kind of question but I don't suppose you know how you achieve this with git shell commands until the patch makes it in?

@arthurschreiber
Member

@vmg You might want to close this one too.

@vmg
Member
vmg commented May 5, 2013

Ayup. Superseded by #181

@vmg vmg closed this May 5, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment