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

Work with arbitrary files #220

Closed
HaleTom opened this issue Feb 23, 2017 · 29 comments
Closed

Work with arbitrary files #220

HaleTom opened this issue Feb 23, 2017 · 29 comments

Comments

@HaleTom
Copy link

HaleTom commented Feb 23, 2017

The output format is indeed preeedy... so pretty I want to use it with standard diff.

Would you consider supporting

diff -u file1 file2 | diff-so-fancy?
or
diff-so-fancy --files file1 file2

There is a workaround but it requires different commands if under git management or not.

It would be great to have one command that works no matter what the git status of the files is.

@scottchiefbaker
Copy link
Contributor

What doesn't work when you do diff -u file1 file2 | diff-so-fancy ?

When I try that on some local files it looks 90% correct. The only part that is not correct is the header line, and that's because we expect some git specific ANSI codes that aren't there. I still see the separator line, it's just not colored.

Can you provide a .diff file, or a screenshot of what doesn't work?

@HaleTom
Copy link
Author

HaleTom commented Feb 24, 2017

In this screenshot note issues with:

  • Header
  • Changed line and added line are not highlighted in green
  • No highlight of the empty line that was removed (the one replaced by "changed line")

Also note that with newer versions of diff supporting --color=always, the leading +/- is not removed.

@paulirish
Copy link
Member

I've been using --no-index for this trick for a while and haven't had problems.

@scottchiefbaker
Copy link
Contributor

I would definitely like to be able to handle:

  1. diff -u file1 file2 | diff-so-fancy
  2. cat my.patch | diff-so-fancy

@paulirish can you elaborate on your --no-index comment above? I'm not following.

@OJFord
Copy link
Member

OJFord commented Feb 27, 2017

@scottchiefbaker I assume he's talking about doing git diff --no-index file1 file2 for this, which should work, with the minor drawback that the header will always be "renamed: file1 to file2".

@paulirish
Copy link
Member

paulirish commented Feb 28, 2017 via email

@scottchiefbaker
Copy link
Contributor

Oh that's cool. I had now idea you could use git to diff files that aren't in a repo. Since we're able to use git like this, making d-s-f to work with it shouldn't be too hard.

How would we want it to look?

Option 1

diff -u file1 file2 | diff-so-fancy
# or 
cat /tmp/mypatch.diff | diff-so-fancy

These seem logical to me, but we would have to add some logic to d-s-f to detect that this code snippet is NOT from a repo, and handle the rename header issue that @OJFord mentioned. Maybe add some sort of --simple option to indicate this? Is there a way to detect that the STDIN is just a vanilla diff and not gitified?

Option 2

diff-so-fancy --files file1 file2

This seems slightly less intuitive, but would be a lot easier to implement. We just modify d-s-f to pass the two files it sees to git diff --no-index and set some flag that handles the above mentioned rename issue.

@OJFord
Copy link
Member

OJFord commented Feb 28, 2017

If I may,

Option 3

diff-so-fancy file1 file2

If stdin is a file descriptor, normal git behaviour, else do this. Or not even check stdin - just look for args that aren't --option -s.

I think I like option 1 most though, it keeps dsf as the thing which does the fancifying - it's more UNIXey.

@OJFord
Copy link
Member

OJFord commented Feb 28, 2017

In re detecting if a git diff - easiest way would be to rev-parse on one of the file paths.

@scottchiefbaker
Copy link
Contributor

The problem I see with option 1 is that it requires some git. D-s-f relies on git to colorize the diffed lines (d-s-f reads that and massages them), which is why diff -u file1 file2 | diff-so-fancy sort-of works, it's lacking the line colorization that git provides.

We could work around that by just reading the output, scraping the file names and calling git diff --no-index file1 file2 and d-s-f'ing that. That would probably work... UNTIL someone decides to try and feed d-s-f some massive multi-file patch which would definitely then fail.

@OJFord
Copy link
Member

OJFord commented Feb 28, 2017

Iirc, diff-highlight can be invoked without git.

@HaleTom
Copy link
Author

HaleTom commented Mar 1, 2017

I've updated the workaround (see top post) to create a bash function:

dsf() { git diff --no-index --color "$@" | diff-so-fancy }

My preference now goes to option 3. It would be easy to detect if there were not exactly 2 non-option arguments and print a usage summary.

diff-so-fancy [-s] [file1 file2]

I would also love to be able to: cat patch | diff-so-fancy ( Modification of option 1)

@stevemao
Copy link
Member

stevemao commented Mar 3, 2017

make diff-so-fancy as a standalone cli command sounds great. Would like to see at least some information printed when running it instead of justing hanging there.

arcticicestudio added a commit to arcticicestudio/igloo that referenced this issue Nov 15, 2017
"diff-so-fancy" (1) has been designed to work within Git controlled
directories. To allow to use it to compare files that are not controlled
by Git the "--no-index" (2) option of "git-diff" has been used.

The script pipes the output to "less" using the options

* "-R, --RAW-CONTROL-CHARS" to only display ANSI "color" escape
  sequences in "raw" form
* "-F, --quit-if-one-screen" to automatically exit if the entire file
  can be displayed on the first screen.
* "-X, --no-init" to disable sending the termcap (de)initialization
  strings to the terminal to avoid unnecessary operations like clearing the screen.
* "-x2, --tabs=2" to use two tab stops.

References:

  * so-fancy/diff-so-fancy#220 (comment)
  (1) https://github.com/so-fancy/diff-so-fancy
  (2) https://git-scm.com/docs/git-diff#git-diff-emgitdiffem--no-index--options--ltpathgt823082
