{"thread":{"id":"29570","subject":"STGIT: Deathpatch in linus tree","startedAt":"2012-02-07T13:02:12Z","lastAt":"2012-02-15T18:40:44Z","messageCount":15,"participants":["Andy Green (林安廸)","Junio C Hamano","Michael Haggerty","Frans Klaver","Catalin Marinas","Jonathan Nieder","Nguyen Thai Ngoc Duy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"184129","messageId":"4F3120D4.1050604@warmcat.com","threadId":"29570","inReplyTo":null,"subject":"STGIT: Deathpatch in linus tree","fromName":"Andy Green (林安廸)","fromEmail":"andy@warmcat.com","sentAt":"2012-02-07T13:02:12Z","receivedAt":"2012-02-07T13:02:12Z","isPatch":false,"sender":{"key":"andy@warmcat.com","avatar":"https://gravatar.com/avatar/cc8d9622972f22b99adb7d46cad8ed719cc7088eafbdef72639b74bce045ae49?d=mp&s=160"},"body":"Hi -\n\nLinus just pushed 105e5180936d69b1aee46ead8a5fc6c68f4d5f65 to\nlinux-2.6... along the lines of the Monty Python joke that was so funny\nit kills anyone who hears it, if I have a stgit branch based on a HEAD\nthat includes this commit then stgit dies when pushing on top of it.\n\nSo...\n\n[agreen@build linux-2.6]$ stg pop --all\n[agreen@build linux-2.6]$ git reset --hard 96e02d1\nHEAD is now at 96e02d1 exec: fix use-after-free bug in setup_new_exec()\n[agreen@build linux-2.6]$ stg push\nPushing patch \"subject-patch-1-3-arm-dt-add-p\" ... done (empty)\nNow at patch \"subject-patch-1-3-arm-dt-add-p\"\n[agreen@build linux-2.6]$ stg pop\nPopped subject-patch-1-3-arm-dt-add-p\nNo patch applied\n\nSo the commit just before the bad guy is happy. Then -->\n\n[agreen@build linux-2.6]$ git reset --hard 105e518\nHEAD is now at 105e518 Merge tag 'hwmon-fixes-for-3.3-rc3' of\ngit://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging\n[agreen@build linux-2.6]$ stg push\nError: Unhandled exception:\nTraceback (most recent call last):\n File \"/usr/lib/python2.7/site-packages/stgit/main.py\", line 152, in _main\n ret = command.func(parser, options, args)\n File \"/usr/lib/python2.7/site-packages/stgit/commands/push.py\", line\n68, in func\n check_clean_iw = clean_iw)\n File \"/usr/lib/python2.7/site-packages/stgit/lib/transaction.py\", line\n95, in __init__\n self.__current_tree = self.__stack.head.data.tree\n File \"/usr/lib/python2.7/site-packages/stgit/lib/git.py\", line 426, in\ndata\n self.__repository.cat_object(self.sha1))\n File \"/usr/lib/python2.7/site-packages/stgit/lib/git.py\", line 408, in\nparse\n assert False\nAssertionError\n\nIt also dies if I use Linus' current HEAD as the basis I am stgitting on\ntop of, which is one patch ahead of the deathpatch.\n\nI'm using Fedora rawhide versions of git and stgit\n\ngit-1.7.9-1.fc17.x86_64\nstgit-0.15-2.fc17.noarch\n\n-Andy\n"},{"id":"184144","messageId":"7vvcni1r5u.fsf@alter.siamese.dyndns.org","threadId":"29570","inReplyTo":"4F3120D4.1050604@warmcat.com","subject":"Re: STGIT: Deathpatch in linus tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-07T17:37:01Z","receivedAt":"2012-02-07T17:37:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Andy Green (林安廸)\" writes:\n\n> [agreen@build linux-2.6]$ git reset --hard 105e518\n> HEAD is now at 105e518 Merge tag 'hwmon-fixes-for-3.3-rc3' of\n> git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging\n> [agreen@build linux-2.6]$ stg push\n> Error: Unhandled exception:\n> Traceback (most recent call last):\n> File \"/usr/lib/python2.7/site-packages/stgit/main.py\", line 152, in _main\n> ret = command.func(parser, options, args)\n> File \"/usr/lib/python2.7/site-packages/stgit/commands/push.py\", line\n> 68, in func\n> check_clean_iw = clean_iw)\n> File \"/usr/lib/python2.7/site-packages/stgit/lib/transaction.py\", line\n> 95, in __init__\n> self.__current_tree = self.__stack.head.data.tree\n> File \"/usr/lib/python2.7/site-packages/stgit/lib/git.py\", line 426, in\n> data\n> self.__repository.cat_object(self.sha1))\n> File \"/usr/lib/python2.7/site-packages/stgit/lib/git.py\", line 408, in\n> parse\n> assert False\n> AssertionError\n\nStgit at least at version 0.15 (which is you used, and which is what I\nchecked its source for) assumes it knows every possible kind of header\nthat can be recorded in the commit object and hits this assert when it\nseems something it does not understand.\n\nVersion 0.16 seems to have removed this assert, like this hunk on the file\nbetween 0.15 and 0.16:\n\n@@ -404,8 +404,6 @@ class CommitData(Immutable, Repr):\n cd = cd.set_author(Person.parse(value))\n elif key == 'committer':\n cd = cd.set_committer(Person.parse(value))\n- else:\n- assert False\n assert False\n\nThe above code in StGit that tries to parse commit object header is\nbroken, and I am surprised that nobody caught it back in late 2006 when\nthe optional encoding header was added to the commit object, which the\nabove does not understand.\n\nI am not sure if that removal of the extra assertion is enough, though.\nIt does not skip lines that it does not understand until the first blank\nline, i.e. the end of the header, as it should. Instead it has this\nbefore starting to inspect if a line in the header is what it knows about:\n\n line = lines[i].strip()\n if not line:\n return cd.set_message(''.join(lines[i+1:]))\n key, value = line.split(None, 1)\n\nI do not think this will work on a continuation line inside a header that\nhas only one token, and it will also ignore the leading whitespace that\nprotects the continuation line.\n\nI think the loop should at least changed line this, until StGit starts\ncaring about multi-line headers with continuation lines.\n\ndiff --git a/stgit/lib/git.py b/stgit/lib/git.py\nindex 56287f6..f2b284d 100644\n--- a/stgit/lib/git.py\n+++ b/stgit/lib/git.py\n@@ -392,18 +392,20 @@ class CommitData(Immutable, Repr):\n cd = cls(parents = [])\n lines = list(s.splitlines(True))\n for i in xrange(len(lines)):\n- line = lines[i].strip()\n+ line = lines[i]\n if not line:\n return cd.set_message(''.join(lines[i+1:]))\n- key, value = line.split(None, 1)\n- if key == 'tree':\n- cd = cd.set_tree(repository.get_tree(value))\n- elif key == 'parent':\n- cd = cd.add_parent(repository.get_commit(value))\n- elif key == 'author':\n- cd = cd.set_author(Person.parse(value))\n- elif key == 'committer':\n- cd = cd.set_committer(Person.parse(value))\n+\t ix = line.find(' ')\n+\t if 0 < ix:\n+\t\tkey, value = line[0:ix], line[ix+1:]\n+\t\tif key == 'tree':\n+\t\t cd = cd.set_tree(repository.get_tree(value))\n+\t\telif key == 'parent':\n+\t\t cd = cd.add_parent(repository.get_commit(value))\n+\t\telif key == 'author':\n+\t\t cd = cd.set_author(Person.parse(value))\n+\t\telif key == 'committer':\n+\t\t cd = cd.set_committer(Person.parse(value))\n assert False\n \n class Commit(GitObject):\n"},{"id":"184179","messageId":"7vd39pzsmq.fsf_-_@alter.siamese.dyndns.org","threadId":"29570","inReplyTo":"7vvcni1r5u.fsf@alter.siamese.dyndns.org","subject":"[StGit PATCH] Parse commit object header correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-08T07:33:33Z","receivedAt":"2012-02-08T07:33:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"To allow parsing the header produced by versions of Git newer than the\ncode written to parse it, all commit parsers are expected to skip unknown\nheader lines, so that newer types of header lines can be added safely.\nThe only three things that are promised are:\n\n (1) the header ends with an empty line (just an LF, not \"a blank line\"),\n (2) unknown lines can be skipped, and\n (3) a header \"field\" begins with the field name, followed by a single SP\n followed by the value.\n\nThe parser used by StGit, introduced by commit cbe4567 (New StGit core\ninfrastructure: repository operations, 2007-12-19), was accidentally a bit\ntoo loose to lose information, and a bit too strict to raise exception\nwhen dealing with a line it does not understand.\n\n - It used \"strip()\" to lose whitespaces from both ends, risking a line\n with only whitespaces to be mistaken as the end of the header.\n\n - It used \"k, v = line.split(None, 1)\", blindly assuming that all header\n lines (including the ones that the version of StGit may not understand)\n can safely be split without raising an exception, which is not true if\n there is no SP on the line.\n\nThis patch changes the parsing logic so that it:\n\n (1) detects end of the hedaer correctly by treating only an empty line as\n such;\n (2) handles multi-line fields (a header line that begins with a single SP\n is appended to the previous line after removing that leading SP but\n retaining the LF between the line and the previous line) correctly;\n (3) splits a line at the first SP to find the field name, but only does\n so when there actually is SP on the line; and\n (4) ignores lines that cannot be understood without barfing.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Earlier I sent a minimum parser fix that ignores multi-line fields, as\n the fields StGit cares about are all single line. This patch also\n teaches multi-line fields to the parser, so that later versions of\n StGit can parse and use them if they choose to.\n\n Python is not my primary language, so please take this with a grain of\n salt.\n\n Thanks.\n\n stgit/lib/git.py | 41 +++++++++++++++++++++++++++--------------\n 1 file changed, 27 insertions(+), 14 deletions(-)\n\ndiff --git a/stgit/lib/git.py b/stgit/lib/git.py\nindex 56287f6..f19371b 100644\n--- a/stgit/lib/git.py\n+++ b/stgit/lib/git.py\n@@ -390,21 +390,34 @@ class CommitData(Immutable, Repr):\n @return: A new L{CommitData} object\n @rtype: L{CommitData}\"\"\"\n cd = cls(parents = [])\n- lines = list(s.splitlines(True))\n+ raw_lines = list(s.splitlines(True))\n+ lines = []\n+ # Collapse multi-line header lines\n+ for i in xrange(len(raw_lines)):\n+ line = raw_lines[i]\n+ if line == '\\n':\n+ cd.set_message(''.join(raw_lines[i+1:]))\n+ break\n+ if line[0] == ' ':\n+ # continuation line\n+ lines[-1] += '\\n' + line[1:]\n+ else:\n+ lines.append(line)\n for i in xrange(len(lines)):\n- line = lines[i].strip()\n- if not line:\n- return cd.set_message(''.join(lines[i+1:]))\n- key, value = line.split(None, 1)\n- if key == 'tree':\n- cd = cd.set_tree(repository.get_tree(value))\n- elif key == 'parent':\n- cd = cd.add_parent(repository.get_commit(value))\n- elif key == 'author':\n- cd = cd.set_author(Person.parse(value))\n- elif key == 'committer':\n- cd = cd.set_committer(Person.parse(value))\n- assert False\n+ line = lines[i].rstrip('\\n')\n+ ix = line.find(' ')\n+ if 0 <= ix:\n+ key, value = line[0:ix], line[ix+1:]\n+ if key == 'tree':\n+ cd = cd.set_tree(repository.get_tree(value))\n+ elif key == 'parent':\n+ cd = cd.add_parent(repository.get_commit(value))\n+ elif key == 'author':\n+ cd = cd.set_author(Person.parse(value))\n+ elif key == 'committer':\n+ cd = cd.set_committer(Person.parse(value))\n+ return cd\n+\n \n class Commit(GitObject):\n \"\"\"Represents a git commit object. All the actual data contents of the\n"},{"id":"184181","messageId":"4F3247CA.1020904@alum.mit.edu","threadId":"29570","inReplyTo":"7vd39pzsmq.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [StGit PATCH] Parse commit object header correctly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-02-08T10:00:42Z","receivedAt":"2012-02-08T10:00:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/08/2012 08:33 AM, Junio C Hamano wrote:\n> To allow parsing the header produced by versions of Git newer than the\n> code written to parse it, all commit parsers are expected to skip unknown\n> header lines, so that newer types of header lines can be added safely.\n> The only three things that are promised are:\n> \n> (1) the header ends with an empty line (just an LF, not \"a blank line\"),\n> (2) unknown lines can be skipped, and\n> (3) a header \"field\" begins with the field name, followed by a single SP\n> followed by the value.\n> \n> The parser used by StGit, introduced by commit cbe4567 (New StGit core\n> infrastructure: repository operations, 2007-12-19), was accidentally a bit\n> too loose to lose information, and a bit too strict to raise exception\n> when dealing with a line it does not understand.\n> \n> - It used \"strip()\" to lose whitespaces from both ends, risking a line\n> with only whitespaces to be mistaken as the end of the header.\n> \n> - It used \"k, v = line.split(None, 1)\", blindly assuming that all header\n> lines (including the ones that the version of StGit may not understand)\n> can safely be split without raising an exception, which is not true if\n> there is no SP on the line.\n> \n> This patch changes the parsing logic so that it:\n> \n> (1) detects end of the hedaer correctly by treating only an empty line as\n> such;\n> (2) handles multi-line fields (a header line that begins with a single SP\n> is appended to the previous line after removing that leading SP but\n> retaining the LF between the line and the previous line) correctly;\n> (3) splits a line at the first SP to find the field name, but only does\n> so when there actually is SP on the line; and\n> (4) ignores lines that cannot be understood without barfing.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> \n> * Earlier I sent a minimum parser fix that ignores multi-line fields, as\n> the fields StGit cares about are all single line. This patch also\n> teaches multi-line fields to the parser, so that later versions of\n> StGit can parse and use them if they choose to.\n> \n> Python is not my primary language, so please take this with a grain of\n> salt.\n> \n> Thanks.\n> \n> stgit/lib/git.py | 41 +++++++++++++++++++++++++++--------------\n> 1 file changed, 27 insertions(+), 14 deletions(-)\n> \n> diff --git a/stgit/lib/git.py b/stgit/lib/git.py\n> index 56287f6..f19371b 100644\n> --- a/stgit/lib/git.py\n> +++ b/stgit/lib/git.py\n> @@ -390,21 +390,34 @@ class CommitData(Immutable, Repr):\n> @return: A new L{CommitData} object\n> @rtype: L{CommitData}\"\"\"\n> cd = cls(parents = [])\n> - lines = list(s.splitlines(True))\n> + raw_lines = list(s.splitlines(True))\n\nstr.splitlines() splits lines at any EOL pattern ('\\n', '\\r\\n' or '\\r'\nalone). If you want to be sure to split only on '\\n', I think the\nsimplest alternative is\n\n raw_lines = s.split('\\n')\n\nstr.split() and str.splitlines() already return lists, so it is not\nnecessary to wrap the result in list().\n\nBut please note that str.split() discards the split characters, and if\nthe last character in the string is '\\n' then the last string in the\nresult list is the empty string.\n\n> + lines = []\n> + # Collapse multi-line header lines\n> + for i in xrange(len(raw_lines)):\n> + line = raw_lines[i]\n\nThe two previous lines can be written\n\n for (i, line) in enumerate(raw_lines):\n\n> + if line == '\\n':\n> + cd.set_message(''.join(raw_lines[i+1:]))\n> + break\n> + if line[0] == ' ':\n> + # continuation line\n> + lines[-1] += '\\n' + line[1:]\n\nIn your original version, lines[-1] would already be LF-terminated, so\nthis line would create a double-LF in the string.\n\n> + else:\n> + lines.append(line)\n> for i in xrange(len(lines)):\n> - line = lines[i].strip()\n> - if not line:\n> - return cd.set_message(''.join(lines[i+1:]))\n> - key, value = line.split(None, 1)\n> - if key == 'tree':\n> - cd = cd.set_tree(repository.get_tree(value))\n> - elif key == 'parent':\n> - cd = cd.add_parent(repository.get_commit(value))\n> - elif key == 'author':\n> - cd = cd.set_author(Person.parse(value))\n> - elif key == 'committer':\n> - cd = cd.set_committer(Person.parse(value))\n> - assert False\n> + line = lines[i].rstrip('\\n')\n> + ix = line.find(' ')\n> + if 0 <= ix:\n> + key, value = line[0:ix], line[ix+1:]\n\nThe above five lines can be written\n\n for line in lines:\n if ' ' in line:\n key, value = line.rstrip('\\n').split(' ', 1)\n\nor (if the lack of a space should be treated more like an exception)\n\n for line in lines:\n try:\n key, value = line.rstrip('\\n').split(' ', 1)\n except ValueError:\n continue\n\n> + if key == 'tree':\n> + cd = cd.set_tree(repository.get_tree(value))\n> + elif key == 'parent':\n> + cd = cd.add_parent(repository.get_commit(value))\n> + elif key == 'author':\n> + cd = cd.set_author(Person.parse(value))\n> + elif key == 'committer':\n> + cd = cd.set_committer(Person.parse(value))\n\nAll in all, I would recommend something like (untested):\n\n @return: A new L{CommitData} object\n @rtype: L{CommitData}\"\"\"\n cd = cls(parents = [])\n lines = []\n raw_lines = s.split('\\n')\n # Collapse multi-line header lines\n for i, line in enumerate(raw_lines):\n if not line:\n cd.set_message('\\n'.join(raw_lines[i+1:]))\n break\n if line.startswith(' '):\n # continuation line\n lines[-1] += '\\n' + line[1:]\n else:\n lines.append(line)\n\n for line in lines:\n if ' ' in line:\n key, value = line.split(' ', 1)\n if key == 'tree':\n cd = cd.set_tree(repository.get_tree(value))\n elif key == 'parent':\n cd = cd.add_parent(repository.get_commit(value))\n elif key == 'author':\n cd = cd.set_author(Person.parse(value))\n elif key == 'committer':\n cd = cd.set_committer(Person.parse(value))\n return cd\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"184183","messageId":"CAH6sp9P=vNjLycgzoWzRbeEsW-kQ5e4HgGYf2jP1+u9rtWV4dg@mail.gmail.com","threadId":"29570","inReplyTo":"4F3247CA.1020904@alum.mit.edu","subject":"Re: [StGit PATCH] Parse commit object header correctly","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2012-02-08T10:43:59Z","receivedAt":"2012-02-08T10:43:59Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Wed, Feb 8, 2012 at 11:00 AM, Michael Haggerty wrote:\n> On 02/08/2012 08:33 AM, Junio C Hamano wrote:\n>\n>>  (1) detects end of the hedaer correctly by treating only an empty line as\n>>      such;\n\ns/hedaer/header/;\n\n\n>> +            line = lines[i].rstrip('\\n')\n>> +            ix = line.find(' ')\n>> +            if 0 <= ix:\n>> +                key, value = line[0:ix], line[ix+1:]\n>\n> The above five lines can be written\n>\n>           for line in lines:\n>               if ' ' in line:\n>                   key, value = line.rstrip('\\n').split(' ', 1)\n>\n> or (if the lack of a space should be treated more like an exception)\n>\n>           for line in lines:\n>               try:\n>                   key, value = line.rstrip('\\n').split(' ', 1)\n>               except ValueError:\n>                   continue\n\nThis is generally considered more pythonic: \"It's easier to ask for\nforgiveness than to get permission\".\n\n\n>\n>> +                if key == 'tree':\n>> +                    cd = cd.set_tree(repository.get_tree(value))\n>> +                elif key == 'parent':\n>> +                    cd = cd.add_parent(repository.get_commit(value))\n>> +                elif key == 'author':\n>> +                    cd = cd.set_author(Person.parse(value))\n>> +                elif key == 'committer':\n>> +                    cd = cd.set_committer(Person.parse(value))\n>\n> All in all, I would recommend something like (untested):\n>\n>        @return: A new L{CommitData} object\n>        @rtype: L{CommitData}\"\"\"\n>        cd = cls(parents = [])\n>        lines = []\n>        raw_lines = s.split('\\n')\n>        # Collapse multi-line header lines\n>        for i, line in enumerate(raw_lines):\n>            if not line:\n>                cd.set_message('\\n'.join(raw_lines[i+1:]))\n>                break\n>            if line.startswith(' '):\n>                # continuation line\n>                lines[-1] += '\\n' + line[1:]\n>            else:\n>                lines.append(line)\n>\n>        for line in lines:\n>            if ' ' in line:\n>                key, value = line.split(' ', 1)\n>                if key == 'tree':\n>                    cd = cd.set_tree(repository.get_tree(value))\n>                elif key == 'parent':\n>                    cd = cd.add_parent(repository.get_commit(value))\n>                elif key == 'author':\n>                    cd = cd.set_author(Person.parse(value))\n>                elif key == 'committer':\n>                    cd = cd.set_committer(Person.parse(value))\n>        return cd\n\nOne could also take the recommended python approach for\nswitch/case-like if/elif/else statements:\n\nupdater = { 'tree': lambda cd, value: cd.set_tree(repository.get_tree(value),\n 'parent': lambda cd, value:\ncd.add_parent(repository.get_commit(value)),\n 'author': lambda cd, value: cd.set_author(Person.parse(value)),\n 'committer': lambda cd, value:\ncd.set_committer(Person.parse(value))\n }\nfor line in lines:\n try:\n key, value = line.split(' ', 1)\n cd = updater[key](cd, value)\n except ValueError:\n continue\n except KeyError:\n continue\n\nIt documents about the same, but adds checking on double 'case'\nstatements. The resulting for loop is rather cleaner and the exception\napproach becomes even more logical. I rather like the result, but I\nguess it's mostly a matter of taste.\n\nCheers,\nFrans\n"},{"id":"184191","messageId":"4F32A014.1000608@alum.mit.edu","threadId":"29570","inReplyTo":"CAH6sp9P=vNjLycgzoWzRbeEsW-kQ5e4HgGYf2jP1+u9rtWV4dg@mail.gmail.com","subject":"Re: [StGit PATCH] Parse commit object header correctly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-02-08T16:17:24Z","receivedAt":"2012-02-08T16:17:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/08/2012 11:43 AM, Frans Klaver wrote:\n> On Wed, Feb 8, 2012 at 11:00 AM, Michael Haggerty wrote:\n>> On 02/08/2012 08:33 AM, Junio C Hamano wrote:\n>>> + line = lines[i].rstrip('\\n')\n>>> + ix = line.find(' ')\n>>> + if 0 <= ix:\n>>> + key, value = line[0:ix], line[ix+1:]\n>>\n>> The above five lines can be written\n>>\n>> for line in lines:\n>> if ' ' in line:\n>> key, value = line.rstrip('\\n').split(' ', 1)\n>>\n>> or (if the lack of a space should be treated more like an exception)\n>>\n>> for line in lines:\n>> try:\n>> key, value = line.rstrip('\\n').split(' ', 1)\n>> except ValueError:\n>> continue\n> \n> This is generally considered more pythonic: \"It's easier to ask for\n> forgiveness than to get permission\".\n\nGiven that Junio explicitly wanted to allow lines with no spaces, I\nassume that lack of a space is not an error but rather a conceivable\nfuture extension. If my assumption is correct, then it is misleading\n(and inefficient) to handle it via an exception.\n\n>>> + if key == 'tree':\n>>> + cd = cd.set_tree(repository.get_tree(value))\n>>> + elif key == 'parent':\n>>> + cd = cd.add_parent(repository.get_commit(value))\n>>> + elif key == 'author':\n>>> + cd = cd.set_author(Person.parse(value))\n>>> + elif key == 'committer':\n>>> + cd = cd.set_committer(Person.parse(value))\n>>\n>> All in all, I would recommend something like (untested):\n>>\n>> @return: A new L{CommitData} object\n>> @rtype: L{CommitData}\"\"\"\n>> cd = cls(parents = [])\n>> lines = []\n>> raw_lines = s.split('\\n')\n>> # Collapse multi-line header lines\n>> for i, line in enumerate(raw_lines):\n>> if not line:\n>> cd.set_message('\\n'.join(raw_lines[i+1:]))\n>> break\n>> if line.startswith(' '):\n>> # continuation line\n>> lines[-1] += '\\n' + line[1:]\n>> else:\n>> lines.append(line)\n>>\n>> for line in lines:\n>> if ' ' in line:\n>> key, value = line.split(' ', 1)\n>> if key == 'tree':\n>> cd = cd.set_tree(repository.get_tree(value))\n>> elif key == 'parent':\n>> cd = cd.add_parent(repository.get_commit(value))\n>> elif key == 'author':\n>> cd = cd.set_author(Person.parse(value))\n>> elif key == 'committer':\n>> cd = cd.set_committer(Person.parse(value))\n>> return cd\n> \n> One could also take the recommended python approach for\n> switch/case-like if/elif/else statements:\n>\n> updater = { 'tree': lambda cd, value: cd.set_tree(repository.get_tree(value),\n> 'parent': lambda cd, value:\n> cd.add_parent(repository.get_commit(value)),\n> 'author': lambda cd, value: cd.set_author(Person.parse(value)),\n> 'committer': lambda cd, value:\n> cd.set_committer(Person.parse(value))\n> }\n> for line in lines:\n> try:\n> key, value = line.split(' ', 1)\n> cd = updater[key](cd, value)\n> except ValueError:\n> continue\n> except KeyError:\n> continue\n> \n> It documents about the same, but adds checking on double 'case'\n> statements. The resulting for loop is rather cleaner and the exception\n> approach becomes even more logical. I rather like the result, but I\n> guess it's mostly a matter of taste.\n\nI know this approach and use it frequently, but when one has to resort\nto lambdas and there are only four cases, it becomes IMHO less readable\nthan the if..else version.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"184198","messageId":"op.v9dl1e0v0aolir@keputer.lokaal","threadId":"29570","inReplyTo":"4F32A014.1000608@alum.mit.edu","subject":"Re: [StGit PATCH] Parse commit object header correctly","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2012-02-08T20:04:16Z","receivedAt":"2012-02-08T20:04:16Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Wed, 08 Feb 2012 17:17:24 +0100, Michael Haggerty \n wrote:\n\n> On 02/08/2012 11:43 AM, Frans Klaver wrote:\n>> On Wed, Feb 8, 2012 at 11:00 AM, Michael Haggerty \n>> wrote:\n>>> On 02/08/2012 08:33 AM, Junio C Hamano wrote:\n>>>> + line = lines[i].rstrip('\\n')\n>>>> + ix = line.find(' ')\n>>>> + if 0 <= ix:\n>>>> + key, value = line[0:ix], line[ix+1:]\n>>>\n>>> The above five lines can be written\n>>>\n>>> for line in lines:\n>>> if ' ' in line:\n>>> key, value = line.rstrip('\\n').split(' ', 1)\n>>>\n>>> or (if the lack of a space should be treated more like an exception)\n>>>\n>>> for line in lines:\n>>> try:\n>>> key, value = line.rstrip('\\n').split(' ', 1)\n>>> except ValueError:\n>>> continue\n>>\n>> This is generally considered more pythonic: \"It's easier to ask for\n>> forgiveness than to get permission\".\n>\n> Given that Junio explicitly wanted to allow lines with no spaces, I\n> assume that lack of a space is not an error but rather a conceivable\n> future extension. If my assumption is correct, then it is misleading\n> (and inefficient) to handle it via an exception.\n\nI find the documenting more convincing than the efficiency, but from the \nphrasing I think you do too.\n\n\n>>> for line in lines:\n>>> if ' ' in line:\n>>> key, value = line.split(' ', 1)\n>>> if key == 'tree':\n>>> cd = cd.set_tree(repository.get_tree(value))\n>>> elif key == 'parent':\n>>> cd = cd.add_parent(repository.get_commit(value))\n>>> elif key == 'author':\n>>> cd = cd.set_author(Person.parse(value))\n>>> elif key == 'committer':\n>>> cd = cd.set_committer(Person.parse(value))\n>>> return cd\n>>\n>> One could also take the recommended python approach for\n>> switch/case-like if/elif/else statements:\n>>\n>> updater = { 'tree': lambda cd, value: \n>> cd.set_tree(repository.get_tree(value),\n>> 'parent': lambda cd, value:\n>> cd.add_parent(repository.get_commit(value)),\n>> 'author': lambda cd, value: \n>> cd.set_author(Person.parse(value)),\n>> 'committer': lambda cd, value:\n>> cd.set_committer(Person.parse(value))\n>> }\n>> for line in lines:\n>> try:\n>> key, value = line.split(' ', 1)\n>> cd = updater[key](cd, value)\n>> except ValueError:\n>> continue\n>> except KeyError:\n>> continue\n>>\n>> It documents about the same, but adds checking on double 'case'\n>> statements. The resulting for loop is rather cleaner and the exception\n>> approach becomes even more logical. I rather like the result, but I\n>> guess it's mostly a matter of taste.\n>\n> I know this approach and use it frequently, but when one has to resort\n> to lambdas and there are only four cases, it becomes IMHO less readable\n> than the if..else version.\n\nWell, as I said, its largely a matter of taste; four items is a corner \ncase to me when thinking maintainability vs. readability. On the other \nhand, this doesn't seem like an oft-changing piece of code, so a longer \nlist of if..elif..else shouldn't be a problem either.\n\nFrans\n"},{"id":"184215","messageId":"7v39akzmgx.fsf@alter.siamese.dyndns.org","threadId":"29570","inReplyTo":"op.v9dl1e0v0aolir@keputer.lokaal","subject":"Re: [StGit PATCH] Parse commit object header correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-09T03:58:54Z","receivedAt":"2012-02-09T03:58:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Frans Klaver\" writes:\n\n>>>> for line in lines:\n>>>> try:\n>>>> key, value = line.rstrip('\\n').split(' ', 1)\n>>>> except ValueError:\n>>>> continue\n>>>\n>>> This is generally considered more pythonic: \"It's easier to ask for\n>>> forgiveness than to get permission\".\n>>\n>> Given that Junio explicitly wanted to allow lines with no spaces, I\n>> assume that lack of a space is not an error but rather a conceivable\n>> future extension. If my assumption is correct, then it is misleading\n>> (and inefficient) to handle it via an exception.\n>\n> I find the documenting more convincing than the efficiency, but from\n> the phrasing I think you do too.\n\nA line that consists entirely of non-SP may or may not a conceivable\nfuture extension, but the point is to \"skip without barfing anything you\ndo not understand\".\n\nI wouldn't oppose the rewrite that uses try/except ValueError if\n\"everything in this try block will parse what I understand correctly, and\nany ValueError exception this try block throws is an indication that I\nencountered what I do not understand and I must skip\" is the more pythonic\nway to express that principle. Python is not my primary language as I\nsaid, and in addition StGit may have its own style I haven't learned.\n"},{"id":"184227","messageId":"CAHkRjk6dr=5wxm+iSC2_CSB-q3k2WG_Um+X7dwsy-H8tL508EA@mail.gmail.com","threadId":"29570","inReplyTo":"7vd39pzsmq.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [StGit PATCH] Parse commit object header correctly","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2012-02-09T09:38:00Z","receivedAt":"2012-02-09T09:38:00Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"Hi Junio,\n\nOn 8 February 2012 07:33, Junio C Hamano wrote:\n> To allow parsing the header produced by versions of Git newer than the\n> code written to parse it, all commit parsers are expected to skip unknown\n> header lines, so that newer types of header lines can be added safely.\n> The only three things that are promised are:\n>\n>  (1) the header ends with an empty line (just an LF, not \"a blank line\"),\n>  (2) unknown lines can be skipped, and\n>  (3) a header \"field\" begins with the field name, followed by a single SP\n>     followed by the value.\n\nThanks for looking into this. Is this the same as an email header? If\nyes, we could just use the python's email.Header.decode_header()\nfunction (I haven't tried yet).\n\nBTW, does Git allow custom headers to be inserted by tools like StGit?\n\n-- \nCatalin\n"},{"id":"184243","messageId":"20120209175158.GA7384@burratino","threadId":"29570","inReplyTo":"CAHkRjk6dr=5wxm+iSC2_CSB-q3k2WG_Um+X7dwsy-H8tL508EA@mail.gmail.com","subject":"Re: [StGit PATCH] Parse commit object header correctly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-09T17:51:58Z","receivedAt":"2012-02-09T17:51:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Catalin,\n\nCatalin Marinas wrote:\n\n> Thanks for looking into this. Is this the same as an email header? If\n> yes, we could just use the python's email.Header.decode_header()\n> function (I haven't tried yet).\n\nThey look like this:\n\n\tencoding ISO8859-1\n\n> BTW, does Git allow custom headers to be inserted by tools like StGit?\n\nNo. There is one list of supported headers, and this list is the\nstandards body that maintains it[*]. So if you end up needing an\nextension to the commit object format, that can be done, but it needs\nto be accepted here (and ideally checked by \"git fsck\", though it's\nlagging a bit in that respect lately).\n\nBy the way, headers have a standard order to avoid spurious changes in\ncommit names from reordering. Additions so far have always happened\nat the end, which is what makes checks by \"git fsck\" possible --- it\ncan't rule out an unrecognized header line being a standard field from\na future version of git, but it would be allowed to complain about\nunrecognized fields before 'encoding', for example.\n\nThanks,\nJonathan\n\n[*] http://thread.gmane.org/gmane.comp.version-control.git/138848/focus=138892\n"},{"id":"184251","messageId":"7vk43vx1zx.fsf@alter.siamese.dyndns.org","threadId":"29570","inReplyTo":"CAHkRjk6dr=5wxm+iSC2_CSB-q3k2WG_Um+X7dwsy-H8tL508EA@mail.gmail.com","subject":"Re: [StGit PATCH] Parse commit object header correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-09T19:04:02Z","receivedAt":"2012-02-09T19:04:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Catalin Marinas writes:\n\n> On 8 February 2012 07:33, Junio C Hamano wrote:\n>> To allow parsing the header produced by versions of Git newer than the\n>> code written to parse it, all commit parsers are expected to skip unknown\n>> header lines, so that newer types of header lines can be added safely.\n>> The only three things that are promised are:\n>>\n>>  (1) the header ends with an empty line (just an LF, not \"a blank line\"),\n>>  (2) unknown lines can be skipped, and\n>>  (3) a header \"field\" begins with the field name, followed by a single SP\n>>     followed by the value.\n>\n> Thanks for looking into this. Is this the same as an email header? If\n> yes, we could just use the python's email.Header.decode_header()\n> function (I haven't tried yet).\n\nIf you are thinking about feeding everything up to the first empty line to\nwhatever is designed to parse email header, please don't. I do not think\nthey obey \"skip unknown lines without barfing\" rule [*1*], so we would be\nback to square one if you did so.\n\nThe fix posted in this thread is necessary because the change to StGit\nbetween v0.15 and v0.16 made to ignore lines starting with \"encoding \" was\na wrong way to work around the broken parser in v0.15 in the first place.\nThe parser assumed that (1) all whitespaces around the header lines can be\nstripped, (2) the result after such stripping will always have at least\none whitespace so that line.split(None, 1) will never barf, and (3)\nbetween the field name and its value there may be arbitrary number of\nwhitespace characters that can be ignored so that line.split(None, 1) is a\nsafe way to split it into a (key,value) pair. None of which is a safe\nthing to assume. The rule for safe parsing is to ignore all lines it does\nnot understand without assuming anything, and I wrote the patch in this\nthread to make sure it makes no such unwarranted assumption.\n\n> BTW, does Git allow custom headers to be inserted by tools like StGit?\n\nThe header format is designed in such a way that it is safe for a parser\nto silently ignore unknown cruft, but that also means tools that work on\nan existing commit and produce a similar one, like \"commit --amend\", are\nfree to either ignore and drop them when creating a new commit out of the\noriginal one, or replay it verbatim without adjusting them to the new\ncontext they appear in. In that sense, they are technically \"allowed\",\nbut depending on the nature of the information you are putting there, it\nsemantically may or may not produce the desired result [*2*]. I would say\nit is strongly discouraged to invent new types of header lines without\nfirst consulting the people who maintain tools you must interoperate with,\nso that they will also be aware of them, and hopefully their tools can be\nadjusted to help you use them.\n\nSorry for the breakage and making you to deal with this post release. We\nobserved that recent StGit did not barf after we added \"encoding \" field\nto Git, and assumed that StGit correctly ignored lines that it did not\nunderstand, without inspecting its code.\n\nAt least we should have Cc'ed you guys directly when the change was being\ndiscussed on the list.\n\n\n[Footnote]\n\n*1* I also suspect that it will handle a line that begins with a single SP\ndifferently if you use email parsing rules. In a commit object header, the\ncontent of such a line is appended to the value of the previous field\nafter turning that leading single SP into a LF, and the resulting value\nwill be a string that consists of multiple lines. The header folding rule\nused for e-mail in RFC2822 is a way to represent a (logically) single line\nas physically multiple lines, so the result of unfolding will become a\nsingle line. This difference may not matter for the purpose of the\ncurrent StGit that understands nothing but tree/parent/author/committer,\nbut because we are discussing a forward-looking fix for its parser, I\nwouldn't recommend \"it does not matter because we do not currently care\"\napproach.\n\n*2* For example, a line that begins with \"gpgsig \" is a field that records\nthe GPG signature of a commit itself (using \"git commit -S\"), and it\nshould not survive across \"commit --amend\". A line that begins with\n\"mergetag \" is a field that records the tag information that was merged\nfrom a side branch, and amending such a merge does not change what was\nmerged, so it should survive across \"commit --amend\".\n"},{"id":"184323","messageId":"CACsJy8AT68zYHnWGurVW19-SZMXoMYhagWqG7iQENBLU9qsRMA@mail.gmail.com","threadId":"29570","inReplyTo":"20120209175158.GA7384@burratino","subject":"Re: [StGit PATCH] Parse commit object header correctly","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-10T04:27:35Z","receivedAt":"2012-02-10T04:27:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 10, 2012 at 12:51 AM, Jonathan Nieder wrote:\n> No.  There is one list of supported headers, and this list is the\n> standards body that maintains it[*].  So if you end up needing an\n> extension to the commit object format, that can be done, but it needs\n> to be accepted here (and ideally checked by \"git fsck\", though it's\n> lagging a bit in that respect lately).\n>\n> [*] http://thread.gmane.org/gmane.comp.version-control.git/138848/focus=138892\n\nDoesn't this deserve a document in Documentation/technical? It's also\na good opportunity to document tree and tag object format in addition\nto commit object. I did not check thoroughly but the commit that\nintroduced encoding field, 4b2bced, did not come with any document\nupdates, so I assume it has not been documented ever since.\n-- \nDuy\n"},{"id":"184760","messageId":"CAHkRjk451=_XaQuUXmxAvB3sRRz6-J+c7A2ZrfLwfGz=z05Lag@mail.gmail.com","threadId":"29570","inReplyTo":"4F3247CA.1020904@alum.mit.edu","subject":"Re: [StGit PATCH] Parse commit object header correctly","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2012-02-15T12:24:34Z","receivedAt":"2012-02-15T12:24:34Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"On 8 February 2012 10:00, Michael Haggerty wrote:\n> On 02/08/2012 08:33 AM, Junio C Hamano wrote:\n>> To allow parsing the header produced by versions of Git newer than the\n>> code written to parse it, all commit parsers are expected to skip unknown\n>> header lines, so that newer types of header lines can be added safely.\n>> The only three things that are promised are:\n>>\n>>  (1) the header ends with an empty line (just an LF, not \"a blank line\"),\n>>  (2) unknown lines can be skipped, and\n>>  (3) a header \"field\" begins with the field name, followed by a single SP\n>>      followed by the value.\n>>\n>> The parser used by StGit, introduced by commit cbe4567 (New StGit core\n>> infrastructure: repository operations, 2007-12-19), was accidentally a bit\n>> too loose to lose information, and a bit too strict to raise exception\n>> when dealing with a line it does not understand.\n...\n> All in all, I would recommend something like (untested):\n>\n>        @return: A new L{CommitData} object\n>        @rtype: L{CommitData}\"\"\"\n>        cd = cls(parents = [])\n>        lines = []\n>        raw_lines = s.split('\\n')\n>        # Collapse multi-line header lines\n>        for i, line in enumerate(raw_lines):\n>            if not line:\n>                cd.set_message('\\n'.join(raw_lines[i+1:]))\n>                break\n>            if line.startswith(' '):\n>                # continuation line\n>                lines[-1] += '\\n' + line[1:]\n>            else:\n>                lines.append(line)\n>\n>        for line in lines:\n>            if ' ' in line:\n>                key, value = line.split(' ', 1)\n>                if key == 'tree':\n>                    cd = cd.set_tree(repository.get_tree(value))\n>                elif key == 'parent':\n>                    cd = cd.add_parent(repository.get_commit(value))\n>                elif key == 'author':\n>                    cd = cd.set_author(Person.parse(value))\n>                elif key == 'committer':\n>                    cd = cd.set_committer(Person.parse(value))\n>        return cd\n\nThank you all for comments and patches. I used a combination of\nJunio's patch with the comments from Michael and a fix from me. I'll\npublish it to the 'master' branch shortly and release a 0.16.1\nhopefully this week.\n\n-- \nCatalin\n"},{"id":"184776","messageId":"7vaa4k0xtz.fsf@alter.siamese.dyndns.org","threadId":"29570","inReplyTo":"CAHkRjk451=_XaQuUXmxAvB3sRRz6-J+c7A2ZrfLwfGz=z05Lag@mail.gmail.com","subject":"Re: [StGit PATCH] Parse commit object header correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-15T18:13:12Z","receivedAt":"2012-02-15T18:13:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Catalin Marinas writes:\n\n> ... I'll\n> publish it to the 'master' branch shortly and release a 0.16.1\n> hopefully this week.\n\nThanks.\n"},{"id":"184778","messageId":"4F3BFC2C.8020007@warmcat.com","threadId":"29570","inReplyTo":"CAHkRjk451=_XaQuUXmxAvB3sRRz6-J+c7A2ZrfLwfGz=z05Lag@mail.gmail.com","subject":"Re: [StGit PATCH] Parse commit object header correctly","fromName":"Andy Green (林安廸)","fromEmail":"andy@warmcat.com","sentAt":"2012-02-15T18:40:44Z","receivedAt":"2012-02-15T18:40:44Z","isPatch":true,"sender":{"key":"andy@warmcat.com","avatar":"https://gravatar.com/avatar/cc8d9622972f22b99adb7d46cad8ed719cc7088eafbdef72639b74bce045ae49?d=mp&s=160"},"body":"On 02/15/2012 04:24 AM, Somebody in the thread at some point said:\n\nHi -\n\n> Thank you all for comments and patches. I used a combination of\n> Junio's patch with the comments from Michael and a fix from me. I'll\n> publish it to the 'master' branch shortly and release a 0.16.1\n> hopefully this week.\n\nI cloned the master branch and installed it locally, it's working well.\n\nThanks a lot to the guys who spent time on this bug and stgit overall,\nwhich I rely heavily on!\n\n-Andy\n"}]}