{"thread":{"id":"27303","subject":"[PATCH/RFC v2 0/2] git-p4: user preservation improvements","startedAt":"2011-05-09T06:40:01Z","lastAt":"2011-05-09T22:39:28Z","messageCount":5,"participants":["Luke Diamand","Pete Wyckoff"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"167463","messageId":"1304923203-26278-1-git-send-email-luke@diamand.org","threadId":"27303","inReplyTo":null,"subject":"[PATCH/RFC v2 0/2] git-p4: user preservation improvements","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2011-05-09T06:40:01Z","receivedAt":"2011-05-09T06:40:01Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Junio suggested that git-p4 should warn people about username\nlossage in a more sensible way. This pair of changes is\nan attempt to do that.\n\n(a) The first change is a few small improvements to git-p4, but without\nspurious warnings (a bit more paranoid, get rid of a superfluous -G\nand don't bother to change the user if it's already correct).\n\n(b) The second change adds a warning to the commit message if\nthe authorship (based on git email vs p4 email) is going to be lost and the\nuser is *not* using --preserve-user. The warning can be suppressed with a git\nconfig option.\n\nLuke Diamand (2):\n  git-p4: small improvements to user-preservation\n  git-p4: warn if git authorship won't be retained\n\n contrib/fast-import/git-p4     |   49 ++++++++++++++++++++++++++++++++++-----\n contrib/fast-import/git-p4.txt |   16 +++++++++---\n t/t9800-git-p4.sh              |   46 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 100 insertions(+), 11 deletions(-)\n"},{"id":"167464","messageId":"1304923203-26278-2-git-send-email-luke@diamand.org","threadId":"27303","inReplyTo":"1304923203-26278-1-git-send-email-luke@diamand.org","subject":"[PATCH/RFC v2 1/2] git-p4: small improvements to user-preservation","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2011-05-09T06:40:02Z","receivedAt":"2011-05-09T06:40:02Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":". Slightly more paranoid checking of results from 'p4 change'\n. Remove superfluous \"-G\"\n. Don't modify the username if it is unchanged.\n. Reword git-p4.txt to point out that the initial commit will\n  be submitted with you as the author.\n\nSigned-off-by: Luke Diamand <luke@diamand.org>\n---\n contrib/fast-import/git-p4     |   16 +++++++++++-----\n contrib/fast-import/git-p4.txt |    9 +++++----\n 2 files changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 36e3d87..bd8c761 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -690,10 +690,15 @@ class P4Submit(Command, P4UserMap):\n     def modifyChangelistUser(self, changelist, newUser):\n         # fixup the user field of a changelist after it has been submitted.\n         changes = p4CmdList(\"change -o %s\" % changelist)\n-        for c in changes:\n-            if c.has_key('User'):\n-                c['User'] = newUser\n-        input = marshal.dumps(changes[0])\n+        if len(changes) != 1:\n+            die(\"Bad output from p4 change modifying %s to user %s\" %\n+                (changelist, newUser))\n+\n+        c = changes[0]\n+        if c['User'] == newUser: return   # nothing to do\n+        c['User'] = newUser\n+        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@@ -707,7 +712,7 @@ class P4Submit(Command, P4UserMap):\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,7 @@ class P4Submit(Command, P4UserMap):\n \n         if self.interactive:\n             submitTemplate = self.prepareLogMessage(template, logMessage)\n+\n             if os.environ.has_key(\"P4DIFF\"):\n                 del(os.environ[\"P4DIFF\"])\n             diff = \"\"\ndiff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt\nindex b6986f0..d46b7a5 100644\n--- a/contrib/fast-import/git-p4.txt\n+++ b/contrib/fast-import/git-p4.txt\n@@ -111,10 +111,11 @@ is not your current git branch you can also pass that as an argument:\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+use --preserve-user then git-p4 will modify the Perforce change's user field\n+to match that of the git commit author after it has been submitted (the change\n+will still initially be submitted with you as the author). You need to have\n+sufficient permissions within Perforce, and the git users need to have Perforce\n+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-- \n1.7.1\n"},{"id":"167465","messageId":"1304923203-26278-3-git-send-email-luke@diamand.org","threadId":"27303","inReplyTo":"1304923203-26278-2-git-send-email-luke@diamand.org","subject":"[PATCH/RFC v2 2/2] git-p4: warn if git authorship won't be retained","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2011-05-09T06:40:03Z","receivedAt":"2011-05-09T06:40:03Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"If the git commits you are submitting contain changes made by\nother people, the authorship will not be retained. Change git-p4\nto warn of this and to note that --preserve-user can be used\nto solve the problem (if you have suitable permissions).\nThe warning can be disabled.\n\nAdd a test case and update documentation.\n\nSigned-off-by: Luke Diamand <luke@diamand.org>\n---\n contrib/fast-import/git-p4     |   33 +++++++++++++++++++++++++++-\n contrib/fast-import/git-p4.txt |    7 ++++++\n t/t9800-git-p4.sh              |   46 ++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 84 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex bd8c761..5db5850 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -614,6 +614,7 @@ class P4Submit(Command, P4UserMap):\n         self.verbose = False\n         self.preserveUser = gitConfig(\"git-p4.preserveUser\").lower() == \"true\"\n         self.isWindows = (platform.system() == \"Windows\")\n+        self.myP4UserId = None\n \n     def check(self):\n         if len(p4CmdList(\"opened ...\")) > 0:\n@@ -721,6 +722,25 @@ class P4Submit(Command, P4UserMap):\n                     return 1\n         return 0\n \n+    def p4UserId(self):\n+        if self.myP4UserId:\n+            return self.myP4UserId\n+\n+        results = p4CmdList(\"user -o\")\n+        for r in results:\n+            if r.has_key('User'):\n+                self.myP4UserId = r['User']\n+                return r['User']\n+        die(\"Could not find your p4 user id\")\n+\n+    def p4UserIsMe(self, p4User):\n+        # return True if the given p4 user is actually me\n+        me = self.p4UserId()\n+        if not p4User or p4User != me:\n+            return False\n+        else:\n+            return True\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@@ -750,8 +770,7 @@ class P4Submit(Command, P4UserMap):\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+        (p4User, gitEmail) = self.p4UserForCommit(id)\n \n         if not self.detectRenames:\n             # If not explicitly set check the config variable\n@@ -887,6 +906,11 @@ class P4Submit(Command, P4UserMap):\n                     newdiff += \"+\" + line\n                 f.close()\n \n+            if self.checkAuthorship and not self.p4UserIsMe(p4User):\n+                submitTemplate += \"######## git author %s does not match your p4 account.\\n\" % gitEmail\n+                submitTemplate += \"######## Use git-p4 option --preserve-user to modify authorship\\n\"\n+                submitTemplate += \"######## Use git-p4 config git-p4.skipUserNameCheck hides this messsage.\\n\"\n+\n             separatorLine = \"######## everything below this line is just the diff #######\\n\"\n \n             [handle, fileName] = tempfile.mkstemp()\n@@ -998,6 +1022,11 @@ class P4Submit(Command, P4UserMap):\n             commits.append(line.strip())\n         commits.reverse()\n \n+        if not self.preserveUser and (gitConfig(\"git-p4.skipUserNameCheck\") != \"true\"):\n+            self.checkAuthorship = True\n+        else:\n+            self.checkAuthorship = False\n+\n         if self.preserveUser:\n             self.checkValidP4Users(commits)\n \ndiff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt\nindex d46b7a5..3c7c138 100644\n--- a/contrib/fast-import/git-p4.txt\n+++ b/contrib/fast-import/git-p4.txt\n@@ -226,6 +226,13 @@ 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+git-p4.skipUserNameCheck\n+\n+  git config [--global] git-p4.skipUserNameCheck false\n+\n+When submitting, git-p4 checks that the git commits are authored by the current\n+p4 user, and warns if they are not. This disables the check.\n+\n Implementation Details...\n =========================\n \ndiff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh\nindex 4fb0e24..0dcaa9c 100755\n--- a/t/t9800-git-p4.sh\n+++ b/t/t9800-git-p4.sh\n@@ -160,6 +160,15 @@ p4_check_commit_author() {\n     fi\n }\n \n+make_change_by_user() {\n+    file=$1\n+    name=$2\n+    email=$3\n+\techo \"username: a change by $name\" >> $file &&\n+\tgit add $file &&\n+\tgit commit --author \"$name <$email>\" -m \"a change by $name\"\n+}\n+\n # Test username support, submitting as user 'alice'\n test_expect_success 'preserve users' '\n \tp4_add_user alice Alice &&\n@@ -213,6 +222,43 @@ test_expect_success 'preserve user where author is unknown to p4' '\n \trm -rf \"$git\" && mkdir \"$git\"\n '\n \n+# If we're *not* using --preserve-user, git-p4 should warn if we're submitting\n+# changes that are not all ours.\n+# Test: user in p4 and user unknown to p4.\n+# Test: warning disabled and user is the same.\n+test_expect_success 'not preserving user with mixed authorship' '\n+\t\"$GITP4\" clone --dest=\"$git\" //depot &&\n+\tcd \"$git\" &&\n+\tgit config git-p4.skipSubmitEditCheck true\n+\tp4_add_user derek Derek &&\n+\n+\tmake_change_by_user usernamefile3 Derek derek@localhost &&\n+\t(P4EDITOR=cat P4USER=alice P4PASSWD=secret \"$GITP4\" commit |\n+\t\tgrep \"git author derek@localhost does not match\") &&\n+\tgit diff --exit-code HEAD..p4/master &&\n+\n+\tmake_change_by_user usernamefile3 Charlie charlie@localhost &&\n+\t(P4EDITOR=cat P4USER=alice P4PASSWD=secret \"$GITP4\" commit |\n+\t\tgrep \"git author charlie@localhost does not match\") &&\n+\tgit diff --exit-code HEAD..p4/master &&\n+\n+\tmake_change_by_user usernamefile3 alice alice@localhost &&\n+\t!(P4EDITOR=cat P4USER=alice P4PASSWD=secret \"$GITP4\" commit |\n+\t\tgrep \"does not match\") &&\n+\tgit diff --exit-code HEAD..p4/master &&\n+\n+\tgit config git-p4.skipUserNameCheck true &&\n+\tmake_change_by_user usernamefile3 Charlie charlie@localhost &&\n+\t!(P4EDITOR=cat P4USER=alice P4PASSWD=secret \"$GITP4\" commit |\n+\t\tgrep \"git author charlie@localhost does not match\") &&\n+\tgit diff --exit-code HEAD..p4/master &&\n+\n+\tp4_check_commit_author usernamefile3 alice &&\n+\tcd \"$TRASH_DIRECTORY\" &&\n+\trm -rf \"$git\" && mkdir \"$git\"\n+'\n+\n+\n test_expect_success 'shutdown' '\n \tpid=`pgrep -f p4d` &&\n \ttest -n \"$pid\" &&\n-- \n1.7.1\n"},{"id":"167543","messageId":"20110509223048.GA1716@arf.padd.com","threadId":"27303","inReplyTo":"1304923203-26278-2-git-send-email-luke@diamand.org","subject":"Re: [PATCH/RFC v2 1/2] git-p4: small improvements to user-preservation","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2011-05-09T22:30:48Z","receivedAt":"2011-05-09T22:30:48Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"luke@diamand.org wrote on Mon, 09 May 2011 07:40 +0100:\n> . Slightly more paranoid checking of results from 'p4 change'\n> . Remove superfluous \"-G\"\n> . Don't modify the username if it is unchanged.\n> . Reword git-p4.txt to point out that the initial commit will\n>   be submitted with you as the author.\n> \n> Signed-off-by: Luke Diamand <luke@diamand.org>\n\nAcked-By: Pete Wyckoff <pw@padd.com>\n"},{"id":"167544","messageId":"20110509223928.GB1716@arf.padd.com","threadId":"27303","inReplyTo":"1304923203-26278-3-git-send-email-luke@diamand.org","subject":"Re: [PATCH/RFC v2 2/2] git-p4: warn if git authorship won't be retained","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2011-05-09T22:39:28Z","receivedAt":"2011-05-09T22:39:28Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"luke@diamand.org wrote on Mon, 09 May 2011 07:40 +0100:\n> If the git commits you are submitting contain changes made by\n> other people, the authorship will not be retained. Change git-p4\n> to warn of this and to note that --preserve-user can be used\n> to solve the problem (if you have suitable permissions).\n> The warning can be disabled.\n> \n> Add a test case and update documentation.\n> \n> Signed-off-by: Luke Diamand <luke@diamand.org>\n\nThis addresses Junio's concerns and makes more sense to me too.\nIt is odd for someone to submit commits not their own, but if\nthey want to, at least the warning can be hushed.\n\nTwo teensy fixes:\n\n> +            if self.checkAuthorship and not self.p4UserIsMe(p4User):\n> +                submitTemplate += \"######## git author %s does not match your p4 account.\\n\" % gitEmail\n> +                submitTemplate += \"######## Use git-p4 option --preserve-user to modify authorship\\n\"\n> +                submitTemplate += \"######## Use git-p4 config git-p4.skipUserNameCheck hides this messsage.\\n\"\n> +\n\ns/messsage/message/\n\n\n>              separatorLine = \"######## everything below this line is just the diff #######\\n\"\n>  \n>              [handle, fileName] = tempfile.mkstemp()\n> @@ -998,6 +1022,11 @@ class P4Submit(Command, P4UserMap):\n>              commits.append(line.strip())\n>          commits.reverse()\n>  \n> +        if not self.preserveUser and (gitConfig(\"git-p4.skipUserNameCheck\") != \"true\"):\n> +            self.checkAuthorship = True\n> +        else:\n> +            self.checkAuthorship = False\n> +\n\nCan we have a bit clearer logic?  I get confused with too many\nnegatives.\n\n\t#\n\t# Issue a warning in the submission template if the\n\t# author of the patch is different from the submitter.\n\t# But if preserveUser is used, the authorship will be\n\t# adjusted, hence no warning.  Also this message can be\n\t# hushed with a config option.\n\t#\n\tself.checkAuthorship = True\n\tif self.preserveUser:\n\t    self.checkAuthorship = False\n\tif gitConfig(\"git-p4.skipUserNameCheck\") != \"true\":\n\t    self.checkAuthorship = False\n\n> +make_change_by_user() {\n> +    file=$1\n> +    name=$2\n> +    email=$3\n> +\techo \"username: a change by $name\" >> $file &&\n> +\tgit add $file &&\n> +\tgit commit --author \"$name <$email>\" -m \"a change by $name\"\n> +}\n\nTab/space oddities.\n\n> +# If we're *not* using --preserve-user, git-p4 should warn if we're submitting\n> +# changes that are not all ours.\n> +# Test: user in p4 and user unknown to p4.\n> +# Test: warning disabled and user is the same.\n> +test_expect_success 'not preserving user with mixed authorship' '\n> +\t\"$GITP4\" clone --dest=\"$git\" //depot &&\n> +\tcd \"$git\" &&\n> +\tgit config git-p4.skipSubmitEditCheck true\n\nMissing &&.  These are hard to remember, and I've been corrected\nbefore too.\n\n> +\tp4_add_user derek Derek &&\n> +\n> +\tmake_change_by_user usernamefile3 Derek derek@localhost &&\n> +\t(P4EDITOR=cat P4USER=alice P4PASSWD=secret \"$GITP4\" commit |\n> +\t\tgrep \"git author derek@localhost does not match\") &&\n> +\tgit diff --exit-code HEAD..p4/master &&\n> +\n> +\tmake_change_by_user usernamefile3 Charlie charlie@localhost &&\n> +\t(P4EDITOR=cat P4USER=alice P4PASSWD=secret \"$GITP4\" commit |\n> +\t\tgrep \"git author charlie@localhost does not match\") &&\n> +\tgit diff --exit-code HEAD..p4/master &&\n> +\n> +\tmake_change_by_user usernamefile3 alice alice@localhost &&\n> +\t!(P4EDITOR=cat P4USER=alice P4PASSWD=secret \"$GITP4\" commit |\n> +\t\tgrep \"does not match\") &&\n> +\tgit diff --exit-code HEAD..p4/master &&\n> +\n> +\tgit config git-p4.skipUserNameCheck true &&\n> +\tmake_change_by_user usernamefile3 Charlie charlie@localhost &&\n> +\t!(P4EDITOR=cat P4USER=alice P4PASSWD=secret \"$GITP4\" commit |\n> +\t\tgrep \"git author charlie@localhost does not match\") &&\n> +\tgit diff --exit-code HEAD..p4/master &&\n\nMaybe -q on the greps.  Not a big deal.\n\n> +\tp4_check_commit_author usernamefile3 alice &&\n> +\tcd \"$TRASH_DIRECTORY\" &&\n> +\trm -rf \"$git\" && mkdir \"$git\"\n> +'\n\nIf you're okay on these fixes, go ahead and add my Acked-By for\nthe resend to Junio.\n\n\t\t-- Pete\n"}]}