Arbitrary diff improvements #293
Conversation
|
👍 for passing all the existing tests with this This is looking merge-worthy assuming we can get some clarification in comments of what certain things do. I'm pretty impressed! |
|
I was just diffing a diff yesterday so I'm super jazzed about this. Nice stuff, @RyanKoppenhaver-NCC ! |
|
@paulirish what was your use case? Something like: |
|
Ping... @paulirish and @RyanKoppenhaver-NCC looking for feedback. |
|
@scottchiefbaker I'm happy to elaborate, but I'm not sure if you're asking for additional code comments, or comments on this PR. Anything specific questions? |
|
@RyanKoppenhaver-NCC I put some comments/questions on specific lines of code in this PR. Can you respond to those? |
|
@scottchiefbaker my usecase is typically this: git diff --no-index fileA fileBbut sometimes i'm falling back to bare diff -u fileA fileB |
I'm not seeing those comments over here, FYI. |
|
No? Here is my view:
|
|
Yeah, I'm not seeing inline comments either. |
|
Ah, it says here that they're not visible until you submit the review: https://help.github.com/articles/reviewing-proposed-changes-in-a-pull-request/ |
diff-so-fancy
Outdated
| } else { | ||
| $last_file_seen = $5; | ||
| $diff_args =~ s/\e.*$//; |
There was a problem hiding this comment.
What do these two lines do? Can you at least add a comment on what these regexp replaces accomplish. Maybe a before and after so we know what the strings should look like?
diff-so-fancy
Outdated
| # Git looks like: diff --git a/diff-so-fancy b/diff-so-fancy | ||
| } elsif ($diff_args =~ /^(?:--git|--cc) (.+?)(\s|\e|$)/) { |
There was a problem hiding this comment.
?: is a non-capturing parenthesis right? I don't use this syntax very often so this might confuse me in the future. Is there a reason to do this, and not just have $last_file_seen = $2 ?
There was a problem hiding this comment.
Yep. I used it out of a vague sense that it's more efficient, but even if it is, I'm sure it's trivial, so I'll switch it.
|
@RyanKoppenhaver-NCC good catch. I just submitted, do you see them now? |
Addresses review comments.
|
@scottchiefbaker can this be merged and released please 😄 ? |
|
@veggiemonk thanks for reminding me this was still open. My feedback still stands for @RyanKoppenhaver-NCC . If he can address the changes I suggested we can certainly revisit this. Also, a LOT of work has gone in to handling standalone diffs in the |
|
Oh I see @RyanKoppenhaver-NCC did reply but it got buried in the GitHub UI. Can you provide a before and after screenshot of exactly what code this addresses. I don't use vanilla Also, is this code still relevant, or does it need to be updated (original submission was March 22nd) to work with |
|
@scottchiefbaker I understand it might be quite outdated but a new release would be nice. What do you think ? The latest release is from 2017 |
|
I'm definitely overdue for a release, I just want to make sure this code is still in good shape (no bitrot) before we merge it. |
|
Version 1.2.5 has been released and should address part of this issue. There is some bitrot here so I'm going to close this particular issue. I'm 100% open to accepting this in the future once it's reworked against version 1.2.5. |
I found that the existing work on #220 didn't really work well for me with with the output of plain
diff(1), particularly recursive diffs. These changes fix the matching on the filename line (which didn't allow for the-uflag), and add colorization to hunk markers and "Only in ..." lines.