I found a few issues with parsing in fast-import (dating back to
git-2.6.0). I was thrown off track for a while by the fact that the
.git/fast_import_crash_$PID file erroneously claimed that the
penultimate command it read was somehow truncated; e.g. for input of
the form:
commit refs/heads/master
mark :1
author Full Name <user@xxxxxxxxxxxxxxxx> 1000000000 +0100
committer Full Name <user@xxxxxxxxxxxxxxxx> 1000000000 +0100
data 53
Commit message for intentionally empty orphan commit
get-mark :1
the crash file was reporting
Most Recent Commands Before Crash
---------------------------------
commit refs/heads/master
mark :1
author Full Name <user@xxxxxxxxxxxxxxxx> 1000000000 +0100
committer Full Name <user@xxxxxxxxxxxxxxxx> 1000000000 +0100
data 53
* get-mark :1
In other words, it dropped the commit message line entirely, which
made me wonder if I was having weird buffering or flushing issues
until I was able to narrow it down to the simple testcase above and
could duplicate with
cat seven-line-test-file | git fast-import
I don't know if there are other ways to get fast-import to give a
corrupted "Most Recent Commands Before Crash" report, but this one
doesn't trigger after my other fixes below.
Switching gears from the bad reporting to the original parsing bug
though, it is clear fast-import somehow mishandled get-mark directives
after empty orphan commits. Digging into it, I think it was caused by
following a precedent set by a somewhat unsafe implementation of
trying to allow cat-blob directives to appear in the middle of a
commit. My spelunking showed the following commits seemed to be the
most relevant as to how we got to the current state:
777f80d7429b ("fast-import: Allow cat-blob requests at arbitrary
points in stream", 2010-11-28)
8dc6a373d201 ("fast-import: add 'ls' command", 2010-12-02)
97313bef2a16 ("fast-import: use skip_prefix for parsing input",
2014-06-18)
28c7b1f7b7b7 ("fast-import: add a get-mark command", 2015-07-01)
I've cc'ed the relevant folks, and have a few patches that fix the
issue and I think make the parser more robust against future issues in
a way that I think is safe enough for backward compatibility, but
"backward compatible enough" might concern some folks; if so, please
take a look at patches 4 and 5.
Elijah Newren (5):
t9300: demonstrate bug with get-mark and empty orphan commits
git-fast-import.txt: fix wording about where ls command can appear
fast-import: check most prominent commands first
fast-import: only allow cat-blob requests where it makes sense
fast-import: fix erroneous handling of get-mark with empty orphan
commits
Documentation/git-fast-import.txt | 22 +++++++++---------
fast-import.c | 31 ++++++++++++++------------
t/t9300-fast-import.sh | 37 +++++++++++++++++++++++++++++++
3 files changed, 66 insertions(+), 24 deletions(-)
--
2.21.0.rc2.5.g8f70af2367
