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

Diff Context Lines (--unified=...) #423

Closed
wants to merge 1 commit into from
Closed

Conversation

dahlbyk
Copy link
Member

@dahlbyk dahlbyk commented May 7, 2013

http://stackoverflow.com/questions/16402124/equivalent-to-git-diff-unified-0-with-libgit2sharp

Currently 3 lines of context is hard-coded:

options.ContextLines = 3;
. That should be configurable.

@nulltoken
Copy link
Member

That should be configurable.

I agree. How about making the Compare() overloads accept an optional CompareOptions (as discussed in #278) which would expose getters/setters to control the number of context and interhunk lines?

@dahlbyk
Copy link
Member Author

dahlbyk commented May 6, 2013

I agree. How about making the Compare() overloads accept an optional CompareOptions (as discussed in #278) which would expose getters/setters to control the number of context and interhunk lines?

I'd forgotten about that thread - good call. Will see how far I can get on this tonight.

dahlbyk added a commit to dahlbyk/libgit2sharp that referenced this pull request May 7, 2013
* ContextLines
* InterhunkLines

Closes libgit2#423
@dahlbyk
Copy link
Member Author

dahlbyk commented May 7, 2013

Here's a start, but I'm not sure how to approach testing. The only test where we actually inspect a full diff is CanCompareTwoVersionsOfAFileWithADiffOfTwoHunks, but I'm not sure how to test the impact of CompareOptions without ridiculous amounts of duplication. Thoughts?

@@ -479,7 +488,7 @@ public void ComparingTwoNullTreesReturnsAnEmptyTreeChanges()
{
using (var repo = new Repository(StandardTestRepoPath))
{
TreeChanges changes = repo.Diff.Compare(null, null, null);
TreeChanges changes = repo.Diff.Compare(default(Tree), default(Tree));
Copy link
Member

Choose a reason for hiding this comment

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

👍 Much more readable!

@yorah
Copy link
Contributor

yorah commented May 7, 2013

Here's a start, but I'm not sure how to approach testing.

@dahlbyk @nulltoken f7fa450 is a proposal to ease testing of patches. I believe that most of the times when reading tests related to patches, you don't care about having the exact content of the patch near the code (and if you want it, you can just open the corresponding .patch file in the resources folder).

{
using (var repo = new Repository(StandardTestRepoPath))
{
Tree rootCommitTree = repo.Lookup<Commit>("f8d44d7").Tree;
Tree commitTreeWithUpdatedFile = repo.Lookup<Commit>("ec9e401").Tree;

TreeChanges changes = repo.Diff.Compare(rootCommitTree, commitTreeWithUpdatedFile);
TreeChanges changes = repo.Diff.Compare(rootCommitTree, commitTreeWithUpdatedFile,
compareOptions: new CompareOptions { ContextLines = (short)contextLines });
Copy link
Member

Choose a reason for hiding this comment

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

Could we not force the consumer to pass in a short?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@dahlbyk
Copy link
Member Author

dahlbyk commented May 22, 2013

Great idea, @yorah! I rearranged f7fa450 a bit under the assumption that over time we'll collect more "expected" output:

  1. Renamed folder to simply "expected"
  2. Changed .patch to .diff since .patch files should have a commit header, which we'll probably need to test in its own right eventually
  3. Added a folder to group expected output relating to the same set of commits

@nulltoken
Copy link
Member

😍

@dahlbyk Could you please rebase this so it can be merged?

@yorah
Copy link
Contributor

yorah commented May 22, 2013

👍

* ContextLines
* InterhunkLines

Closes libgit2#423
@nulltoken nulltoken closed this in 96553df May 25, 2013
@nulltoken
Copy link
Member

@dahlbyk 💖

Could you please update your answer on SO to add a piece of code demonstrating your newest addition?

@dahlbyk
Copy link
Member Author

dahlbyk commented May 25, 2013

Could you please update your answer on SO to add a piece of code demonstrating your newest addition?

Done!

@dahlbyk dahlbyk deleted the gh423 branch May 25, 2013 13:08
@nulltoken
Copy link
Member

:Thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants