Skip to content

Expose a more generic DiffAlgorithm property in CompareOptions #1043

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

Merged
merged 1 commit into from
May 27, 2015

Conversation

nulltoken
Copy link
Member

Following #1039 and in order to avoid future breaking changes maybe would it be wise to expose a property accepting an enum of Diff algorithms (Meyer, Minimal, ...).

/cc @dmalikov

  • Add new property
  • Obsolete current property
  • Implement minimal tests to cover each algorithm

@dmalikov
Copy link
Contributor

dmalikov commented May 9, 2015

Ok, sure, why not

@nulltoken
Copy link
Member Author

Ready for review

public bool UsePatienceAlgorithm { get; set; }

/// <summary>
/// Algorithm to be used when performing a Diff.
Copy link
Member

Choose a reason for hiding this comment

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

(Default = Algorithm.Meyers)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@Therzok
Copy link
Member

Therzok commented May 23, 2015

Other than missing implementation for minimal diff, that's all I can say.

@nulltoken
Copy link
Member Author

@Therzok I've been refraining from implementing the --minimal algo as I haven't found a proper concise example of its output (which should differ from what would be computed through Meyer or Patience).

Any idea?

@Therzok
Copy link
Member

Therzok commented May 23, 2015

Only seems to do things for big diffs. I just did:

➜  monodevelop git:(master) git diff --diff-algorithm=minimal HEAD~600 HEAD | wc -l
   77496
➜  monodevelop git:(master) git diff --diff-algorithm=myers HEAD~600 HEAD | wc -l
   79049

Returned same results until around ~600 commits, and I tracked down what changed the stuff. It seems to be XML that's the culprit of making minimal better, but only in a big diff context.

Here's the diff between the two.

https://gist.github.com/Therzok/0f1f127ab2c9bb95681b

They make chrome choke when trying to gist them:
https://www.dropbox.com/s/dsmizfjupayh6sm/diff-min.diff?dl=0
https://www.dropbox.com/s/rh7kfxo5b9w4fwo/diff-myers.diff?dl=0

Differences also happen in libgit2sharp repo with ~300 commits from HEAD.

➜  libgit2sharp git:(therzok/mono4) git diff --diff-algorithm=minimal HEAD~300 HEAD | wc -l
   38993
➜  libgit2sharp git:(therzok/mono4) git diff --diff-algorithm=myers HEAD~300 HEAD | wc -l
   39058

https://gist.github.com/Therzok/8c8a87c4586d428513b8

@Therzok
Copy link
Member

Therzok commented May 23, 2015

In short it only affects huge diffs. This would be useful for someone diffing HEAD to 1 year prior, but not many people do this. So it would be best to keep it.

Short things short: minimal is just myers with better diffs for huge stuff.

Edit: The time difference is actually not noticeable.

@nulltoken
Copy link
Member Author

Proposal: Merge this and log an up-for-grabs issue for Minimal implementation. Eventually @dmalikov's question may get an answer...

nulltoken added a commit that referenced this pull request May 27, 2015
Expose a more generic DiffAlgorithm property in CompareOptions
@nulltoken nulltoken merged commit ef63b09 into vNext May 27, 2015
@nulltoken nulltoken deleted the ntk/diff branch May 27, 2015 07:28
@nulltoken nulltoken added this to the v0.22 milestone May 27, 2015
@nulltoken
Copy link
Member Author

Published as NuGet pre-release package LibGit2Sharp.0.22.0-pre20150527180327

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

Successfully merging this pull request may close these issues.

3 participants