Re: [StGit PATCH] Parse commit object header correctly

[Michael Haggerty <mhagger@alum.mit.edu>]

On 02/08/2012 08:33 AM, Junio C Hamano wrote:
> To allow parsing the header produced by versions of Git newer than the
> code written to parse it, all commit parsers are expected to skip unknown
> header lines, so that newer types of header lines can be added safely.
> The only three things that are promised are:
> 
>  (1) the header ends with an empty line (just an LF, not "a blank line"),
>  (2) unknown lines can be skipped, and
>  (3) a header "field" begins with the field name, followed by a single SP
>      followed by the value.
> 
> The parser used by StGit, introduced by commit cbe4567 (New StGit core
> infrastructure: repository operations, 2007-12-19), was accidentally a bit
> too loose to lose information, and a bit too strict to raise exception
> when dealing with a line it does not understand.
> 
>  - It used "strip()" to lose whitespaces from both ends, risking a line
>    with only whitespaces to be mistaken as the end of the header.
> 
>  - It used "k, v = line.split(None, 1)", blindly assuming that all header
>    lines (including the ones that the version of StGit may not understand)
>    can safely be split without raising an exception, which is not true if
>    there is no SP on the line.
> 
> This patch changes the parsing logic so that it:
> 
>  (1) detects end of the hedaer correctly by treating only an empty line as
>      such;
>  (2) handles multi-line fields (a header line that begins with a single SP
>      is appended to the previous line after removing that leading SP but
>      retaining the LF between the line and the previous line) correctly;
>  (3) splits a line at the first SP to find the field name, but only does
>      so when there actually is SP on the line; and
>  (4) ignores lines that cannot be understood without barfing.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  * Earlier I sent a minimum parser fix that ignores multi-line fields, as
>    the fields StGit cares about are all single line.  This patch also
>    teaches multi-line fields to the parser, so that later versions of
>    StGit can parse and use them if they choose to.
> 
>    Python is not my primary language, so please take this with a grain of
>    salt.
> 
>    Thanks.
> 
>  stgit/lib/git.py |   41 +++++++++++++++++++++++++++--------------
>  1 file changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/stgit/lib/git.py b/stgit/lib/git.py
> index 56287f6..f19371b 100644
> --- a/stgit/lib/git.py
> +++ b/stgit/lib/git.py
> @@ -390,21 +390,34 @@ class CommitData(Immutable, Repr):
>          @return: A new L{CommitData} object
>          @rtype: L{CommitData}"""
>          cd = cls(parents = [])
> -        lines = list(s.splitlines(True))
> +        raw_lines = list(s.splitlines(True))

str.splitlines() splits lines at any EOL pattern ('\n', '\r\n' or '\r'
alone).  If you want to be sure to split only on '\n', I think the
simplest alternative is

    raw_lines = s.split('\n')

str.split() and str.splitlines() already return lists, so it is not
necessary to wrap the result in list().

But please note that str.split() discards the split characters, and if
the last character in the string is '\n' then the last string in the
result list is the empty string.

> +        lines = []
> +        # Collapse multi-line header lines
> +        for i in xrange(len(raw_lines)):
> +            line = raw_lines[i]

The two previous lines can be written

           for (i, line) in enumerate(raw_lines):

> +            if line == '\n':
> +                cd.set_message(''.join(raw_lines[i+1:]))
> +                break
> +            if line[0] == ' ':
> +                # continuation line
> +                lines[-1] += '\n' + line[1:]

In your original version, lines[-1] would already be LF-terminated, so
this line would create a double-LF in the string.

> +            else:
> +                lines.append(line)
>          for i in xrange(len(lines)):
> -            line = lines[i].strip()
> -            if not line:
> -                return cd.set_message(''.join(lines[i+1:]))
> -            key, value = line.split(None, 1)
> -            if key == 'tree':
> -                cd = cd.set_tree(repository.get_tree(value))
> -            elif key == 'parent':
> -                cd = cd.add_parent(repository.get_commit(value))
> -            elif key == 'author':
> -                cd = cd.set_author(Person.parse(value))
> -            elif key == 'committer':
> -                cd = cd.set_committer(Person.parse(value))
> -        assert False
> +            line = lines[i].rstrip('\n')
> +            ix = line.find(' ')
> +            if 0 <= ix:
> +                key, value = line[0:ix], line[ix+1:]

The above five lines can be written

           for line in lines:
               if ' ' in line:
                   key, value = line.rstrip('\n').split(' ', 1)

or (if the lack of a space should be treated more like an exception)

           for line in lines:
               try:
                   key, value = line.rstrip('\n').split(' ', 1)
               except ValueError:
                   continue

> +                if key == 'tree':
> +                    cd = cd.set_tree(repository.get_tree(value))
> +                elif key == 'parent':
> +                    cd = cd.add_parent(repository.get_commit(value))
> +                elif key == 'author':
> +                    cd = cd.set_author(Person.parse(value))
> +                elif key == 'committer':
> +                    cd = cd.set_committer(Person.parse(value))

All in all, I would recommend something like (untested):

        @return: A new L{CommitData} object
        @rtype: L{CommitData}"""
        cd = cls(parents = [])
        lines = []
        raw_lines = s.split('\n')
        # Collapse multi-line header lines
        for i, line in enumerate(raw_lines):
            if not line:
                cd.set_message('\n'.join(raw_lines[i+1:]))
                break
            if line.startswith(' '):
                # continuation line
                lines[-1] += '\n' + line[1:]
            else:
                lines.append(line)

        for line in lines:
            if ' ' in line:
                key, value = line.split(' ', 1)
                if key == 'tree':
                    cd = cd.set_tree(repository.get_tree(value))
                elif key == 'parent':
                    cd = cd.add_parent(repository.get_commit(value))
                elif key == 'author':
                    cd = cd.set_author(Person.parse(value))
                elif key == 'committer':
                    cd = cd.set_committer(Person.parse(value))
        return cd

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/