crash on git diff

[classabbyamp]

when doing git diff during a rebase with unmerged conflicts, I was able to crash riff. It reproduces outside of git if copying the offending diff to a file and rendering that.

panic message:

-v-v-v----------- RIFF CRASHED ---------------v-v-v-

Panic message: <PanicInfo {
    payload: Any { .. },
    message: Some(
        assertion failed: line.starts_with('\\'),
    ),
    location: Location {
        file: "src/hunk_highlighter.rs",
        line: 158,
        col: 9,
    },
    can_unwind: true,
    force_no_backtrace: false,
}>

   0: <unknown>
   1: <unknown>
   2: <unknown>
   3: <unknown>
   4: <unknown>
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: <unknown>
  14: <unknown>
  15: <unknown>
  16: <unknown>
  17: __libc_start_call_main
             at ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
  18: __libc_start_main_impl
             at ./csu/../csu/libc-start.c:360:3
  19: <unknown>

Riff version: 3.1.0

Command line arguments: Args { inner: ["riff", "-f", "/tmp/bad.diff"] }

-^-^-^------- END OF RIFF CRASH REPORT -------^-^-^-


Please copy the above crash report and report it at one of:
* <https://github.com/walles/riff/issues> (preferred)
* <johan.walles@gmail.com>

ERROR at end of input: Still expecting more lines, but got EOF: [0, 3, 0]

diff that caused the crash:

diff --cc ansible/roles/zfsbootmenu/tasks/main.yaml
index 73fa864,08648fa..0000000
--- a/ansible/roles/zfsbootmenu/tasks/main.yaml
+++ b/ansible/roles/zfsbootmenu/tasks/main.yaml
@@@ -6,6 -6,60 +6,60 @@@
        - zfsbootmenu
      state: present
  
++<<<<<<< HEAD
++=======
+ - name: Create ESP mdadm config
+   template:
+     src: mdadm.conf.j2
+     dest: /etc/mdadm.conf
+     owner: root
+     group: root
+     mode: 0644
+   when: esp_mdadm_array is defined
+ 
+ - name: Install dropbear
+   xbps:
+     pkg:
+       - dropbear
+       - mkinitcpio-dropbear
+     state: present
+ 
+ - name: Create dropbear directory
+   file:
+     path: /etc/dropbear
+     owner: root
+     group: root
+     mode: 0755
+     state: directory
+ 
+ - name: Create dropbear keys
+   command:
+     cmd: /usr/bin/dropbearkey -t {{ item }} -f /etc/dropbear/dropbear_{{ item }}_host_key
+     creates: /etc/dropbear/dropbear_{{ item }}_host_key
+   loop:
+     - rsa
+     - ecdsa
+     - ed25519
+ 
+ - name: Create dropbear authorised keys
+   template:
+     src: authorized_keys.j2
+     dest: /etc/dropbear/root_key
+     owner: root
+     group: root
+     mode: 0644
+     lstrip_blocks: true
+ 
+ - name: Create dropbear config
+   template:
+     src: dropbear.conf.j2
+     dest: /etc/dropbear/dropbear.conf
+     owner: root
+     group: root
+     mode: 0644
+     lstrip_blocks: true
+ 
++>>>>>>> 8eec118 (fixup! ansible/roles/zfsbootmenu: add role)
  - name: Create zfsbootmenu config
    template:
      src: config.yaml.j2
* Unmerged path ansible/host_vars/foo.yaml

[classabbyamp]

removing the * Unmerged path ansible/host_vars/foo.yaml line removes the panic, but leaves the ERROR at end of input: Still expecting more lines, but got EOF: [0, 3, 0]

[walles]

Confirmed, thanks for the clear repro case!

[walles]

I want to provide an update.

I tried to repro this by creating a delete-edit conflict and rebasing, result below.

But riff handles this output just fine.

Did you edit the repro example in any way?

riff parses the hunk headers (@@@ -6,6 -6,60 +6,60 @@@) and expects the input to match, but if this counting is off then riff will handle it badly.

No matter whether the counts are off due to bugs in riff or due to the input being malformed in some way.

My primary focus right now is for riff to provide better diagnostics when the counts are off so that I can understand what exactly riff thinks is lacking in your diff.

diff --cc README.md
index 3a1fc52,5245317..0000000
--- README.md
+++ README.md
@@@ -1,2 -1,1 +1,6 @@@
++<<<<<<< HEAD
 +initial
 +main
++=======
+ Branch
++>>>>>>> 28d4562 (Branch commit)
* Unmerged path pain.txt

[classabbyamp]

Did you edit the repro example in any way?

the only thing i changed was the actual filename to foo.yaml, which shouldn't affect it at all (and i could still repro with the foo)

[walles]

Is there any way for me to repro this git output myself?

Instructions like "Clone this repo, add this remote, run these commands"?

It's more likely that me / riff is wrong than Git, but I can't understand this diff.

Being able to experiment with git here would be super helpful!

Also, what does git --version say?

Analysis

First, @@@ -6,6 -6,60 +6,60 @@@ means that there are two initial versions (I'll call them v1 and v2) merged into one destination version. These versions contain 6, 60 and 60 lines respectively. Documented here.

✅ v1 consists of the six context lines starting with , three at the top and three at the bottom. This matches the number in the header.

Then, a + in column 1 means that a line is new versus v1. A + in column 2 means that a line is new versus v2. A space in either column means that the line was already there in this version.

So my understanding is that v2 should consist of:

• The six context lines, same as in v1
• All lines starting with + (a plus and a space)

❌ This adds up to 57 lines. But the header says 60.

Just by counts, the difference here matches the number of ++ lines, but the + in the second column there supposedly means the line is new relative to v2, and thus not part of the v2 line counts.

And what confuses me even more is that I have verified this logic with other diffs, also from git, and it all adds up there.

[classabbyamp]

unfortunately that repo is private, and that was from the middle of a rebase where some entangled commits were reordered, so I don't think I'll be able to easily recreate a reproducing repo state for you

$ git --version
git version 2.44.0

[walles]

Since I don't understand it and I can't reproduce it, I would like to at least have riff handle this better.

I'm thinking that rather than crashing, if riff doesn't understand some parts of its input it should:

• Render those parts in yellow
• Print warning messages after paging is done with what it had problems with

Actually tracking this down and understanding it would be better obviously, but if I can't then I'll have to go for the next best thing...

[classabbyamp]

I'll try to remember to tar up the repo the next time it happens

[walles]

Would you be OK with me sending your diff to the git mailing list git@vger.kernel.org?

You said the repo is private so I wanted to ask you first.

[classabbyamp]

yes, that's fine. nothing in the diff is private (i made sure of that already), but I can't say the same about the rest of the repo :)

[walles]

Asked here: https://lore.kernel.org/git/CAKtyTbevS7W-X4qoO=uPnHmXCEtKhqycD8DZZY-oK-z0p4CEAQ@mail.gmail.com/T/#u

[walles]

Junio C Hamano confirms Git seems to be in the wrong here:

So, yes, I think you are counting correctly.

Sorry, but I do not know what's counting the hunk length numbers in
today's code offhand, so I won't be digging it further for now.

But since upstream won't look into it and I can't repro it the new error handling that I just merged will count as the fix for this.