{"thread":{"id":"27261","subject":"[PATCH v3] git-p4: add option to preserve user names","startedAt":"2011-05-05T06:43:38Z","lastAt":"2011-05-08T20:35:28Z","messageCount":9,"participants":["Luke Diamand","Junio C Hamano","Pete Wyckoff"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"167082","messageId":"1304577819-703-1-git-send-email-luke@diamand.org","threadId":"27261","inReplyTo":null,"subject":"[PATCH v3] git-p4: add option to preserve user names","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2011-05-05T06:43:38Z","receivedAt":"2011-05-05T06:43:38Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"This is version 3 of my patch. It's almost the same as\nthe previous one, except that:\n\n - it skips changing the user if the user field is already\n   correct\n\n - it puts a comment into the submit template saying what\n   the user will be changed to (so you know it hasn't\n   forgotten).\n\n - fixes a bug in actually forming the changespec (I think\n   it was relying on the way python orders dictionaries).\n---\n\nPatches from git passed into p4 end up with the committer\nbeing identified as the person who ran git-p4.\n\nThis patch adds an option --preserve-user. When enabled, git-p4\nwill modify the changelist after it has been submitted (\"p4 change -f\")\nand set the username to the one matching the git author.\n\nIf the person running git-p4 does not have sufficient permissions,\ngit-p4 will refuse to run (detected using \"p4 protects\").\nIt's possible that complicated permissions setups might confuse\ngit-p4 - it just looks to see if the user has admin or super on\nthe repo. In theory they might have permissions in some parts\nand not in others.\n\nIf there are commits with authors who do not have p4 accounts, then\ngit-p4 will refuse to run unless git-p4.allowMissingP4Users is true,\nin which case it falls back to the standard behaviour for those\ncommits.\n\nThe code has to get the p4 changelist number. The way it\ndoes this is by simply doing 'p4 changes -c <client>', which\nmeans if someone else is using the same clientspec at the\nsame time, there is a potential race hazard. The alternative\nis to rewrite the submit logic to submit a properly marshalled\ntemplate, which felt a bit too intrusive.\n\nI've hoisted the p4 user name cache to a separate class, since it\ngets used in a couple of different places now.\n\nI've added an option git-p4.skipSubmitModTimeCheck so that I can\nwrite a test case without having to jump through hoops with the\neditor.\n\n\nLuke Diamand (1):\n  git-p4: add option to preserve user names\n\n contrib/fast-import/git-p4     |  188 ++++++++++++++++++++++++++++++++--------\n contrib/fast-import/git-p4.txt |   29 ++++++\n t/t9800-git-p4.sh              |   84 ++++++++++++++++++\n 3 files changed, 263 insertions(+), 38 deletions(-)\n"},{"id":"167083","messageId":"1304577819-703-2-git-send-email-luke@diamand.org","threadId":"27261","inReplyTo":"1304577819-703-1-git-send-email-luke@diamand.org","subject":"[PATCH v3] git-p4: add option to preserve user names","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2011-05-05T06:43:39Z","receivedAt":"2011-05-05T06:43:39Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Patches from git passed into p4 end up with the committer\nbeing identified as the person who ran git-p4.\n\nWith \"submit --preserve-user\", git-p4 modifies the p4\nchangelist (after it has been submitted), setting the\np4 author field.\n\nThe submitter is required to have sufficient p4 permissions\nor git-p4 refuses to proceed. If the git author is not\nknown to p4, the submit will be abandoned unless\ngit-p4.allowMissingP4Users is true.\n\nOption git-p4.skipSubmitModTimeCheck is added so that the unit\ntest for this can be written without needing editor interaction.\n\nSigned-off-by: Luke Diamand <luke@diamand.org>\n---\n contrib/fast-import/git-p4     |  188 ++++++++++++++++++++++++++++++++--------\n contrib/fast-import/git-p4.txt |   29 ++++++\n t/t9800-git-p4.sh              |   84 ++++++++++++++++++\n 3 files changed, 263 insertions(+), 38 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 78e5b3a..6018507 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -474,6 +474,47 @@ class Command:\n         self.usage = \"usage: %prog [options]\"\n         self.needsGit = True\n \n+class P4UserMap:\n+    def __init__(self):\n+        self.userMapFromPerforceServer = False\n+\n+    def getUserCacheFilename(self):\n+        home = os.environ.get(\"HOME\", os.environ.get(\"USERPROFILE\"))\n+        return home + \"/.gitp4-usercache.txt\"\n+\n+    def getUserMapFromPerforceServer(self):\n+        if self.userMapFromPerforceServer:\n+            return\n+        self.users = {}\n+        self.emails = {}\n+\n+        for output in p4CmdList(\"users\"):\n+            if not output.has_key(\"User\"):\n+                continue\n+            self.users[output[\"User\"]] = output[\"FullName\"] + \" <\" + output[\"Email\"] + \">\"\n+            self.emails[output[\"Email\"]] = output[\"User\"]\n+\n+\n+        s = ''\n+        for (key, val) in self.users.items():\n+            s += \"%s\\t%s\\n\" % (key.expandtabs(1), val.expandtabs(1))\n+\n+        open(self.getUserCacheFilename(), \"wb\").write(s)\n+        self.userMapFromPerforceServer = True\n+\n+    def loadUserMapFromCache(self):\n+        self.users = {}\n+        self.userMapFromPerforceServer = False\n+        try:\n+            cache = open(self.getUserCacheFilename(), \"rb\")\n+            lines = cache.readlines()\n+            cache.close()\n+            for line in lines:\n+                entry = line.strip().split(\"\\t\")\n+                self.users[entry[0]] = entry[1]\n+        except IOError:\n+            self.getUserMapFromPerforceServer()\n+\n class P4Debug(Command):\n     def __init__(self):\n         Command.__init__(self)\n@@ -554,13 +595,16 @@ class P4RollBack(Command):\n \n         return True\n \n-class P4Submit(Command):\n+class P4Submit(Command, P4UserMap):\n     def __init__(self):\n         Command.__init__(self)\n+        P4UserMap.__init__(self)\n         self.options = [\n                 optparse.make_option(\"--verbose\", dest=\"verbose\", action=\"store_true\"),\n                 optparse.make_option(\"--origin\", dest=\"origin\"),\n                 optparse.make_option(\"-M\", dest=\"detectRenames\", action=\"store_true\"),\n+                # preserve the user, requires relevant p4 permissions\n+                optparse.make_option(\"--preserve-user\", dest=\"preserveUser\", action=\"store_true\"),\n         ]\n         self.description = \"Submit changes from git to the perforce depot.\"\n         self.usage += \" [name of git branch to submit into perforce depot]\"\n@@ -568,6 +612,7 @@ class P4Submit(Command):\n         self.origin = \"\"\n         self.detectRenames = False\n         self.verbose = False\n+        self.preserveUser = gitConfig(\"git-p4.preserveUser\").lower() == \"true\"\n         self.isWindows = (platform.system() == \"Windows\")\n \n     def check(self):\n@@ -602,6 +647,80 @@ class P4Submit(Command):\n \n         return result\n \n+    def p4UserForCommit(self,id):\n+        # Return the tuple (perforce user,git email) for a given git commit id\n+        self.getUserMapFromPerforceServer()\n+        gitEmail = read_pipe(\"git log --max-count=1 --format='%%ae' %s\" % id)\n+        gitEmail = gitEmail.strip()\n+        if not self.emails.has_key(gitEmail):\n+            return (None,gitEmail)\n+        else:\n+            return (self.emails[gitEmail],gitEmail)\n+\n+    def checkValidP4Users(self,commits):\n+        # check if any git authors cannot be mapped to p4 users\n+        for id in commits:\n+            (user,email) = self.p4UserForCommit(id)\n+            if not user:\n+                msg = \"Cannot find p4 user for email %s in commit %s.\" % (email, id)\n+                if gitConfig('git-p4.allowMissingP4Users').lower() == \"true\":\n+                    print \"%s\" % msg\n+                else:\n+                    die(\"Error: %s\\nSet git-p4.allowMissingP4Users to true to allow this.\" % msg)\n+\n+    def lastP4Changelist(self):\n+        # Get back the last changelist number submitted in this client spec. This\n+        # then gets used to patch up the username in the change. If the same\n+        # client spec is being used by multiple processes then this might go\n+        # wrong.\n+        results = p4CmdList(\"client -o\")        # find the current client\n+        client = None\n+        for r in results:\n+            if r.has_key('Client'):\n+                client = r['Client']\n+                break\n+        if not client:\n+            die(\"could not get client spec\")\n+        results = p4CmdList(\"changes -c %s -m 1\" % client)\n+        for r in results:\n+            if r.has_key('change'):\n+                return r['change']\n+        die(\"Could not get changelist number for last submit - cannot patch up user details\")\n+\n+    def modifyChangelistUser(self, changelist, newUser):\n+        # fixup the user field of a single changelist after it has been submitted.\n+        changes = p4CmdList(\"change -o %s\" % changelist)\n+        input = ''\n+        for c in changes:\n+            if c.has_key('User'):\n+                if c['User'] == newUser:\n+                    # nothing to do here\n+                    return\n+                c['User'] = newUser\n+\n+            input = input + marshal.dumps(c)\n+\n+        result = p4CmdList(\"change -f -i\", stdin=input)\n+        for r in result:\n+            if r.has_key('code'):\n+                if r['code'] == 'error':\n+                    die(\"Could not modify user field of changelist %s to %s:%s\" % (changelist, newUser, r['data']))\n+            if r.has_key('data'):\n+                print(\"Updated user field for changelist %s to %s\" % (changelist, newUser))\n+                return\n+        die(\"Could not modify user field of changelist %s to %s\" % (changelist, newUser))\n+    def canChangeChangelists(self):\n+        # check to see if we have p4 admin or super-user permissions, either of\n+        # which are required to modify changelists.\n+        results = p4CmdList(\"protects %s\" % self.depotPath)\n+        for r in results:\n+            if r.has_key('perm'):\n+                if r['perm'] == 'admin':\n+                    return 1\n+                if r['perm'] == 'super':\n+                    return 1\n+        return 0\n+\n     def prepareSubmitTemplate(self):\n         # remove lines in the Files section that show changes to files outside the depot path we're committing into\n         template = \"\"\n@@ -631,6 +750,9 @@ class P4Submit(Command):\n     def applyCommit(self, id):\n         print \"Applying %s\" % (read_pipe(\"git log --max-count=1 --pretty=oneline %s\" % id))\n \n+        if self.preserveUser:\n+            (p4User, gitEmail) = self.p4UserForCommit(id)\n+\n         if not self.detectRenames:\n             # If not explicitly set check the config variable\n             self.detectRenames = gitConfig(\"git-p4.detectRenames\").lower() == \"true\"\n@@ -748,6 +870,10 @@ class P4Submit(Command):\n \n         if self.interactive:\n             submitTemplate = self.prepareLogMessage(template, logMessage)\n+\n+            if self.preserveUser:\n+               submitTemplate = submitTemplate + (\"\\n######## Actual user %s, modified after commit\\n\" % p4User)\n+\n             if os.environ.has_key(\"P4DIFF\"):\n                 del(os.environ[\"P4DIFF\"])\n             diff = \"\"\n@@ -781,8 +907,13 @@ class P4Submit(Command):\n                 editor = read_pipe(\"git var GIT_EDITOR\").strip()\n             system(editor + \" \" + fileName)\n \n+            if gitConfig(\"git-p4.skipSubmitEditCheck\") == \"true\":\n+                checkModTime = False\n+            else:\n+                checkModTime = True\n+\n             response = \"y\"\n-            if os.stat(fileName).st_mtime <= mtime:\n+            if checkModTime and (os.stat(fileName).st_mtime <= mtime):\n                 response = \"x\"\n                 while response != \"y\" and response != \"n\":\n                     response = raw_input(\"Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) \")\n@@ -795,6 +926,14 @@ class P4Submit(Command):\n                 if self.isWindows:\n                     submitTemplate = submitTemplate.replace(\"\\r\\n\", \"\\n\")\n                 p4_write_pipe(\"submit -i\", submitTemplate)\n+\n+                if self.preserveUser:\n+                    if p4User:\n+                        # Get last changelist number. Cannot easily get it from\n+                        # the submit command output as the output is unmarshalled.\n+                        changelist = self.lastP4Changelist()\n+                        self.modifyChangelistUser(changelist, p4User)\n+\n             else:\n                 for f in editedFiles:\n                     p4_system(\"revert \\\"%s\\\"\" % f);\n@@ -831,6 +970,10 @@ class P4Submit(Command):\n         if len(self.origin) == 0:\n             self.origin = upstream\n \n+        if self.preserveUser:\n+            if not self.canChangeChangelists():\n+                die(\"Cannot preserve user names without p4 super-user or admin permissions\")\n+\n         if self.verbose:\n             print \"Origin branch is \" + self.origin\n \n@@ -858,6 +1001,9 @@ class P4Submit(Command):\n             commits.append(line.strip())\n         commits.reverse()\n \n+        if self.preserveUser:\n+            self.checkValidP4Users(commits)\n+\n         while len(commits) > 0:\n             commit = commits[0]\n             commits = commits[1:]\n@@ -877,11 +1023,12 @@ class P4Submit(Command):\n \n         return True\n \n-class P4Sync(Command):\n+class P4Sync(Command, P4UserMap):\n     delete_actions = ( \"delete\", \"move/delete\", \"purge\" )\n \n     def __init__(self):\n         Command.__init__(self)\n+        P4UserMap.__init__(self)\n         self.options = [\n                 optparse.make_option(\"--branch\", dest=\"branch\"),\n                 optparse.make_option(\"--detect-branches\", dest=\"detectBranches\", action=\"store_true\"),\n@@ -1236,41 +1383,6 @@ class P4Sync(Command):\n                     print (\"Tag %s does not match with change %s: file count is different.\"\n                            % (labelDetails[\"label\"], change))\n \n-    def getUserCacheFilename(self):\n-        home = os.environ.get(\"HOME\", os.environ.get(\"USERPROFILE\"))\n-        return home + \"/.gitp4-usercache.txt\"\n-\n-    def getUserMapFromPerforceServer(self):\n-        if self.userMapFromPerforceServer:\n-            return\n-        self.users = {}\n-\n-        for output in p4CmdList(\"users\"):\n-            if not output.has_key(\"User\"):\n-                continue\n-            self.users[output[\"User\"]] = output[\"FullName\"] + \" <\" + output[\"Email\"] + \">\"\n-\n-\n-        s = ''\n-        for (key, val) in self.users.items():\n-            s += \"%s\\t%s\\n\" % (key.expandtabs(1), val.expandtabs(1))\n-\n-        open(self.getUserCacheFilename(), \"wb\").write(s)\n-        self.userMapFromPerforceServer = True\n-\n-    def loadUserMapFromCache(self):\n-        self.users = {}\n-        self.userMapFromPerforceServer = False\n-        try:\n-            cache = open(self.getUserCacheFilename(), \"rb\")\n-            lines = cache.readlines()\n-            cache.close()\n-            for line in lines:\n-                entry = line.strip().split(\"\\t\")\n-                self.users[entry[0]] = entry[1]\n-        except IOError:\n-            self.getUserMapFromPerforceServer()\n-\n     def getLabels(self):\n         self.labels = {}\n \ndiff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt\nindex e09da44..b6986f0 100644\n--- a/contrib/fast-import/git-p4.txt\n+++ b/contrib/fast-import/git-p4.txt\n@@ -110,6 +110,12 @@ is not your current git branch you can also pass that as an argument:\n \n You can override the reference branch with the --origin=mysourcebranch option.\n \n+The Perforce changelists will be created with the user who ran git-p4. If you\n+use --preserve-user then git-p4 will attempt to create Perforce changelists\n+with the Perforce user corresponding to the git commit author. You need to\n+have sufficient permissions within Perforce, and the git users need to have\n+Perforce accounts. Permissions can be granted using 'p4 protect'.\n+\n If a submit fails you may have to \"p4 resolve\" and submit manually. You can\n continue importing the remaining changes with\n \n@@ -196,6 +202,29 @@ able to find the relevant client.  This client spec will be used to\n both filter the files cloned by git and set the directory layout as\n specified in the client (this implies --keep-path style semantics).\n \n+git-p4.skipSubmitModTimeCheck\n+\n+  git config [--global] git-p4.skipSubmitModTimeCheck false\n+\n+If true, submit will not check if the p4 change template has been modified.\n+\n+git-p4.preserveUser\n+\n+  git config [--global] git-p4.preserveUser false\n+\n+If true, attempt to preserve user names by modifying the p4 changelists. See\n+the \"--preserve-user\" submit option.\n+\n+git-p4.allowMissingPerforceUsers\n+\n+  git config [--global] git-p4.allowMissingP4Users false\n+\n+If git-p4 is setting the perforce user for a commit (--preserve-user) then\n+if there is no perforce user corresponding to the git author, git-p4 will\n+stop. With allowMissingPerforceUsers set to true, git-p4 will use the\n+current user (i.e. the behavior without --preserve-user) and carry on with\n+the perforce commit.\n+\n Implementation Details...\n =========================\n \ndiff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh\nindex a523473..fd3204b 100755\n--- a/t/t9800-git-p4.sh\n+++ b/t/t9800-git-p4.sh\n@@ -12,6 +12,8 @@ test_description='git-p4 tests'\n GITP4=$GIT_BUILD_DIR/contrib/fast-import/git-p4\n P4DPORT=10669\n \n+export P4PORT=localhost:$P4DPORT\n+\n db=\"$TRASH_DIRECTORY/db\"\n cli=\"$TRASH_DIRECTORY/cli\"\n git=\"$TRASH_DIRECTORY/git\"\n@@ -129,6 +131,88 @@ test_expect_success 'clone bare' '\n \trm -rf \"$git\" && mkdir \"$git\"\n '\n \n+p4_add_user() {\n+    name=$1\n+    fullname=$2\n+    p4 user -f -i <<EOF &&\n+User: $name\n+Email: $name@localhost\n+FullName: $fullname\n+EOF\n+    p4 passwd -P secret $name\n+}\n+\n+p4_grant_admin() {\n+    name=$1\n+    p4 protect -o |\\\n+        awk \"{print}END{print \\\"    admin user $name * //depot/...\\\"}\" |\\\n+        p4 protect -i\n+}\n+\n+p4_check_commit_author() {\n+    file=$1\n+    user=$2\n+    if p4 changes -m 1 //depot/$file | grep $user > /dev/null ; then\n+        return 0\n+    else\n+        echo \"file $file not modified by user $user\" 1>&2\n+        return 1\n+    fi\n+}\n+\n+# Test username support, submitting as user 'alice'\n+test_expect_success 'preserve users' '\n+\tp4_add_user alice Alice &&\n+\tp4_add_user bob Bob &&\n+\tp4_grant_admin alice &&\n+\t\"$GITP4\" clone --dest=\"$git\" //depot &&\n+\tcd \"$git\" &&\n+\techo \"username: a change by alice\" >> file1 &&\n+\techo \"username: a change by bob\" >> file2 &&\n+\tgit commit --author \"Alice <alice@localhost>\" -m \"a change by alice\" file1 &&\n+\tgit commit --author \"Bob <bob@localhost>\" -m \"a change by bob\" file2 &&\n+\tgit config git-p4.skipSubmitEditCheck true &&\n+\tP4EDITOR=touch P4USER=alice P4PASSWD=secret \"$GITP4\" commit --preserve-user &&\n+\tp4_check_commit_author file1 alice &&\n+\tp4_check_commit_author file2 bob &&\n+\tcd \"$TRASH_DIRECTORY\" &&\n+\trm -rf \"$git\" && mkdir \"$git\"\n+'\n+\n+# Test username support, submitting as bob, who lacks admin rights. Should\n+# not submit change to p4 (git diff should show deltas).\n+test_expect_success 'refuse to preserve users without perms' '\n+\t\"$GITP4\" clone --dest=\"$git\" //depot &&\n+\tcd \"$git\" &&\n+\techo \"username-noperms: a change by alice\" >> file1 &&\n+\tgit commit --author \"Alice <alice@localhost>\" -m \"perms: a change by alice\" file1 &&\n+\t! P4EDITOR=touch P4USER=bob P4PASSWD=secret \"$GITP4\" commit --preserve-user &&\n+\t! git diff --exit-code HEAD..p4/master > /dev/null &&\n+\tcd \"$TRASH_DIRECTORY\" &&\n+\trm -rf \"$git\" && mkdir \"$git\"\n+'\n+\n+# What happens with unknown author? Without allowMissingP4Users it should fail.\n+test_expect_success 'preserve user where author is unknown to p4' '\n+\t\"$GITP4\" clone --dest=\"$git\" //depot &&\n+\tcd \"$git\" &&\n+\tgit config git-p4.skipSubmitEditCheck true\n+\techo \"username-bob: a change by bob\" >> file1 &&\n+\tgit commit --author \"Bob <bob@localhost>\" -m \"preserve: a change by bob\" file1 &&\n+\techo \"username-unknown: a change by charlie\" >> file1 &&\n+\tgit commit --author \"Charlie <charlie@localhost>\" -m \"preserve: a change by charlie\" file1 &&\n+\t! P4EDITOR=touch P4USER=alice P4PASSWD=secret \"$GITP4\" commit --preserve-user &&\n+\t! git diff --exit-code HEAD..p4/master > /dev/null &&\n+\techo \"$0: repeat with allowMissingP4Users enabled\" &&\n+\tgit config git-p4.allowMissingP4Users true &&\n+\tgit config git-p4.preserveUser true &&\n+\tP4EDITOR=touch P4USER=alice P4PASSWD=secret \"$GITP4\" commit &&\n+\tgit diff --exit-code HEAD..p4/master > /dev/null &&\n+\tp4_check_commit_author file1 alice &&\n+\tcd \"$TRASH_DIRECTORY\" &&\n+\trm -rf \"$git\" && mkdir \"$git\"\n+'\n+\n test_expect_success 'shutdown' '\n \tpid=`pgrep -f p4d` &&\n \ttest -n \"$pid\" &&\n-- \n1.7.1\n"},{"id":"167215","messageId":"7vy62k2z8j.fsf@alter.siamese.dyndns.org","threadId":"27261","inReplyTo":"1304577819-703-1-git-send-email-luke@diamand.org","subject":"Re: [PATCH v3] git-p4: add option to preserve user names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-06T05:07:40Z","receivedAt":"2011-05-06T05:07:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand <luke@diamand.org> writes:\n\n> This is version 3 of my patch.\n\nThe previous one from Apr 21st is already on \"next\" with Ack from Pete.\n"},{"id":"167217","messageId":"4DC38657.7020703@diamand.org","threadId":"27261","inReplyTo":"7vy62k2z8j.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] git-p4: add option to preserve user names","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2011-05-06T05:25:43Z","receivedAt":"2011-05-06T05:25:43Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 06/05/11 06:07, Junio C Hamano wrote:\n> Luke Diamand<luke@diamand.org>  writes:\n>\n>> This is version 3 of my patch.\n>\n> The previous one from Apr 21st is already on \"next\" with Ack from Pete.\n\nAh, sorry.\n\nShould I submit a patch against that then?\n\nThanks!\nLuke\n"},{"id":"167303","messageId":"20110506235912.GA11842@arf.padd.com","threadId":"27261","inReplyTo":"4DC38657.7020703@diamand.org","subject":"Re: [PATCH v3] git-p4: add option to preserve user names","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2011-05-06T23:59:12Z","receivedAt":"2011-05-06T23:59:12Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"luke@diamand.org wrote on Fri, 06 May 2011 06:25 +0100:\n> On 06/05/11 06:07, Junio C Hamano wrote:\n> >Luke Diamand<luke@diamand.org>  writes:\n> >\n> >>This is version 3 of my patch.\n> >\n> >The previous one from Apr 21st is already on \"next\" with Ack from Pete.\n> \n> Ah, sorry.\n> \n> Should I submit a patch against that then?\n\nYes, your changes look good and fix bugs.\n\nTo the diff from v2 to v3:\n\nAcked-by: Pete Wyckoff <pw@padd.com>\n\n\n\t\t-- Pete\n"},{"id":"167339","messageId":"7viptmup53.fsf@alter.siamese.dyndns.org","threadId":"27261","inReplyTo":"20110506235912.GA11842@arf.padd.com","subject":"Re: [PATCH v3] git-p4: add option to preserve user names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-07T22:22:48Z","receivedAt":"2011-05-07T22:22:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pete Wyckoff <pw@padd.com> writes:\n\n> luke@diamand.org wrote on Fri, 06 May 2011 06:25 +0100:\n>> On 06/05/11 06:07, Junio C Hamano wrote:\n>> >Luke Diamand<luke@diamand.org>  writes:\n>> >\n>> >>This is version 3 of my patch.\n>> >\n>> >The previous one from Apr 21st is already on \"next\" with Ack from Pete.\n>> \n>> Ah, sorry.\n>> \n>> Should I submit a patch against that then?\n>\n> Yes, your changes look good and fix bugs.\n>\n> To the diff from v2 to v3:\n>\n> Acked-by: Pete Wyckoff <pw@padd.com>\n\nSo the only thing lacking at this point is the commit log message?\n\nI am not sure if the \"actual user is luke\" message you give when (and only\nwhen) preserveUser is used is a good \"reminder\".  Isn't it that the user\nneeds reminder when the user should have used but forgot to use this\noption, not the other way around like your patch does?\n\nI suspect that the message would show an unexpected name only when the new\ncodepath is buggy or the P4 changes the code is interacting are formatted\nin ways that the new codepath is not expecting (well, they amount to the\nsame thing after all, no?), and having such a message may prevent users\nfrom submitting the changeset under an incorrect name, but at that point\nwhat recourse do they have?\n\nIt looks to me that the message is not helping the users, even though it\nmay help as a debugging aid for git-p4 developers.\n\n-- >8 --\nSubject: git-p4 --preserve-user: finishing touches\n\nThe earlier round unnecessarily updated the user field that is already\ncorrect.\n\nAdd a message with the P4 user name used in the submit template to\nremind the user when this option is given.\n\nForm the change spec string correctly, without relying on the way\nPython happens to order the dictionary contents.\n\nSigned-off-by: Luke Diamand <luke@diamand.org>\nAcked-by: Pete Wyckoff <pw@padd.com>\n---\n contrib/fast-import/git-p4 |   17 +++++++++++++----\n 1 files changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 36e3d87..6018507 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -688,12 +688,18 @@ class P4Submit(Command, P4UserMap):\n         die(\"Could not get changelist number for last submit - cannot patch up user details\")\n \n     def modifyChangelistUser(self, changelist, newUser):\n-        # fixup the user field of a changelist after it has been submitted.\n+        # fixup the user field of a single changelist after it has been submitted.\n         changes = p4CmdList(\"change -o %s\" % changelist)\n+        input = ''\n         for c in changes:\n             if c.has_key('User'):\n+                if c['User'] == newUser:\n+                    # nothing to do here\n+                    return\n                 c['User'] = newUser\n-        input = marshal.dumps(changes[0])\n+\n+            input = input + marshal.dumps(c)\n+\n         result = p4CmdList(\"change -f -i\", stdin=input)\n         for r in result:\n             if r.has_key('code'):\n@@ -703,11 +709,10 @@ class P4Submit(Command, P4UserMap):\n                 print(\"Updated user field for changelist %s to %s\" % (changelist, newUser))\n                 return\n         die(\"Could not modify user field of changelist %s to %s\" % (changelist, newUser))\n-\n     def canChangeChangelists(self):\n         # check to see if we have p4 admin or super-user permissions, either of\n         # which are required to modify changelists.\n-        results = p4CmdList(\"-G protects %s\" % self.depotPath)\n+        results = p4CmdList(\"protects %s\" % self.depotPath)\n         for r in results:\n             if r.has_key('perm'):\n                 if r['perm'] == 'admin':\n@@ -865,6 +870,10 @@ class P4Submit(Command, P4UserMap):\n \n         if self.interactive:\n             submitTemplate = self.prepareLogMessage(template, logMessage)\n+\n+            if self.preserveUser:\n+               submitTemplate = submitTemplate + (\"\\n######## Actual user %s, modified after commit\\n\" % p4User)\n+\n             if os.environ.has_key(\"P4DIFF\"):\n                 del(os.environ[\"P4DIFF\"])\n             diff = \"\"\n"},{"id":"167361","messageId":"4DC67756.3090002@diamand.org","threadId":"27261","inReplyTo":"7viptmup53.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] git-p4: add option to preserve user names","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2011-05-08T10:58:30Z","receivedAt":"2011-05-08T10:58:30Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 07/05/11 23:22, Junio C Hamano wrote:\n> Pete Wyckoff<pw@padd.com>  writes:\n>\n>> luke@diamand.org wrote on Fri, 06 May 2011 06:25 +0100:\n>>> On 06/05/11 06:07, Junio C Hamano wrote:\n>>>> Luke Diamand<luke@diamand.org>   writes:\n>>>>\n>>>>> This is version 3 of my patch.\n>>>>\n\n<snip>\n\n>\n> So the only thing lacking at this point is the commit log message?\n>\n> I am not sure if the \"actual user is luke\" message you give when (and only\n> when) preserveUser is used is a good \"reminder\".  Isn't it that the user\n> needs reminder when the user should have used but forgot to use this\n> option, not the other way around like your patch does?\n\nI put that in so that when I'm at the point where I'm about to submit to \nPerforce I know that git-p4 hasn't forgotten that it's going to patch up \nthe user name, and hasn't got it horribly wrong.\n\ni.e. to reassure me it's not about to mess up Perforce.\n\n>\n> I suspect that the message would show an unexpected name only when the new\n> codepath is buggy or the P4 changes the code is interacting are formatted\n> in ways that the new codepath is not expecting (well, they amount to the\n> same thing after all, no?),\n\nExactly.\n\n(The submit template does have a userid field in it, but this is always \n*your* userid, which I thought might be a bit confusing. Hence the message).\n\n> and having such a message may prevent users\n> from submitting the changeset under an incorrect name, but at that point\n> what recourse do they have?\n\nApart from not submitting the changelist, none.\n>\n> It looks to me that the message is not helping the users, even though it\n> may help as a debugging aid for git-p4 developers.\n\nShould I just remove it?\n\nI guess it only adds a small amount of information which could be \nexplained in the instructions.\n\nRegards!\nLuke\n"},{"id":"167433","messageId":"7voc3dt7wq.fsf@alter.siamese.dyndns.org","threadId":"27261","inReplyTo":"4DC67756.3090002@diamand.org","subject":"Re: [PATCH v3] git-p4: add option to preserve user names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-08T17:32:37Z","receivedAt":"2011-05-08T17:32:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand <luke@diamand.org> writes:\n\n>> It looks to me that the message is not helping the users, even though it\n>> may help as a debugging aid for git-p4 developers.\n>\n> Should I just remove it?\n>\n> I guess it only adds a small amount of information which could be\n> explained in the instructions.\n\nI was trying to get a feel of how much thinking went behind that message,\nby suggesting a possible improvement to help users who forgot to pass the\nnew option when they might have wanted to, instead of just assuring users\nwho did pass the option when the command did the right thing for them.\n\nPeople learn to quickly ignore repeated and regular messages. They will\nlearn that they will get that message whenever they pass the new option,\nand they learn that most of the time it says what they wanted, and will\neasily miss when the username you put in the message is different from\nwhat they expect.\n\nIn our commit template, we say \"# Author: author name\" when and only when\nit is different from you. Most of the time, you are committing your own\ncommit, so this line is _unusual_ and that is very much deliberate.\n\nIf you do not think of a good way to improve the \"help\" part of the patch,\nthat is Ok. We are still making progress by giving a functionality that\nhas been missing.\n\nTo remove or to keep, I am less qualified to judge than either you or\nPete. I'm not the primary audience. If you think it helps users, leave it\nin. Otherwise remove.\n\nThanks.\n"},{"id":"167447","messageId":"4DC6FE90.6040406@diamand.org","threadId":"27261","inReplyTo":"7voc3dt7wq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] git-p4: add option to preserve user names","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2011-05-08T20:35:28Z","receivedAt":"2011-05-08T20:35:28Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 08/05/11 18:32, Junio C Hamano wrote:\n> Luke Diamand<luke@diamand.org>  writes:\n>\n>>> It looks to me that the message is not helping the users, even though it\n>>> may help as a debugging aid for git-p4 developers.\n>>\n>> Should I just remove it?\n>>\n>> I guess it only adds a small amount of information which could be\n>> explained in the instructions.\n>\n> I was trying to get a feel of how much thinking went behind that message,\n> by suggesting a possible improvement to help users who forgot to pass the\n> new option when they might have wanted to, instead of just assuring users\n> who did pass the option when the command did the right thing for them.\n>\n> People learn to quickly ignore repeated and regular messages. They will\n> learn that they will get that message whenever they pass the new option,\n> and they learn that most of the time it says what they wanted, and will\n> easily miss when the username you put in the message is different from\n> what they expect.\n>\n> In our commit template, we say \"# Author: author name\" when and only when\n> it is different from you. Most of the time, you are committing your own\n> commit, so this line is _unusual_ and that is very much deliberate.\n>\n> If you do not think of a good way to improve the \"help\" part of the patch,\n> that is Ok. We are still making progress by giving a functionality that\n> has been missing.\n>\n> To remove or to keep, I am less qualified to judge than either you or\n> Pete. I'm not the primary audience. If you think it helps users, leave it\n> in. Otherwise remove.\n\nPer your suggestion, I'm working on a pair of patches that are:\n\n(a) the previous patch (\"minor improvements\"), but without the warning. \ngit-p4.txt is updated to make it clearer what to expect.\n\n(b) a second patch that puts a warning in the commit template if you are \ngoing to lose authorship information. The warning is disableable  via \ngit-config.\n\nRegards!\nLuke\n"}]}