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

Cleanup edit groups after coalescing #259

Merged
merged 1 commit into from May 25, 2021
Merged

Cleanup edit groups after coalescing #259

merged 1 commit into from May 25, 2021

Conversation

dsnet
Copy link
Collaborator

@dsnet dsnet commented Apr 30, 2021

Even with an optimal diffing algoritm, coalescing adjacent edit groups
may cause the corresponding pair of strings for an edit group to have
leading or trailing spans of equal elements.

While this is technically a correct representation of a diff,
it is a suboptimal outcome. As such, scan through all unequal groups and
move leading/trailing equal spans to the preceding/succeeding equal group.

Before this change:

  strings.Join({
    "org-4747474747474747,bucket-4242424242424242:m,tag1=a,tag2=aa",
-   ",#=_value _value=2 ",
+   " _value=2 ",
    `11 org-4747474747474747,bucket-4242424242424242:m,tag1=a,tag2=bb`,
-   ",#=_value _value=2 2",
+   " _value=2 2",
    `1  org-4747474747474747,bucket-4242424242424242:m,tag1=b,tag2=cc`,
-   ",#=_value",
    ` _value=1 21 org-4747474747474747,bucket-4242424242424242:m,tag1`,
    "=a,tag2",
-   "=dd,#=_value",
+   "=dd",
    ` _value=3 31 org-4747474747474747,bucket-4242424242424242:m,tag1`,
-   "=c,#=_value",
+   "=c",
    ` _value=4 41 `,
  }, "")

After this change:

  strings.Join({
    "org-4747474747474747,bucket-4242424242424242:m,tag1=a,tag2=aa",
-   ",#=_value",
    ` _value=2 11 org-4747474747474747,bucket-4242424242424242:m,tag1`,
    "=a,tag2=bb",
-   ",#=_value",
    ` _value=2 21 org-4747474747474747,bucket-4242424242424242:m,tag1`,
    "=b,tag2=cc",
-   ",#=_value",
    ` _value=1 21 org-4747474747474747,bucket-4242424242424242:m,tag1`,
    "=a,tag2=dd",
-   ",#=_value",
    ` _value=3 31 org-4747474747474747,bucket-4242424242424242:m,tag1`,
    "=c",
-   ",#=_value",
    ` _value=4 41 `,
  }, "")

@dsnet dsnet requested a review from neild April 30, 2021 21:28
@dsnet
Copy link
Collaborator Author

dsnet commented Apr 30, 2021

\cc @rogpeppe, this PR will fix the sub-optimal printing of the diffs.

Copy link
Contributor

@rogpeppe rogpeppe left a comment

Choose a reason for hiding this comment

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

LGTM - this looks like it'll be a huge improvement, thanks!
Note: I haven't looked at the detail of the implementation properly I'm afraid, but I'm very happy to see this change.

cmp/report_slices.go Outdated Show resolved Hide resolved
cmp/report_slices.go Outdated Show resolved Hide resolved
neild
neild approved these changes May 24, 2021
Even with an optimal diffing algoritm, coalescing adjacent edit groups
may cause the corresponding pair of strings for an edit group to have
leading or trailing spans of equal elements.

While this is technically a correct representation of a diff,
it is a suboptimal outcome. As such, scan through all unequal groups and
move leading/trailing equal spans to the preceding/succeeding equal group.

Before this change:
    strings.Join({
      "org-4747474747474747,bucket-4242424242424242:m,tag1=a,tag2=aa",
  -   ",#=_value _value=2 ",
  +   " _value=2 ",
      `11 org-4747474747474747,bucket-4242424242424242:m,tag1=a,tag2=bb`,
  -   ",#=_value _value=2 2",
  +   " _value=2 2",
      `1  org-4747474747474747,bucket-4242424242424242:m,tag1=b,tag2=cc`,
  -   ",#=_value",
      ` _value=1 21 org-4747474747474747,bucket-4242424242424242:m,tag1`,
      "=a,tag2",
  -   "=dd,#=_value",
  +   "=dd",
      ` _value=3 31 org-4747474747474747,bucket-4242424242424242:m,tag1`,
  -   "=c,#=_value",
  +   "=c",
      ` _value=4 41 `,
    }, "")

After this change:
    strings.Join({
      "org-4747474747474747,bucket-4242424242424242:m,tag1=a,tag2=aa",
  -   ",#=_value",
      ` _value=2 11 org-4747474747474747,bucket-4242424242424242:m,tag1`,
      "=a,tag2=bb",
  -   ",#=_value",
      ` _value=2 21 org-4747474747474747,bucket-4242424242424242:m,tag1`,
      "=b,tag2=cc",
  -   ",#=_value",
      ` _value=1 21 org-4747474747474747,bucket-4242424242424242:m,tag1`,
      "=a,tag2=dd",
  -   ",#=_value",
      ` _value=3 31 org-4747474747474747,bucket-4242424242424242:m,tag1`,
      "=c",
  -   ",#=_value",
      ` _value=4 41 `,
    }, "")
@dsnet dsnet merged commit c5c3378 into master May 25, 2021
37 checks passed
@dsnet dsnet deleted the cleanup-groups branch May 25, 2021 00:33
Copy link

@AnomalRoil AnomalRoil left a comment

Choose a reason for hiding this comment

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

I know this is already merged, but since I'm reviewing our dependencies before updating, I'm just letting you know my thoughts ;)

Thanks for the comments, it helped a lot with the review process.

nx := ds.NumIdentical + ds.NumRemoved + ds.NumModified
ny := ds.NumIdentical + ds.NumInserted + ds.NumModified
var numLeadingIdentical, numTrailingIdentical int
for i := 0; i < nx && i < ny && eq(ix+i, iy+i); i++ {

Choose a reason for hiding this comment

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

The shadowing of i is not helping with readability.

for i := 0; i < nx && i < ny && eq(ix+i, iy+i); i++ {
numLeadingIdentical++
}
for i := 0; i < nx && i < ny && eq(ix+nx-1-i, iy+ny-1-i); i++ {

Choose a reason for hiding this comment

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

Same

@dsnet
Copy link
Collaborator Author

dsnet commented May 27, 2021

@AnomalRoil, I commend you for reviewing changes to your dependencies! This is something I wish people would do more often. Thank you.

@AnomalRoil
Copy link

Yup, and that's especially true when dealing with crypto & passwords, since these are quite sensitive.
So I try to do it for Gopass.

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

4 participants