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

Mercurial support #193

Closed
wants to merge 1 commit into from
Closed

Mercurial support #193

wants to merge 1 commit into from

Conversation

frantic
Copy link

@frantic frantic commented Aug 15, 2016

This will help with Mercurial compatibility – it doesn't output index line, and diff-so-fancy depends on it to draw the first horizontal line.

All bats tests pass.

Fixes #186

@frantic
Copy link
Author

frantic commented Aug 15, 2016

Related to #38. Maybe I should add a new test for the Mercurial usecase?

@frantic frantic changed the title Don't depend on 'index' in diff output Mercurial support Aug 15, 2016
@@ -55,13 +55,15 @@
# Look for git index and replace it horizontal line (header later) #
####################################################################
if ($line =~ /^${ansi_color_regex}index /) {
# Print the line color and then the actual line
$horizontal_color = $1;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a pretty crucial part of the git diff logic. It captures the color of the header that we use later. Simply skipping this WILL cause output coloring issues.

Copy link
Author

Choose a reason for hiding this comment

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

But all the automated tests pass, and when trying it out locally with git the coloring was fine. I guess what we could do is we could capture the color here but print the horizontal line when getting to diff --git part. This will require additional checks because mercurial does not print index at all.

@scottchiefbaker
Copy link
Contributor

Are you still willing to work on this? I'd like to discuss options moving forward.

@frantic
Copy link
Author

frantic commented Feb 28, 2017

@scottchiefbaker – I've been using this locally for a while now and I'm pretty happy with it. Would be awesome to get this upstreamed. Are you willing to merge this in? If so I'll spend some time rebasing it

@scottchiefbaker
Copy link
Contributor

Ya I'd be willing to merge this pending some tweaks and testing on my side.

I don't use mercurial, so I'll have to rely on your for that. As long as these changes don't break the git pieces I'm ok with that.

@scottchiefbaker
Copy link
Contributor

@frantic where are we at on this? Just going through old issues trying to clean up.

@frantic
Copy link
Author

frantic commented Dec 28, 2017

Hey @scottchiefbaker! I've been using this without modifications for 1.5 years now. You mentioned in your last comment that you'll do some testing on your side?

If you are still onboard I'll try rebasing

@scottchiefbaker
Copy link
Contributor

Yeah go ahead and do that and I'll do some testing on my side!

@frantic
Copy link
Author

frantic commented Dec 28, 2017

A quick update. Basically git uses this:

diff --git a/diff-so-fancy b/diff-so-fancy
index e5b1ef0..925bf26 100755
--- a/diff-so-fancy
+++ b/diff-so-fancy
@@ -116,7 +116,7 @@ sub do_dsf_stuff {

Mercurial uses this:

diff -r 7bedd0c41e23 -r 120d1b486a5f diff-so-fancy
--- a/diff-so-fancy	Wed Dec 27 16:20:27 2017 -0800
+++ a/diff-so-fancy	Wed Dec 27 18:09:27 2017 -0800
@@ -116,7 +116,7 @@

I guess I could refactor the code a little to not duplicate the logic, but don't have time at the moment.

@scottchiefbaker
Copy link
Contributor

I can look at tweaking the code... can you get me a simple diff out from Mercurial? Something like hg diff HEAD~1 or whatever the equivalent is. I need the complete output to make sure we're compatible.

If it's like you list above it should be super minor changes. I'm pretty confident we can make it work.

@scottchiefbaker
Copy link
Contributor

I found a sample hg repo and used it as a base... I think I have a rough implementation here. Can you test that and let me know?

screenshot_20171228_152758

@frantic
Copy link
Author

frantic commented Dec 29, 2017

@scottchiefbaker your version works great with my hg show! However I found my hg diff has the following header

diff --git a/test.js b/test.js
--- a/test.js
+++ b/test.js
@@ -17,3 +17,4 @@

i.e. no "index" and no "-r" either

@scottchiefbaker
Copy link
Contributor

That looks like pretty standard git output, minus the index line. It should already work with d-s-f. Are you saying it doesn't?

@frantic
Copy link
Author

frantic commented Dec 29, 2017

Because the index line and "-r" are missing, there's no initial line (first output is hg show, second output is hg diff):

image

in raw format:

$ hg show
diff -r 000000000000 -r 7d10fc5fa483 test.js
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test.js	Fri Dec 29 12:21:40 2017 -0800
@@ -0,0 +1,1 @@
+console.log({a: 1})

$ hg diff
diff --git a/test.js b/test.js
--- a/test.js
+++ b/test.js
@@ -1,1 +1,2 @@
 console.log({a: 1})
+console.log({b: 1})

@scottchiefbaker
Copy link
Contributor

Try a git pull I think I just fixed this.

@frantic
Copy link
Author

frantic commented Dec 29, 2017

Still seeing no line before the file name…
image

@scottchiefbaker
Copy link
Contributor

Even with 0e73558a35a8d45e7014925242ea398426694128? That's weird... can you post your full output

hg diff --color always > /tmp/out.txt

I need the ANSI codes to be included.

@frantic
Copy link
Author

frantic commented Dec 29, 2017

hgdiff.txt

@scottchiefbaker
Copy link
Contributor

Try pulling again... I just landed 4fcb82013f988798454395bbf9ff1ea6a4650606

@frantic
Copy link
Author

frantic commented Dec 29, 2017

Yay!

image

@scottchiefbaker
Copy link
Contributor

Ok great!

Play around with it for a couple of days and make sure it works for you. I'll land it on next next week if we're happy with it. I haven't noticed any side effects on the Git parsing, which is our primary focus.

@frantic
Copy link
Author

frantic commented Dec 29, 2017

Awesome, thank you!

@scottchiefbaker
Copy link
Contributor

@frantic have you had a chance to test things on your side? I know it's been a long holiday weekend, so you may not have had time yet.

I'd like to merge this if you think it's good enough. I don't use mercurial, so I don't have an easy way to verify this.

scottchiefbaker added a commit to scottchiefbaker/diff-so-fancy that referenced this pull request Jan 3, 2018
@frantic
Copy link
Author

frantic commented Jan 4, 2018

@scottchiefbaker sorry for the delay! I tested a few commands and the results look amazing! I'll give it another try next week when I resume heavy work in Mercurial repo, this should help me test edge cases more.

@scottchiefbaker
Copy link
Contributor

I've merged all my changes in to next. Barring anything crazy this will land in the next official release. That said we don't need this pull request any more so I'm closing it.

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.

2 participants