Re: [StGit PATCH] Parse commit object header correctly
On 02/08/2012 08:33 AM, Junio C Hamano wrote:
Show 60 quoted lines
> 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):
Show 6 quoted 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.
Show 20 quoted lines
> + 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:
continueShow 8 quoted lines
> + 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 cdMichael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
Andy Green (林安廸)
