Breaks gollum's revert_page

[marclove]

gollum-lib's Wiki#revert_page depends on the underlying git adapter providing an apply_patch method. Unfortunately, due to grit's unreliability, the tests for Wiki#revert_page were commented out a long time ago. So while rugged_adapter passes all of gollum-lib's tests, it breaks gollum's "Revert Changes" functionality when you swap it in as the git adapter because apply_patch was never actually implemented.

[bartkamphorst]

Thanks @marclove , it's good to have an issue tracking this. The comment in the code suggests that it is actually gollum-lib that needs fixing, so we'll have to look into the best way of resolving the issue. Any thoughts, @dometto ?

[dometto]

Meh, this should never have gotten through. Can blame grit and the test's being off, but I should've made sure the apply_patch stuff was at least functional.

As I say in the comment I think the best thing to do is to abstract from the relatively low level calls gollum-lib is currently making. With rugged and rjgit we can revert without first generating a patch file, so that would be much cleaner and faster. So instead of having a separate diff and apply_patch methods, we should just have something like revert(filename, old_rev) in the adapters.

[dometto]

Looks like we will have to wait until rugged's revert behavior is released :-(

(https://github.com/libgit2/rugged/pull/545 is in master, but not yet in the gem release)

[marclove]

Thanks @bartkamphorst @dometto! Question… libgit2/rugged#545's revert behavior won't let us revert a single file/page's changes in a commit, right? It reverts an entire commit. Perhaps most users of gollum only make commits through gollum and therefore all commits only change one file/page. Our company sometimes makes larger changes using PR's on GitHub rather than using gollum and sometimes those commits change multiple files. If that's not intended to be supported behavior in gollum, that's cool, but wanted to call it out.

[dometto]

@marclove, we definitely want to support the ability to revert a single file. However, I'm hoping that might be supported via the optional options hash argument to Repository#revert_commit (see here). It's difficult to tell whether that is possible without digging into the libgit2 codebase. If it's not I'll definitely try to bug the rugged people about it. :-)

[marclove]

Got it. Thanks @dometto!

[bartkamphorst]

@dometto Git consistently uses revert to change content of a repo back to the state of a previous commit, in a new commit. I think that is why grit uses apply_patch to change a single path and why rugged has revert_commit.

[dometto]

Hmm. It's easy to checkout an entire blob from an old revision, but we want to be able to revert only some changes, so it does look like we'll have to find a way to apply changes.

[dometto]

Sorry, that was unclear. What I meant is: a checkout is not good enough, we really need something to mimic git apply. That means the upcoming revert_commit method won't help us either. We could do two things:

• Wait for rugged to support apply_patch. There is an apply method in the pipeline for libgit2, but it will take some time before it is merged, let alone before it makes its way into rugged.
• Temporarily implement a simple apply algorithm for rugged diffs in ruby. Rugged:Diff is quite complete and functional, but this would still be non-trivial, and won't perform as good as a libgit2-based solution.

[dometto]

Temporarily implement a simple apply algorithm for rugged diffs in ruby.

That comes to down to this logic (mutatis mutandis): https://github.com/eclipse/jgit/blob/master/org.eclipse.jgit/src/org/eclipse/jgit/api/ApplyCommand.java#L191-L250

[bartkamphorst]

I think that if rugged doesn't support this yet, then that just means that the rugged adapter is not yet ready to replace grit as the default adapter for gollum. Better to wait for the apply functionality than to roll our own.

[marclove]

Awesome! Thank you all, @bartkamphorst @dometto and @hiroponz!!

[hiroponz]

Sorry, it need to implement the revert feature by the rugged. The PR is only upgrading the rugged version that may support revert feature.

[dometto]

libgit2 can now read patch files. Application of them on a per-file basis hangs on https://github.com/libgit2/libgit2/pull/4184

[dometto]

New PR to watch: https://github.com/libgit2/libgit2/pull/4705