Support vanilla diff recursive flag

[brewsfab]

Thank you so much for the great project you brought to us.

Needed support for the recursive flag so I made this improvement thinking it could help more people.
It basically treats the vanilla diff with recursive flag as Mercurial.

• Related to Support for recursive flag #394
• All tests are passing.

[scottchiefbaker]

I consider myself fairly well versed in regular expressions, but man what you wrote is beyond me :)

I'd like the comment above the regexp adjusted so the $4 and $5 stuff line up with the appropriate parens, and then a comment added explaining the logic of the regexp so when someone has to look at it in the future we'll have a starting point.

[scottchiefbaker]

Otherwise things look good. Clean diff and solid unit tests.

[brewsfab]

Sorry for missing that part about the patterns positions.
Since the $4 is very long I have decided to add some decoration to show its scope.
I also had some comments about the regex, I hope it is easy to understand.
Let me know!

[scottchiefbaker]

This looks good, and a lot clearer to me :)

What string are we actually matching against in a recursive diff. Do you have an example?

[brewsfab]

Basically the vanilla uses the -r or --recursive flag for recursive diff.
So in the regex, we are matching either r if it is part of a shorthand expression (preceded by single dash) or the string --recursive itself.
In that case, we match the string r in all the shorthands combinations regardless of its position like in diff --color -r -u or in diff -b -r -u or diff -bru etc. The reason for matching only the r in the group instead of the -r as initially is because r could be as any position from the preceding -.
They might be a way to still capture the -r in something like diff -bru but I am not good enough in regex to find it.

For the longhand --recursive string we match it as is like in diff --color -u --recursive or diff --ignore-space-change --recursive --color -u.

[scottchiefbaker]

Since all we're trying to do is capture -r or --recursive somewhere after the diff wouldn't it be easier to just do:

(\b-r\b)|(--recursive)|(--git)|(--cc))

That will ensure the -r is not part of another word and not a double --, and would be more readable?

[brewsfab]

Sorry I was not clear in the answer. My bad.
In fact, all the variants below are valid for recursive diff.

1. diff -ur folder1/filer1 folder2/filer1
2. diff -u -r folder1/filer1 folder2/filer1
3. diff --recursive folder1/filer1 folder2/filer1
4. diff --color -u -r folder1/filer1 folder2/filer1
5. diff --color -r -u folder1/filer1 folder2/filer1
6. diff --color --recursive folder1/filer1 folder2/filer1
7. diff -bur folder1/filer1 folder2/filer1
8. diff -rub folder1/filer1 folder2/filer1
9. diff -r -ub folder1/filer1 folder2/filer1
10. diff -bru folder1/filer1 folder2/filer1

So the current proposal covers all 10 variants. regex101.com complex sample
Whereas the more readable

diff (-r\b|--recursive|--git|--cc) (.*?)(\e| b\/|$)

will cover only the number 3 and number 9 variants. regex101.com readable sample

I think there should be a trade-off between readability and adaptability.

Since it is user managed more than the git diff which is somehow standardized as diff --git at the beginning of the line, there might be different flavors for the diff recursive.
I guess there are people like me with alias configuration as:

alias diffr='diff -br' #diff recursive ignoring whitespace changes

or much more complex.
So for us, doing diffr -u folder1 folder2 | diff-so-fancy will fail to use the power of diff-so-fancy.

Also forgetting to remember to use the flag -r separated made me re-run the diff a second time every time to fix it.

So, if the choice is the readability one option to consider is to enforce/clarify in the docs to use -r (separated) or --recursive flags as first arguments.
So people will change their default configuration for diff recursive to something like

alias diffr='diff -r -b <extra_args>' #diff recursive ignoring whitespace changes

or

alias diffr='diff --recursive -b <extra_args>' #diff recursive ignoring whitespace changes

Let me know about your choice so I will make change to the PR to suit if needed.

[scottchiefbaker]

Let me start with... dang you are thorough! I love it. I wish all of our contributions were as thorough and well designed as this.

I personally don't use --recursive so maybe we should start with "what problem are we trying to solve". Do people use --recursive in their git workflow, or is this more a vanilla diff thing?

I'm leaning towards just going the KISS route and only supporting the simplest use case, because it will make the code a lot easier to maintain. This issue has not been a major sticking point for a lot of people, so I don't get the impression a lot of people use it. I might be wrong there though.

[brewsfab]

Thank you very much. I just wanted to be as clear as possible. Actually, I thought you would have been bored reading such long comment 😅

You are right asking the main question "what problem are we trying to solve".
In fact, I can not myself evaluate how many people are unable to leverage diff-so-fancy for the recursive because it is a vanilla diff thing and not so common.
I started to work on a proposal after searching for recursive mode and ended up on the issue #436.
But to be honest, it is not a hot topic looking at the lack of comments and reactions to the issue.
So, I would like to go on the KISS route as well. Therefore let's discard my proposal.

However, I think it will be helpful to have a comment in the doc for stating the support for recursive with vanilla diff as below.

With diff

Use -u with diff for unified output, and pipe the output to diff-so-fancy:

diff -u file_a file_b | diff-so-fancy

It also supports the recursive mode of diff with -r as first argument

diff -r -u file_a file_b | diff-so-fancy

Adding the first argument will let people to even tweak their config if needed to accommodate.

What do you think ?

[scottchiefbaker]

@brewsfab to hit on everything I was thinking. No need to over-complicate the code a simple feature. I think we should have support for two calling methods:

diff -r -u file_a file_b | diff-so-fancy
or
diff --recursive -u file_a file_b | diff-so-fancy

and update the documentation to reflect that the recursive flag needs to be immediately after the diff like you mentioned. I think that is a good compromise if simplicity and feature support. If you want to update this pull request to reflect that I will land it.

[brewsfab]

Thanks.

I have simplified the regex and added the comment as requested.
Please verify.

[scottchiefbaker]

Looks great... thanks for all the hard work and the revisions!

[Kumm-Kai]

It would be great if the instructions from the readme would also be added to the --help output.

[scottchiefbaker]

Pull requests welcome