arcticicestudio added a commit to arcticicestudio/igloo that referenced this issue Nov 15, 2017
"diff-so-fancy" (1) has been designed to work within Git controlled
directories. To allow to use it to compare files that are not controlled
by Git the "--no-index" (2) option of "git-diff" has been used.

The script pipes the output to "less" using the options

* "-R, --RAW-CONTROL-CHARS" to only display ANSI "color" escape
  sequences in "raw" form
* "-F, --quit-if-one-screen" to automatically exit if the entire file
  can be displayed on the first screen.
* "-X, --no-init" to disable sending the termcap (de)initialization
  strings to the terminal to avoid unnecessary operations like clearing the screen.
* "-x2, --tabs=2" to use two tab stops.

References:

  * so-fancy/diff-so-fancy#220 (comment)
  (1) https://github.com/so-fancy/diff-so-fancy
  (2) https://git-scm.com/docs/git-diff#git-diff-emgitdiffem--no-index--options--ltpathgt823082

GH-61
@scottchiefbaker
Copy link
Contributor

I'm back looking at this... what types of formats do we want to support?

@HaleTom
Copy link
Author

HaleTom commented Jan 1, 2018

What more would be needed apart from diff / patch?

@scottchiefbaker
Copy link
Contributor

The problem I've run in to with this is that Git handles the line add/remove coloring. If we remove Git from the equation because we're processing standard diff/patch files d-s-f will have to do that line coloring itself. That's also doable, if d-s-f knows it's in standalone file mode. Detecting that may be complicated. I propose we do the following

git diff | diff-so-fancy     # Read input from STDIN and assumes git

diff-so-fancy /tmp/foo.patch # Read $ARGV[0] as a file and process it in standalone mode

That might break people's expectations if they expect something like this to work:

diff -u a.txt b.txt | diff-so-fancy 

If we're just reading from STDIN it's really hard for d-s-f to know if the difference between Git output, and a regular patch file. Ideally the third diff syntax above, but we'll have to get creative about detecting if we're in standalone mode.

scottchiefbaker added a commit to scottchiefbaker/diff-so-fancy that referenced this issue Jan 3, 2018
@scottchiefbaker
Copy link
Contributor

I did some more thinking, and wrote up an initial implementation on next. I think the only sane way to handle arbitrary files is via STDIN. For consistency everything will be of the form:

git diff | diff-so-fancy
cat foo.patch | diff-so-fancy
diff -u a.txt b.txt | diff-so-fancy
hg diff | diff-so-fancy

If you check out the next branch you'll see support for both Mercurial (#193) as well as standalone files mentioned above. If you can test that branch and provide feedback I'd love to hear it.

@scottchiefbaker
Copy link
Contributor

@HaleTom @OJFord @paulirish

Anyone had a chance to test this yet?

@paulirish
Copy link
Member

I haven't, no.

I was also excited for diff-so-fancy file1 file2. Did you run into problems with that approach?

@scottchiefbaker
Copy link
Contributor

@paulirish doing diff-so-fancy file1 file2 is less consistent syntax-wise and would muddy up the code logic. I tried to keep everything the same format. If there are strong feelings about that I could look at it again, but my preference is to process everything through STDIN.

@OJFord
Copy link
Member

OJFord commented Jan 22, 2018

doing diff-so-fancy file1 file2 is less consistent syntax-wise

Less consistent with what? GNU diff and git diff both operate like that.

@scottchiefbaker
Copy link
Contributor

Consistent in that d-s-f is called the same way for all the different methods:

git diff | diff-so-fancy
cat foo.patch | diff-so-fancy
diff -u a.txt b.txt | diff-so-fancy
hg diff | diff-so-fancy

It's always reading piped in data via STDIN, whether it's from git, diff, or even cat. It certainly simplifies the logic in the code, and I think it's easier to remember.

"For git I do git diff | d-s-f, but this is a raw patch file so do I do d-s-f < raw.patch" etc... No need to stop and think about what mode you're in. Everything is via a pipe.

@stevemao
Copy link
Member

Works for me. Once we document it we should make a release :)

@staabm
Copy link

staabm commented Jan 30, 2018

not 100% sure, but I guess its the same use-case.

I want diff-so-fancy to colorize a patch file I created with a diff command beforehand.

$ cat db.patch
Index: config/db.php
===================================================================
--- config/db.php       (Revision 865)
+++ config/db.php       (Arbeitskopie)
@@ -19,6 +19,6 @@
 }

 /**
- * Global DB Config11
+ * Global DB Config11345
  */
-define("FOREIGN_KEY_SEPERATOR", "");
\ No newline at end of file
+define("FOREIGN_KEY_SEPERATOR", "");

cat db.patch | diff-so-fancy doesnt work for me

@scottchiefbaker
Copy link
Contributor

@staabm I just tested and it works for me. Are you using the next branch? I'm very close to releasing a 1.2.1 which will be based on next. Just looking for some more testing before I release.

@staabm
Copy link

staabm commented Jan 30, 2018

I am using the latest stable version.

@scottchiefbaker
Copy link
Contributor

@staabm this is a new unreleased feature. If you'd like to help us test you'll need to use the next branch

@RyanKoppenhaver-NCC
Copy link

In addition to the things tweaked in the PR above, one thing that would be nice to change is that currently the filename line

diff -ur old/modified.txt new/modified.txt

gets turned into

renamed: old/modified.txt to new/modified.txt

instead of

modified: modified.txt

....but I didn't attempt to fix that because I'm not sure what sorts of corner cases are going to come up in the process.

@scottchiefbaker
Copy link
Contributor

This issue has been partially addressed in the just released v.1.2.5. In order to keep things clean I'm closing this issue. If we need to let's create a new issue for further improvements to this mechanism.

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

No branches or pull requests

7 participants