diff: allow headers with spaces in filename#5030
Conversation
|
/rebuild |
|
Okay, @tiennou, I started to rebuild this pull request as build #1703. |
Parsing diff headers with spaces in filenames caused libgit2
to crash because it assumed the new path always starts at
a SPACE character.
diff --git a/sp ace.txt b/sp ace.txt
header_path_len now looks ahead for the sequence "?b/ to
correctly determine the end of the file path.
|
On the topic of actually parsing those patches, I'm not sure if that's in scope… @jrn, since |
pks-t
left a comment
There was a problem hiding this comment.
$ git init repo
$ cd repo
$ touch "file ab.txt"
$ git add "file ab.txt"
$ git commit -mfile
$ mkdir "file b"
$ git mv "file ab.txt" "file b/"
$ git diff --cached
diff --git a/file ab.txt b/file b/file ab.txt
similarity index 100%
rename from file ab.txt
rename to file b/file ab.txt
As you can see above, it's plain impossible to parse. That's why I've implemented the bailing out logic in 80226b5 (patch_parse: allow parsing ambiguous patch headers, 2017-09-22), and so I don't think we should merge your fix. While it might improve the situation in some cases (probably most), it does open the gate for us to do the wrong thing. Instead we should continue using the bail-out logic that falls back to the "+++" and "---" lines to extract the paths. We do need a fix for your error case, though.
But in fact, I'd argue that it's quite a bit simpler. Your error happens in case where there's a file with space deleted or added. We aren't able to extract the filename prefixes in that case, but I'd argue we don't need one anyway in that case. "/dev/null" doesn't have a prefix and the header is unparseable. Instead, we should just drop back to the default prefix.
I've implemented a separate fix that does what I propose. I've retained your test including your authorship in that PR (which I'll create in a minute).
|
See #5035 |
Parsing diff headers with spaces in filenames caused libgit2
to crash because it assumed the new path always starts at
a SPACE character.
header_path_len now looks ahead for the sequence "?b/ to
correctly determine the end of the file path.
Closes #5029