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

Add context diff support #15

Closed
jan-matejka opened this issue Mar 13, 2013 · 14 comments
Closed

Add context diff support #15

jan-matejka opened this issue Mar 13, 2013 · 14 comments

Comments

@jan-matejka
Copy link

http://en.wikipedia.org/wiki/Diff#Context_format

@ymattw
Copy link
Owner

ymattw commented Mar 14, 2013

This is in my plan however not able to implement quickly, I will get back
to this soon. Thanks for support.

On Wednesday, March 13, 2013, yaccz wrote:

http://en.wikipedia.org/wiki/Diff#Context_format


Reply to this email directly or view it on GitHubhttps://github.com//issues/15
.

ymattw added a commit that referenced this issue Mar 16, 2013
@ymattw
Copy link
Owner

ymattw commented Mar 16, 2013

Could you describe what is expected use case for context diff? I want to understand how robust the parse need to be to evaluate the ROI as the fix is much difficult than I expected, the Hunk and DiffOps abstraction need refactor.

@myint
Copy link
Contributor

myint commented Mar 16, 2013

I just noticed filterdiff. You guys probably already know about this. But just in case you don't:

filterdiff --format=unified < context_diff | cdiff

@jan-matejka
Copy link
Author

@ymattw unidiff is not standardized, while context diff is in posix. So I expect posix compliant projects to prefer using context diffs, like postgresql.

@ymattw
Copy link
Owner

ymattw commented Mar 18, 2013

@myint Thanks for the info, that tool make things easier.

@jan-matejka
Copy link
Author

I would be perfectly happy with just detecting a context diff and doing filterdiff --format=unified < context | cdiff in the background

@ymattw
Copy link
Owner

ymattw commented Mar 19, 2013

Tried filterdiff with the example at http://en.wikipedia.org/wiki/Diff#Context_format, looks like it is broken. FYI someone put patchutils on github here.

$ cat 1.cdiff

*** /path/to/original   ''timestamp''
--- /path/to/new        ''timestamp''
***************
*** 1,3 ****
--- 1,9 ----
+ This is an important
+ notice! It should
+ therefore be located at
+ the beginning of this
+ document!
+
  This part of the
  document has stayed the
  same from version to
***************
*** 5,20 ****
  be shown if it doesn't
  change.  Otherwise, that
  would not be helping to
! compress the size of the
! changes.
!
! This paragraph contains
! text that is outdated.
! It will be deleted in the
! near future.

  It is important to spell
! check this dokument. On
  the other hand, a
  misspelled word isn't
  the end of the world.
--- 11,20 ----
  be shown if it doesn't
  change.  Otherwise, that
  would not be helping to
! compress anything.

  It is important to spell
! check this document. On
  the other hand, a
  misspelled word isn't
  the end of the world.
***************
*** 22,24 ****
--- 22,28 ----
  this paragraph needs to
  be changed. Things can
  be added after it.
+
+ This paragraph contains
+ important new additions
+ to this document.

$ cat 1.cdiff | filterdiff --format=unified

--- /path/to/original   ''timestamp''
+++ /path/to/new        ''timestamp''
@@ -1,3 +1,9 @@
+This is an important
+notice! It should
+therefore be located at
+the beginning of this
+document!
+ This part of the
 document has stayed the
 same from version to
@@ -5,16 +11,10 @@

@myint
Copy link
Contributor

myint commented Mar 19, 2013

It looks like that is due to the trailing whitespace after the ! symbols being removed.

You can see that by generating a context diff from the unified diff on that same WIkipedia page.

$ filterdiff --format=context < unified_diff > generated_context
$ cat generated_context
*** /path/to/original   ''timestamp''
--- /path/to/new        ''timestamp''
***************
*** 1,3 ****
--- 1,9 ----
+ This is an important
+ notice! It should
+ therefore be located at
+ the beginning of this
+ document!
+ 
  This part of the
  document has stayed the
  same from version to
***************
*** 5,20 ****
  be shown if it doesn't
  change.  Otherwise, that
  would not be helping to
! compress the size of the
! changes.
! 
! This paragraph contains
! text that is outdated.
! It will be deleted in the
! near future.

  It is important to spell
! check this dokument. On
  the other hand, a
  misspelled word isn't
  the end of the world.
--- 11,20 ----
  be shown if it doesn't
  change.  Otherwise, that
  would not be helping to
! compress anything.

  It is important to spell
! check this document. On
  the other hand, a
  misspelled word isn't
  the end of the world.
***************
*** 22,24 ****
--- 22,28 ----
  this paragraph needs to
  be changed. Things can
  be added after it.
+ 
+ This paragraph contains
+ important new additions
+ to this document.

$ filterdiff --format=unified < generated_context 
--- /path/to/original   ''timestamp''
+++ /path/to/new        ''timestamp''
@@ -1,3 +1,9 @@
+This is an important
+notice! It should
+therefore be located at
+the beginning of this
+document!
+
 This part of the
 document has stayed the
 same from version to
@@ -5,16 +11,10 @@
 be shown if it doesn't
 change.  Otherwise, that
 would not be helping to
-compress the size of the
-changes.
-
-This paragraph contains
-text that is outdated.
-It will be deleted in the
-near future.
+compress anything.

 It is important to spell
-check this dokument. On
+check this document. On
 the other hand, a
 misspelled word isn't
 the end of the world.
@@ -22,3 +22,7 @@
 this paragraph needs to
 be changed. Things can
 be added after it.
+
+This paragraph contains
+important new additions
+to this document.

@ymattw
Copy link
Owner

ymattw commented Mar 19, 2013

@myint You rock! lines contain only a !, + or Space were all missing a trailing blank.

This tool seems installed on RHEL (at least on >= 5.6) and available in debian/ubuntu package repo, not sure other linux distros, it is also available on MacPorts. It most likely not installed by default, I am not sure invoking filterdiff in background to do the converting is a good idea, @yaccz how about do a lazy fix, detect context diff and print out guidance points to filterdiff? The context-diff branch already has the detection integrated.

@jan-matejka
Copy link
Author

@myint nice catch.

@ymattw what's the issue with it? And how is just displaying info about filterdiff different from just running filterdiff in background besides annoying the user? I'd use it as an optional dependency.

@ymattw
Copy link
Owner

ymattw commented Mar 20, 2013

What do you expect when filterdiff does not exist?

On Tuesday, March 19, 2013, yaccz wrote:

@myint https://github.com/myint nice catch.

@ymattw https://github.com/ymattw what's the issue with it? And how is
just displaying info about filterdiff different from just running
filterdiff in background besides annoying the user? I'd use it as an
optional dependency.


Reply to this email directly or view it on GitHubhttps://github.com//issues/15#issuecomment-15121013
.

@myint
Copy link
Contributor

myint commented Mar 20, 2013

Maybe print out a message like "Context diff support depends on filterdiff".

(subprocess.Popen() will raise an OSError if the program you execute does not exist so you can catch that.)

@jan-matejka
Copy link
Author

@ymattw as @myint says.

@ymattw ymattw closed this as completed in e7854dd Mar 21, 2013
@ymattw
Copy link
Owner

ymattw commented Mar 21, 2013

Sorry for late fix, too much occupied these days. I feel the fix isn't elegant, enhancement welcome. Document will be updated later.

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

No branches or pull requests

3 participants