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.

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

Implement ApplyPatchToIndex Class #50

Merged
merged 6 commits into from
Mar 14, 2020
Merged

Conversation

dometto
Copy link
Contributor

@dometto dometto commented Oct 11, 2019

This started as an experiment to see how difficult it would be to implement a version of JGit's ApplyCommand that applies a patch to a tree in the repository, rather than to the working directory. See here for a description of the functionality that is currently missing.

The new Plumbing::ApplyPatchToIndex class is a ruby implementation of this JGit class, or at least of its main logic:

  • The ApplyPatchToIndex#build_map method executes the JGit ApplyCommand.calllogic
  • The ApplyPatchToIndex#apply method executes the JGit ApplyCommand.apply logic

In writing these methods I just followed the Java code step by step and attempted to translate it to some sane Ruby. It turned out the actual patch application logic was relatively simple: much of the code in the JGit class concerns initializing, copying, and looping over arrays, which we can vastly simplify thanks to JRuby. And another large part of the JGit class concerns making the changes on the file system, which we're not interested in.

I was amazed to find out that the code seems to actually work. :) Try running this test script:

require './lib/rjgit.rb'
include RJGit

r = Repo.new('/path/to/repo')

patch = ''
diff  = RJGit::Porcelain.diff(r, {:patch => true, :new_rev => r.commits[1].id, :old_rev => r.commits[0].id})
diff.each {|x| patch << x[:patch]}

builder = Plumbing::ApplyPatchToIndex.new(r, patch)

builder.build_map

puts builder.treemap.inspect

This will produce a treemap corresponding to a revert of the last commit of your target repo. You could theoretically then commit this to the repository by calling builder.commit. Neat, if I do say so myself. :)

This at least gives us some basis for judging whether we want to implement this code in RJGit, or whether it is too hackish and we need to submit a patch to JGit. @bartkamphorst: I would understand if you still prefer the second option, but wanted to give this a go at least.

Note: the only logic that I haven't implemented yet, because I don't understand it well enough, is this bit concerning newlines.

@coveralls
Copy link

coveralls commented Oct 11, 2019

Coverage Status

Coverage decreased (-0.09%) to 99.25% when pulling bf7e22f on dometto:apply_tree into 919e739 on repotag:master.

@dometto
Copy link
Contributor Author

dometto commented Oct 18, 2019

@bartkamphorst if you think we should implement this functionality ourselves for now, I'll work on adding tests for this.

@dometto
Copy link
Contributor Author

dometto commented Mar 13, 2020

Added specs. If travis passes, should be ready to merge!

@bartkamphorst
Copy link
Contributor

Test suite on Travis succeeded, but coveralls complains: `Coveralls encountered an exception:

NoMethodError
undefined method coverage' for #<SimpleCov::SourceFile:0x56dae6>. Any ideas?

@dometto dometto closed this Mar 13, 2020
@dometto dometto reopened this Mar 13, 2020
@dometto
Copy link
Contributor Author

dometto commented Mar 13, 2020

Where are you seeing this coveralls exception?

@bartkamphorst
Copy link
Contributor

Where are you seeing this coveralls exception?

In Travis (e.g., #329.1):

Screen Shot 2020-03-14 at 09 18 52

lib/rjgit.rb Outdated Show resolved Hide resolved
lib/rjgit.rb Outdated Show resolved Hide resolved
lib/rjgit.rb Outdated Show resolved Hide resolved
lib/rjgit.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@bartkamphorst bartkamphorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my 'Single comments'. 🤔

@dometto
Copy link
Contributor Author

dometto commented Mar 14, 2020

Test suite on Travis succeeded, but coveralls complains: `Coveralls encountered an exception:

Fixed by updating coveralls in the Gemfile

@dometto dometto merged commit 34ff09a into repotag:master Mar 14, 2020
@dometto dometto deleted the apply_tree branch March 14, 2020 16:32
@bartkamphorst
Copy link
Contributor

🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants