[StGit PATCH] Parse commit object header correctly

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 <redacted>
---

 * 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))
+        lines = []
+        # Collapse multi-line header lines
+        for i in xrange(len(raw_lines)):
+            line = raw_lines[i]
+            if line == '\n':
+                cd.set_message(''.join(raw_lines[i+1:]))
+                break
+            if line[0] == ' ':
+                # continuation line
+                lines[-1] += '\n' + line[1:]
+            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:]
+                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
+
 
 class Commit(GitObject):
     """Represents a git commit object. All the actual data contents of the

On 02/08/2012 08:33 AM, Junio C Hamano wrote:

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/

On Wed, Feb 8, 2012 at 11:00 AM, Michael Haggerty [off-list ref] wrote:

On 02/08/2012 08:33 AM, Junio C Hamano wrote:

quoted

 (1) detects end of the hedaer correctly by treating only an empty line as
     such;

s/hedaer/header/;

quoted

+            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

This is generally considered more pythonic: "It's easier to ask for
forgiveness than to get permission".

quoted

+                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

One could also take the recommended python approach for
switch/case-like if/elif/else statements:

updater = { 'tree': lambda cd, value: cd.set_tree(repository.get_tree(value),
                 'parent': lambda cd, value:
cd.add_parent(repository.get_commit(value)),
                 'author': lambda cd, value: cd.set_author(Person.parse(value)),
                 'committer': lambda cd, value:
cd.set_committer(Person.parse(value))
              }
for line in lines:
    try:
        key, value = line.split(' ', 1)
        cd = updater[key](cd, value)
    except ValueError:
        continue
    except KeyError:
        continue

It documents about the same, but adds checking on double 'case'
statements. The resulting for loop is rather cleaner and the exception
approach becomes even more logical. I rather like the result, but I
guess it's mostly a matter of taste.

Cheers,
Frans

On 02/08/2012 11:43 AM, Frans Klaver wrote:

On Wed, Feb 8, 2012 at 11:00 AM, Michael Haggerty [off-list ref] wrote:

quoted

On 02/08/2012 08:33 AM, Junio C Hamano wrote:

quoted

+            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

This is generally considered more pythonic: "It's easier to ask for
forgiveness than to get permission".

Given that Junio explicitly wanted to allow lines with no spaces, I
assume that lack of a space is not an error but rather a conceivable
future extension.  If my assumption is correct, then it is misleading
(and inefficient) to handle it via an exception.

quoted

quoted

+                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

One could also take the recommended python approach for
switch/case-like if/elif/else statements:

updater = { 'tree': lambda cd, value: cd.set_tree(repository.get_tree(value),
                 'parent': lambda cd, value:
cd.add_parent(repository.get_commit(value)),
                 'author': lambda cd, value: cd.set_author(Person.parse(value)),
                 'committer': lambda cd, value:
cd.set_committer(Person.parse(value))
              }
for line in lines:
    try:
        key, value = line.split(' ', 1)
        cd = updater[key](cd, value)
    except ValueError:
        continue
    except KeyError:
        continue

It documents about the same, but adds checking on double 'case'
statements. The resulting for loop is rather cleaner and the exception
approach becomes even more logical. I rather like the result, but I
guess it's mostly a matter of taste.

I know this approach and use it frequently, but when one has to resort
to lambdas and there are only four cases, it becomes IMHO less readable
than the if..else version.

Michael

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

On Wed, 08 Feb 2012 17:17:24 +0100, Michael Haggerty  
[off-list ref] wrote:

On 02/08/2012 11:43 AM, Frans Klaver wrote:

quoted

On Wed, Feb 8, 2012 at 11:00 AM, Michael Haggerty  
[off-list ref] wrote:

quoted

On 02/08/2012 08:33 AM, Junio C Hamano wrote:

quoted

+            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

This is generally considered more pythonic: "It's easier to ask for
forgiveness than to get permission".

Given that Junio explicitly wanted to allow lines with no spaces, I
assume that lack of a space is not an error but rather a conceivable
future extension.  If my assumption is correct, then it is misleading
(and inefficient) to handle it via an exception.

I find the documenting more convincing than the efficiency, but from the  
phrasing I think you do too.

quoted

quoted

       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

One could also take the recommended python approach for
switch/case-like if/elif/else statements:

updater = { 'tree': lambda cd, value:  
cd.set_tree(repository.get_tree(value),
                 'parent': lambda cd, value:
cd.add_parent(repository.get_commit(value)),
                 'author': lambda cd, value:  
cd.set_author(Person.parse(value)),
                 'committer': lambda cd, value:
cd.set_committer(Person.parse(value))
              }
for line in lines:
    try:
        key, value = line.split(' ', 1)
        cd = updater[key](cd, value)
    except ValueError:
        continue
    except KeyError:
        continue

It documents about the same, but adds checking on double 'case'
statements. The resulting for loop is rather cleaner and the exception
approach becomes even more logical. I rather like the result, but I
guess it's mostly a matter of taste.

I know this approach and use it frequently, but when one has to resort
to lambdas and there are only four cases, it becomes IMHO less readable
than the if..else version.

Well, as I said, its largely a matter of taste; four items is a corner  
case to me when thinking maintainability vs. readability. On the other  
hand, this doesn't seem like an oft-changing piece of code, so a longer  
list of if..elif..else shouldn't be a problem either.

Frans

Hi Junio,

On 8 February 2012 07:33, Junio C Hamano [off-list ref] 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.

Thanks for looking into this. Is this the same as an email header? If
yes, we could just use the python's email.Header.decode_header()
function (I haven't tried yet).

BTW, does Git allow custom headers to be inserted by tools like StGit?

-- 
Catalin

Hi Catalin,

Catalin Marinas wrote:

Thanks for looking into this. Is this the same as an email header? If
yes, we could just use the python's email.Header.decode_header()
function (I haven't tried yet).

They look like this:

	encoding ISO8859-1

BTW, does Git allow custom headers to be inserted by tools like StGit?

No.  There is one list of supported headers, and this list is the
standards body that maintains it[*].  So if you end up needing an
extension to the commit object format, that can be done, but it needs
to be accepted here (and ideally checked by "git fsck", though it's
lagging a bit in that respect lately).

By the way, headers have a standard order to avoid spurious changes in
commit names from reordering.  Additions so far have always happened
at the end, which is what makes checks by "git fsck" possible --- it
can't rule out an unrecognized header line being a standard field from
a future version of git, but it would be allowed to complain about
unrecognized fields before 'encoding', for example.

Thanks,
Jonathan

[*] http://thread.gmane.org/gmane.comp.version-control.git/138848/focus=138892

On Fri, Feb 10, 2012 at 12:51 AM, Jonathan Nieder [off-list ref] wrote:

No.  There is one list of supported headers, and this list is the
standards body that maintains it[*].  So if you end up needing an
extension to the commit object format, that can be done, but it needs
to be accepted here (and ideally checked by "git fsck", though it's
lagging a bit in that respect lately).

[*] http://thread.gmane.org/gmane.comp.version-control.git/138848/focus=138892

Doesn't this deserve a document in Documentation/technical? It's also
a good opportunity to document tree and tag object format in addition
to commit object. I did not check thoroughly but the commit that
introduced encoding field, 4b2bced, did not come with any document
updates, so I assume it has not been documented ever since.
-- 
Duy

On 8 February 2012 10:00, Michael Haggerty [off-list ref] wrote:

On 02/08/2012 08:33 AM, Junio C Hamano wrote:

quoted

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.

...

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

Thank you all for comments and patches. I used a combination of
Junio's patch with the comments from Michael and a fix from me. I'll
publish it to the 'master' branch shortly and release a 0.16.1
hopefully this week.

-- 
Catalin

On 02/15/2012 04:24 AM, Somebody in the thread at some point said:

Hi -

Thank you all for comments and patches. I used a combination of
Junio's patch with the comments from Michael and a fix from me. I'll
publish it to the 'master' branch shortly and release a 0.16.1
hopefully this week.

I cloned the master branch and installed it locally, it's working well.

Thanks a lot to the guys who spent time on this bug and stgit overall,
which I rely heavily on!

-Andy