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

🐛Handle diff.submodule log #60

Closed
fdcds opened this issue Nov 22, 2019 · 4 comments · Fixed by #62
Closed

🐛Handle diff.submodule log #60

fdcds opened this issue Nov 22, 2019 · 4 comments · Fixed by #62
Labels
bug Something isn't working

Comments

@fdcds
Copy link

fdcds commented Nov 22, 2019

It appears that delta has some trouble finding the end of a hunk:

--- a   2019-11-27 12:06:18.472973416 +0100
+++ b   2019-11-27 12:06:23.992967043 +0100
@@ -1,7 +1,7 @@
 x
 y
 z
-a
+b
 z
 y
 x
Submodule x/y/z contains untracked content

Is displayed (roughly) as:

a
---------------------------------------
-----|
 ... |
-----|
1
 x
 y
 z
-a
+b
 z
 y
 x
 ubmodule x/y/z contains untracked content

Note how "Submodule"'s first character gets removed by delta.

Using delta 0.0.14.

@fdcds fdcds changed the title Use hunk sizes to determine what to mangle Improve hunk size detections and do not mangle lines outside a hunk Nov 22, 2019
@fdcds fdcds changed the title Improve hunk size detections and do not mangle lines outside a hunk Improve hunk size detection and do not mangle lines outside a hunk Nov 22, 2019
@fdcds
Copy link
Author

fdcds commented Nov 22, 2019

Proposal how to fix this: A hunk in unified diff starts with ^@@. That line already contains all the information you need: The first number is the line the hunk starts on (irrelevant for you) and the second number (after the comma) is the size of the hunk before (-) / after (+) the changes. So from there you count the number of lines starting with (keep line), - (remove) and + (add). Then num_keep + num_remove == hunk_size_before and num_keep + num_add - num_remove == hunk_size_after.

So your state machine can be quite dump: You go into a "hunk state" once you reach ^@@ and then read lines until both equations above hold, at which point you have reached the end of the hunk. Then you can do the highlighting of that hunk, output it, leave "hunk state" and search for the next hunk.

For robustness, you should probably also stop when you find a line that does not match ^[ +-], independently of how many lines you already read (reason unknown, but you should not care why).

dandavison added a commit that referenced this issue Nov 26, 2019
dandavison added a commit that referenced this issue Nov 26, 2019
dandavison added a commit that referenced this issue Nov 26, 2019
dandavison added a commit that referenced this issue Nov 26, 2019
dandavison added a commit that referenced this issue Nov 26, 2019
@dandavison dandavison added the bug Something isn't working label Dec 4, 2019
@fdcds
Copy link
Author

fdcds commented Dec 16, 2019

Somehow this seems to not have worked. With git config --global diff.submodule log git diff outputs:

[...]
Submodule a/b/c a12bc..de34f:
  > Commit 2 Message
  > Commit 1 Message
[...]

With git config --global core.pager "delta --dark" I get following output:

[...]
Submodule a/b/c a12bc..de34f:
----
[...]

I.e. the changes to the commits are not displayed any longer.

It thus appears that delta still mangles lines outside the changed files, in this case also the indented changes to the submodule's commits, which is in "normal diff" format (>/<).

@dandavison dandavison reopened this Dec 17, 2019
@dandavison
Copy link
Owner

Hi @fdcds Sorry this second bug has taken so long to fix. I can reproduce it. I notice that a workaround would be to use --file-style plain or --color-only, but I agree another fix to the parsing logic is still needed here.

@dandavison dandavison changed the title Improve hunk size detection and do not mangle lines outside a hunk Handle diff.submodule log Apr 30, 2020
@dandavison dandavison changed the title Handle diff.submodule log 🐛Handle diff.submodule log Apr 30, 2020
@dandavison
Copy link
Owner

Looks like this has been fixed in the intervening 5 years; I haven't bisected to find out where yet. Thanks very much @fdcds. A PR doing something in the direction of your very nice suggestion was just merged (#1787) which brought this issue to mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants