{"thread":{"id":"13946","subject":"[StGIT PATCH] Implement a new patch identification scheme and id command","startedAt":"2008-06-14T07:28:33Z","lastAt":"2008-06-16T14:08:57Z","messageCount":6,"participants":["Catalin Marinas","Karl Hasselström"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"79826","messageId":"20080614072833.7899.91460.stgit@localhost.localdomain","threadId":"13946","inReplyTo":null,"subject":"[StGIT PATCH] Implement a new patch identification scheme and id command","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-14T07:28:33Z","receivedAt":"2008-06-14T07:28:33Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"The new scheme allows '[:]' and '[:]{base}'\n(the latter showing the base of a stack). The former format allows\nsymbols like ^, ^{...} etc.\n\nSigned-off-by: Catalin Marinas \n---\n\n stgit/commands/common.py | 32 ++++++++++++++++++++++++++++++++\n stgit/commands/id.py | 25 +++++++++++--------------\n stgit/lib/git.py | 4 ++--\n t/t0001-subdir-branches.sh | 26 +++++++++++---------------\n t/t1200-push-modified.sh | 2 +-\n t/t1201-pull-trailing.sh | 2 +-\n t/t2200-rebase.sh | 2 +-\n 7 files changed, 59 insertions(+), 34 deletions(-)\n\ndiff --git a/stgit/commands/common.py b/stgit/commands/common.py\nindex 029ec65..d1545e4 100644\n--- a/stgit/commands/common.py\n+++ b/stgit/commands/common.py\n@@ -28,6 +28,7 @@ from stgit.run import *\n from stgit import stack, git, basedir\n from stgit.config import config, file_extensions\n from stgit.lib import stack as libstack\n+from stgit.lib import git as libgit\n \n # Command exception class\n class CmdException(StgException):\n@@ -116,6 +117,37 @@ def git_id(crt_series, rev):\n \n raise CmdException, 'Unknown patch or revision: %s' % rev\n \n+def git_sha1(repository, branch, name):\n+ \"\"\"Return the SHA1 value if 'name' is a patch name or Git commit.\n+ The patch names allowed are in the form ':' and can be\n+ followed by standard symbols used by git-rev-parse. If is '{base}',\n+ it represents the bottom of the stack.\n+ \"\"\"\n+ # Try a Git commit first\n+ try:\n+ return repository.rev_parse(name, discard_stderr = True).sha1\n+ except libgit.RepositoryException:\n+ pass\n+\n+ # Git didn't recognise it, try a [branch:]patch name\n+ try:\n+ branch, patch = name.split(':', 1)\n+ except ValueError:\n+ patch = name\n+ if not branch:\n+ branch = repository.current_branch_name\n+\n+ # the stack base\n+ if patch == '{base}':\n+ return repository.get_stack(branch).base.sha1\n+\n+ # any other combination of branch and patch\n+ try:\n+ return repository.rev_parse('patches/%s/%s' % (branch, patch),\n+ discard_stderr = True).sha1\n+ except libgit.RepositoryException:\n+ raise CmdException('%s: Unknown patch or revision name' % name)\n+\n def check_local_changes():\n if git.local_changes():\n raise CmdException('local changes in the tree. Use \"refresh\" or'\ndiff --git a/stgit/commands/id.py b/stgit/commands/id.py\nindex 94b0229..50f6e8a 100644\n--- a/stgit/commands/id.py\n+++ b/stgit/commands/id.py\n@@ -15,29 +15,26 @@ along with this program; if not, write to the Free Software\n Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA\n \"\"\"\n \n-import sys, os\n from optparse import OptionParser, make_option\n \n-from stgit.commands.common import *\n-from stgit.utils import *\n-from stgit.out import *\n-from stgit import stack, git\n-\n+from stgit.out import out\n+from stgit.commands import common\n+from stgit.lib import stack\n \n help = 'print the GIT hash value of a StGIT reference'\n usage = \"\"\"%prog [options] [id]\n \n-Print the hash value of a GIT id (defaulting to HEAD). In addition to\n-the standard GIT id's like heads and tags, this command also accepts\n-'base[@]' and '[[@]][//[bottom | top]]'. If no\n-'top' or 'bottom' are passed and is a valid patch name, 'top'\n-will be used by default.\"\"\"\n+Print the SHA1 value of a Git id (defaulting to HEAD). In addition to\n+the standard Git id's like heads and tags, this command also accepts\n+'[:]]' and '[:]{base}]' showing the id of a patch\n+or the base of the stack. If no branch is specified, it defaults to the\n+current one. The bottom of a patch is accessible with the\n+'[:]]^' format.\"\"\"\n \n-directory = DirectoryHasRepository()\n+directory = common.DirectoryHasRepositoryLib()\n options = [make_option('-b', '--branch',\n help = 'use BRANCH instead of the default one')]\n \n-\n def func(parser, options, args):\n \"\"\"Show the applied patches\n \"\"\"\n@@ -48,4 +45,4 @@ def func(parser, options, args):\n else:\n parser.error('incorrect number of arguments')\n \n- out.stdout(git_id(crt_series, id_str))\n+ out.stdout(common.git_sha1(directory.repository, options.branch, id_str))\ndiff --git a/stgit/lib/git.py b/stgit/lib/git.py\nindex 0e0cccb..379fb52 100644\n--- a/stgit/lib/git.py\n+++ b/stgit/lib/git.py\n@@ -422,11 +422,11 @@ class Repository(RunWithEnv):\n refs = property(lambda self: self.__refs)\n def cat_object(self, sha1):\n return self.run(['git', 'cat-file', '-p', sha1]).raw_output()\n- def rev_parse(self, rev):\n+ def rev_parse(self, rev, discard_stderr = False):\n try:\n return self.get_commit(self.run(\n ['git', 'rev-parse', '%s^{commit}' % rev]\n- ).output_one_line())\n+ ).discard_stderr(discard_stderr).output_one_line())\n except run.RunException:\n raise RepositoryException('%s: No such revision' % rev)\n def get_tree(self, sha1):\ndiff --git a/t/t0001-subdir-branches.sh b/t/t0001-subdir-branches.sh\nindex 0eed3a4..cf2059a 100755\n--- a/t/t0001-subdir-branches.sh\n+++ b/t/t0001-subdir-branches.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n #\n-# Copyright (c) 2006 Karl Hasselstr�m\n+# Copyright (c) 2006 Karl Hasselstr�m\n #\n \n test_description='Branch names containing slashes\n@@ -18,25 +18,21 @@ test_expect_success 'Create a patch' \\\n stg new foo -m \"Add foo.txt\" &&\n stg refresh'\n \n-test_expect_success 'Old and new id with non-slashy branch' \\\n- 'stg id foo &&\n- stg id foo// &&\n- stg id foo/ &&\n- stg id foo//top &&\n- stg id foo/top &&\n- stg id foo@master &&\n- stg id foo@master//top &&\n- stg id foo@master/top'\n+test_expect_success 'Try id with non-slashy branch' \\\n+ 'stg id &&\n+ stg id foo &&\n+ stg id foo^ &&\n+ stg id master:foo &&\n+ stg id master:foo^'\n \n test_expect_success 'Clone branch to slashier name' \\\n 'stg branch --clone x/y/z'\n \n-test_expect_success 'Try new form of id with slashy branch' \\\n+test_expect_success 'Try new id with slashy branch' \\\n 'stg id foo &&\n- stg id foo// &&\n- stg id foo//top &&\n- stg id foo@x/y/z &&\n- stg id foo@x/y/z//top'\n+ stg id foo^ &&\n+ stg id x/y/z:foo &&\n+ stg id x/y/z:foo^'\n \n test_expect_success 'Try old id with slashy branch' '\n ! stg id foo/ &&\ndiff --git a/t/t1200-push-modified.sh b/t/t1200-push-modified.sh\nindex ba4f70c..e3c6425 100755\n--- a/t/t1200-push-modified.sh\n+++ b/t/t1200-push-modified.sh\n@@ -36,7 +36,7 @@ test_expect_success \\\n (\n cd foo &&\n GIT_DIR=../bar/.git git-format-patch --stdout \\\n- $(cd ../bar && stg id base@master)..HEAD | git-am -3 -k\n+ $(cd ../bar && stg id master:{base})..HEAD | git-am -3 -k\n )\n '\n \ndiff --git a/t/t1201-pull-trailing.sh b/t/t1201-pull-trailing.sh\nindex 9d70fe0..8a74873 100755\n--- a/t/t1201-pull-trailing.sh\n+++ b/t/t1201-pull-trailing.sh\n@@ -30,7 +30,7 @@ test_expect_success \\\n 'Port those patches to orig tree' \\\n '(cd foo &&\n GIT_DIR=../bar/.git git-format-patch --stdout \\\n- $(cd ../bar && stg id base@master)..HEAD |\n+ $(cd ../bar && stg id master:{base})..HEAD |\n git-am -3 -k\n )\n '\ndiff --git a/t/t2200-rebase.sh b/t/t2200-rebase.sh\nindex ec2a104..cd43c41 100755\n--- a/t/t2200-rebase.sh\n+++ b/t/t2200-rebase.sh\n@@ -27,7 +27,7 @@ test_expect_success \\\n \t'Rebase to previous commit' \\\n \t'\n \tstg rebase master~1 &&\n-\ttest `stg id base@stack` = `git rev-parse master~1` &&\n+\ttest `stg id stack:{base}` = `git rev-parse master~1` &&\n \ttest `stg applied | wc -l` = 1\n \t'\n \n"},{"id":"79828","messageId":"b0943d9e0806140032p53a02352h5de1b8aa3a37e07f@mail.gmail.com","threadId":"13946","inReplyTo":"20080614072833.7899.91460.stgit@localhost.localdomain","subject":"Re: [StGIT PATCH] Implement a new patch identification scheme and id command","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-14T07:32:29Z","receivedAt":"2008-06-14T07:32:29Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/6/14 Catalin Marinas :\n> --- a/t/t0001-subdir-branches.sh\n> +++ b/t/t0001-subdir-branches.sh\n> @@ -1,6 +1,6 @@\n> #!/bin/sh\n> #\n> -# Copyright (c) 2006 Karl Hasselström\n> +# Copyright (c) 2006 Karl Hasselstr�m\n\nI'll remove this hunk :-) (not sure how it got here).\n\n-- \nCatalin\n"},{"id":"79842","messageId":"20080614094714.GC14282@diana.vm.bytemark.co.uk","threadId":"13946","inReplyTo":"20080614072833.7899.91460.stgit@localhost.localdomain","subject":"Re: [StGIT PATCH] Implement a new patch identification scheme and id command","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-06-14T09:47:14Z","receivedAt":"2008-06-14T09:47:14Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-06-14 08:28:33 +0100, Catalin Marinas wrote:\n\n> The new scheme allows '[:]' and '[:]{base}'\n> (the latter showing the base of a stack). The former format allows\n> symbols like ^, ^{...} etc.\n\nI like your choices.\n\n> +def git_sha1(repository, branch, name):\n> + \"\"\"Return the SHA1 value if 'name' is a patch name or Git commit.\n> + The patch names allowed are in the form ':' and can be\n> + followed by standard symbols used by git-rev-parse. If is '{base}',\n> + it represents the bottom of the stack.\n> + \"\"\"\n\nWhy not return the Commit directly, and let the caller extract its\nsha1 if that's what it wants?\n\nYou don't remove the old parse_rev() and git_id(), and particularly\nthe latter has a lot of callers. Meaning that the rest of StGit still\nspeaks the old syntax.\n\n> + # Try a Git commit first\n> + try:\n> + return repository.rev_parse(name, discard_stderr = True).sha1\n> + except libgit.RepositoryException:\n> + pass\n\nWhat if you have a branch or tag with the same name as a patch? This\nwill prefer the branch, which might not be the best choice.\n\n> +current one. The bottom of a patch is accessible with the\n> +'[:]]^' format.\"\"\"\n\nYou have an extra ] here.\n\n> -directory = DirectoryHasRepository()\n> +directory = common.DirectoryHasRepositoryLib()\n> options = [make_option('-b', '--branch',\n> help = 'use BRANCH instead of the default one')]\n\nCouldn't we kill this option? (And in the process, the branch argument\nto git_sha1.)\n\n> -test_expect_success 'Try new form of id with slashy branch' \\\n> +test_expect_success 'Try new id with slashy branch' \\\n\nStrictly speaking, this isn't so new anymore.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"80011","messageId":"b0943d9e0806160300q55dde16fg90de0fa12e3d5dc0@mail.gmail.com","threadId":"13946","inReplyTo":"20080614094714.GC14282@diana.vm.bytemark.co.uk","subject":"Re: [StGIT PATCH] Implement a new patch identification scheme and id command","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-16T10:00:22Z","receivedAt":"2008-06-16T10:00:22Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/6/14 Karl Hasselström :\n> On 2008-06-14 08:28:33 +0100, Catalin Marinas wrote:\n>\n>> The new scheme allows '[:]' and '[:]{base}'\n>> (the latter showing the base of a stack). The former format allows\n>> symbols like ^, ^{...} etc.\n>\n> I like your choices.\n\nI think the initial idea belongs to Yann.\n\n>> +def git_sha1(repository, branch, name):\n>> + \"\"\"Return the SHA1 value if 'name' is a patch name or Git commit.\n>> + The patch names allowed are in the form ':' and can be\n>> + followed by standard symbols used by git-rev-parse. If is '{base}',\n>> + it represents the bottom of the stack.\n>> + \"\"\"\n>\n> Why not return the Commit directly, and let the caller extract its\n> sha1 if that's what it wants?\n\nOK, I was thinking about that. I'll convert it to git_commit(...) and\nreturn a commit since I think it's only \"stg id\" that needs the SHA1\nvalue.\n\n> You don't remove the old parse_rev() and git_id(), and particularly\n> the latter has a lot of callers. Meaning that the rest of StGit still\n> speaks the old syntax.\n\nI thought about removing them when we convert the commands to the new\ninfrastructure. In the meantime, I can rewrite git_id to use\ngit_commit directly. The parse_rev is only used by the 'pick' command\n(and git_id). I'll have a look at these functions.\n\n>> + # Try a Git commit first\n>> + try:\n>> + return repository.rev_parse(name, discard_stderr = True).sha1\n>> + except libgit.RepositoryException:\n>> + pass\n>\n> What if you have a branch or tag with the same name as a patch? This\n> will prefer the branch, which might not be the best choice.\n\nI can swap them and have the patch as preferred. People can be more\nprecise if they want the branch or tag by passing heads/... or\ntags/...\n\n>> -directory = DirectoryHasRepository()\n>> +directory = common.DirectoryHasRepositoryLib()\n>> options = [make_option('-b', '--branch',\n>> help = 'use BRANCH instead of the default one')]\n>\n> Couldn't we kill this option? (And in the process, the branch argument\n> to git_sha1.)\n\nNo problem with the option but I would like to keep the branch\nargument to git_sha1. There might be cases where it is used like\npicking multiple patches and I only specify one -B option to 'pick'. I\ndon't want to build the qualified patch name for every patch. I can\nchange the prototype of git_sha1 though (to git_commit):\n\ndef git_commit(name, repository, branch = None):\n\nand branch needs to be passed explicitly and used instead of the\ndefault one (when the patch name doesn't contain any).\n\n>> -test_expect_success 'Try new form of id with slashy branch' \\\n>> +test_expect_success 'Try new id with slashy branch' \\\n>\n> Strictly speaking, this isn't so new anymore.\n\nThe id format is new.\n\n-- \nCatalin\n"},{"id":"80028","messageId":"b0943d9e0806160630m12f5bc7x39053e6c81d983c0@mail.gmail.com","threadId":"13946","inReplyTo":"b0943d9e0806160300q55dde16fg90de0fa12e3d5dc0@mail.gmail.com","subject":"Re: [StGIT PATCH] Implement a new patch identification scheme and id command","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-16T13:30:32Z","receivedAt":"2008-06-16T13:30:32Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/6/16 Catalin Marinas :\n> 2008/6/14 Karl Hasselström :\n>> On 2008-06-14 08:28:33 +0100, Catalin Marinas wrote:\n>> You don't remove the old parse_rev() and git_id(), and particularly\n>> the latter has a lot of callers. Meaning that the rest of StGit still\n>> speaks the old syntax.\n>\n> I thought about removing them when we convert the commands to the new\n> infrastructure. In the meantime, I can rewrite git_id to use\n> git_commit directly. The parse_rev is only used by the 'pick' command\n> (and git_id). I'll have a look at these functions.\n\nDone that - git_id/parse_rev now use the new id mechanism. Eventually\nwe'll drop git_id once all the commands are converted.\n\n>>> -directory = DirectoryHasRepository()\n>>> +directory = common.DirectoryHasRepositoryLib()\n>>> options = [make_option('-b', '--branch',\n>>> help = 'use BRANCH instead of the default one')]\n>>\n>> Couldn't we kill this option? (And in the process, the branch argument\n>> to git_sha1.)\n>\n> No problem with the option but I would like to keep the branch\n> argument to git_sha1. There might be cases where it is used like\n> picking multiple patches and I only specify one -B option to 'pick'.\n\nWhat about supporting patch ranges with the new id format, something like:\n\n branch:patch1..patch2,patch3\n\nor\n\n branch:patch1..patch2 branch:patch3\n\nThe following can be valid as well but patch3 may be on a different branch:\n\n branch:patch1..patch2 patch3\n\nThis way we could get rid of many --branch options.\n\n-- \nCatalin\n"},{"id":"80030","messageId":"20080616140857.GA24433@diana.vm.bytemark.co.uk","threadId":"13946","inReplyTo":"b0943d9e0806160630m12f5bc7x39053e6c81d983c0@mail.gmail.com","subject":"Re: [StGIT PATCH] Implement a new patch identification scheme and id command","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-06-16T14:08:57Z","receivedAt":"2008-06-16T14:08:57Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-06-16 14:30:32 +0100, Catalin Marinas wrote:\n\n> What about supporting patch ranges with the new id format, something\n> like:\n>\n> branch:patch1..patch2,patch3\n>\n> or\n>\n> branch:patch1..patch2 branch:patch3\n\nYes, that's a good idea; the endpoints of a range have to be on the\nsame branch no matter what, so having the branch: prefix apply to both\nof the endpoint patches in \"branch:patch1..patch2\" is a good idea.\n\nI'm not sure if the comma notation is worth it. And if it turns out to\nhave been useful, we can just advise users to write\n\n $ stg foo branch:{p1..p2,p3}\n\nwhich the shell will expand to\n\n $ stg foo branch:p1..p2 branch:p3\n\n> This way we could get rid of many --branch options.\n\nIndeed.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"}]}