diffcore-rename with duplicate tree entries can segfault
Get series with b4
b4 am [email protected]Versions newest first
- v1 Latest2 patches 1 replyIn review
Patches in v1 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/2 | diffcore-rename: split locate_rename_dst into two functionsdiffcore-rename.c | +26 | 0 replies | |
| 2/2 | diffcore-rename: avoid processing duplicate destinationsdiffcore-rename.c t/ | +85 | 1 reply |
Reviews trailers per patch
No Reviewed-by, Acked-by or Tested-by trailers for v1 yet.
Cover letter [PATCH 0/2] diffcore-rename with duplicate tree entries can segfault
Quoted 6 lines · Junio C Hamano
Quoted 6 lines
So to go forward, I'm happy to prepare a patch, but I'd like to know:
1. Does something like the above look reasonable to you (I'd probably refactor it to avoid the bizarre return value semantics from
locate_rename_dst, though)?
Quoted 8 lines
Quoted 4 lines
2. If so, do you want something minimal like what's above, or do you mind if I build it on top of a hashmap conversion? I suspect the logic may also end up more clear with the hashmap (since inserting versus lookup will be more distinct in the callers).
No, I don't mind. The diff-b-m topic seems to need a lot deeper rethink than I originally anticipated anyway, and it can wait for a clean-up to use hashmap to stabilize.
I tried switching to a hashmap, but the diff_score code actually wants to index into the array using an int. In a hashmap, we'd use a pointer instead, but that increases the size of "struct diff_score", which is something that we have to allocate a lot of (src * dst, I think).
So I punted on that and just cleaned up the locate_rename_dst interface a bit. Here's the result.
[1/2]: diffcore-rename: split locate_rename_dst into two functions [2/2]: diffcore-rename: avoid processing duplicate destinations
-Peff