[RFC PATCH 0/5] Fix some fast-import parsing issues

[Elijah Newren <newren@gmail.com>]

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@organization.tld> 1000000000 +0100
    committer Full Name <user@organization.tld> 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@organization.tld> 1000000000 +0100
      committer Full Name <user@organization.tld> 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