[PATCH/RFC v2 0/2] git-p4: user preservation improvements

[PATCH/RFC v2 0/2] git-p4: user preservation improvements

[Luke Diamand]

Junio suggested that git-p4 should warn people about username
lossage in a more sensible way. This pair of changes is
an attempt to do that.

(a) The first change is a few small improvements to git-p4, but without
spurious warnings (a bit more paranoid, get rid of a superfluous -G
and don't bother to change the user if it's already correct).

(b) The second change adds a warning to the commit message if
the authorship (based on git email vs p4 email) is going to be lost and the
user is *not* using --preserve-user. The warning can be suppressed with a git
config option.

Luke Diamand (2):
  git-p4: small improvements to user-preservation
  git-p4: warn if git authorship won't be retained

 contrib/fast-import/git-p4     |   49 ++++++++++++++++++++++++++++++++++-----
 contrib/fast-import/git-p4.txt |   16 +++++++++---
 t/t9800-git-p4.sh              |   46 +++++++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+), 11 deletions(-)

[PATCH/RFC v2 1/2] git-p4: small improvements to user-preservation

[Luke Diamand]

. Slightly more paranoid checking of results from 'p4 change'
. Remove superfluous "-G"
. Don't modify the username if it is unchanged.
. Reword git-p4.txt to point out that the initial commit will
  be submitted with you as the author.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 contrib/fast-import/git-p4     |   16 +++++++++++-----
 contrib/fast-import/git-p4.txt |    9 +++++----
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 36e3d87..bd8c761 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -690,10 +690,15 @@ class P4Submit(Command, P4UserMap):
     def modifyChangelistUser(self, changelist, newUser):
         # fixup the user field of a changelist after it has been submitted.
         changes = p4CmdList("change -o %s" % changelist)
-        for c in changes:
-            if c.has_key('User'):
-                c['User'] = newUser
-        input = marshal.dumps(changes[0])
+        if len(changes) != 1:
+            die("Bad output from p4 change modifying %s to user %s" %
+                (changelist, newUser))
+
+        c = changes[0]
+        if c['User'] == newUser: return   # nothing to do
+        c['User'] = newUser
+        input = marshal.dumps(c)
+
         result = p4CmdList("change -f -i", stdin=input)
         for r in result:
             if r.has_key('code'):
@@ -707,7 +712,7 @@ class P4Submit(Command, P4UserMap):
     def canChangeChangelists(self):
         # check to see if we have p4 admin or super-user permissions, either of
         # which are required to modify changelists.
-        results = p4CmdList("-G protects %s" % self.depotPath)
+        results = p4CmdList("protects %s" % self.depotPath)
         for r in results:
             if r.has_key('perm'):
                 if r['perm'] == 'admin':
@@ -865,6 +870,7 @@ class P4Submit(Command, P4UserMap):
 
         if self.interactive:
             submitTemplate = self.prepareLogMessage(template, logMessage)
+
             if os.environ.has_key("P4DIFF"):
                 del(os.environ["P4DIFF"])
             diff = ""
diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt
index b6986f0..d46b7a5 100644
--- a/contrib/fast-import/git-p4.txt
+++ b/contrib/fast-import/git-p4.txt
@@ -111,10 +111,11 @@ is not your current git branch you can also pass that as an argument:
 You can override the reference branch with the --origin=mysourcebranch option.
 
 The Perforce changelists will be created with the user who ran git-p4. If you
-use --preserve-user then git-p4 will attempt to create Perforce changelists
-with the Perforce user corresponding to the git commit author. You need to
-have sufficient permissions within Perforce, and the git users need to have
-Perforce accounts. Permissions can be granted using 'p4 protect'.
+use --preserve-user then git-p4 will modify the Perforce change's user field
+to match that of the git commit author after it has been submitted (the change
+will still initially be submitted with you as the author). You need to have
+sufficient permissions within Perforce, and the git users need to have Perforce
+accounts. Permissions can be granted using 'p4 protect'.
 
 If a submit fails you may have to "p4 resolve" and submit manually. You can
 continue importing the remaining changes with
-- 
1.7.1

[PATCH/RFC v2 2/2] git-p4: warn if git authorship won't be retained

[Luke Diamand]

If the git commits you are submitting contain changes made by
other people, the authorship will not be retained. Change git-p4
to warn of this and to note that --preserve-user can be used
to solve the problem (if you have suitable permissions).
The warning can be disabled.

Add a test case and update documentation.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 contrib/fast-import/git-p4     |   33 +++++++++++++++++++++++++++-
 contrib/fast-import/git-p4.txt |    7 ++++++
 t/t9800-git-p4.sh              |   46 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 84 insertions(+), 2 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index bd8c761..5db5850 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -614,6 +614,7 @@ class P4Submit(Command, P4UserMap):
         self.verbose = False
         self.preserveUser = gitConfig("git-p4.preserveUser").lower() == "true"
         self.isWindows = (platform.system() == "Windows")
+        self.myP4UserId = None
 
     def check(self):
         if len(p4CmdList("opened ...")) > 0:
@@ -721,6 +722,25 @@ class P4Submit(Command, P4UserMap):
                     return 1
         return 0
 
+    def p4UserId(self):
+        if self.myP4UserId:
+            return self.myP4UserId
+
+        results = p4CmdList("user -o")
+        for r in results:
+            if r.has_key('User'):
+                self.myP4UserId = r['User']
+                return r['User']
+        die("Could not find your p4 user id")
+
+    def p4UserIsMe(self, p4User):
+        # return True if the given p4 user is actually me
+        me = self.p4UserId()
+        if not p4User or p4User != me:
+            return False
+        else:
+            return True
+
     def prepareSubmitTemplate(self):
         # remove lines in the Files section that show changes to files outside the depot path we're committing into
         template = ""
@@ -750,8 +770,7 @@ class P4Submit(Command, P4UserMap):
     def applyCommit(self, id):
         print "Applying %s" % (read_pipe("git log --max-count=1 --pretty=oneline %s" % id))
 
-        if self.preserveUser:
-            (p4User, gitEmail) = self.p4UserForCommit(id)
+        (p4User, gitEmail) = self.p4UserForCommit(id)
 
         if not self.detectRenames:
             # If not explicitly set check the config variable
@@ -887,6 +906,11 @@ class P4Submit(Command, P4UserMap):
                     newdiff += "+" + line
                 f.close()
 
+            if self.checkAuthorship and not self.p4UserIsMe(p4User):
+                submitTemplate += "######## git author %s does not match your p4 account.\n" % gitEmail
+                submitTemplate += "######## Use git-p4 option --preserve-user to modify authorship\n"
+                submitTemplate += "######## Use git-p4 config git-p4.skipUserNameCheck hides this messsage.\n"
+
             separatorLine = "######## everything below this line is just the diff #######\n"
 
             [handle, fileName] = tempfile.mkstemp()
@@ -998,6 +1022,11 @@ class P4Submit(Command, P4UserMap):
             commits.append(line.strip())
         commits.reverse()
 
+        if not self.preserveUser and (gitConfig("git-p4.skipUserNameCheck") != "true"):
+            self.checkAuthorship = True
+        else:
+            self.checkAuthorship = False
+
         if self.preserveUser:
             self.checkValidP4Users(commits)
 
diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt
index d46b7a5..3c7c138 100644
--- a/contrib/fast-import/git-p4.txt
+++ b/contrib/fast-import/git-p4.txt
@@ -226,6 +226,13 @@ stop. With allowMissingPerforceUsers set to true, git-p4 will use the
 current user (i.e. the behavior without --preserve-user) and carry on with
 the perforce commit.
 
+git-p4.skipUserNameCheck
+
+  git config [--global] git-p4.skipUserNameCheck false
+
+When submitting, git-p4 checks that the git commits are authored by the current
+p4 user, and warns if they are not. This disables the check.
+
 Implementation Details...
 =========================
 
diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index 4fb0e24..0dcaa9c 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -160,6 +160,15 @@ p4_check_commit_author() {
     fi
 }
 
+make_change_by_user() {
+    file=$1
+    name=$2
+    email=$3
+	echo "username: a change by $name" >> $file &&
+	git add $file &&
+	git commit --author "$name <$email>" -m "a change by $name"
+}
+
 # Test username support, submitting as user 'alice'
 test_expect_success 'preserve users' '
 	p4_add_user alice Alice &&
@@ -213,6 +222,43 @@ test_expect_success 'preserve user where author is unknown to p4' '
 	rm -rf "$git" && mkdir "$git"
 '
 
+# If we're *not* using --preserve-user, git-p4 should warn if we're submitting
+# changes that are not all ours.
+# Test: user in p4 and user unknown to p4.
+# Test: warning disabled and user is the same.
+test_expect_success 'not preserving user with mixed authorship' '
+	"$GITP4" clone --dest="$git" //depot &&
+	cd "$git" &&
+	git config git-p4.skipSubmitEditCheck true
+	p4_add_user derek Derek &&
+
+	make_change_by_user usernamefile3 Derek derek@localhost &&
+	(P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |
+		grep "git author derek@localhost does not match") &&
+	git diff --exit-code HEAD..p4/master &&
+
+	make_change_by_user usernamefile3 Charlie charlie@localhost &&
+	(P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |
+		grep "git author charlie@localhost does not match") &&
+	git diff --exit-code HEAD..p4/master &&
+
+	make_change_by_user usernamefile3 alice alice@localhost &&
+	!(P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |
+		grep "does not match") &&
+	git diff --exit-code HEAD..p4/master &&
+
+	git config git-p4.skipUserNameCheck true &&
+	make_change_by_user usernamefile3 Charlie charlie@localhost &&
+	!(P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |
+		grep "git author charlie@localhost does not match") &&
+	git diff --exit-code HEAD..p4/master &&
+
+	p4_check_commit_author usernamefile3 alice &&
+	cd "$TRASH_DIRECTORY" &&
+	rm -rf "$git" && mkdir "$git"
+'
+
+
 test_expect_success 'shutdown' '
 	pid=`pgrep -f p4d` &&
 	test -n "$pid" &&
-- 
1.7.1

Re: [PATCH/RFC v2 1/2] git-p4: small improvements to user-preservation

[Pete Wyckoff]

luke@diamand.org wrote on Mon, 09 May 2011 07:40 +0100:
> . Slightly more paranoid checking of results from 'p4 change'
> . Remove superfluous "-G"
> . Don't modify the username if it is unchanged.
> . Reword git-p4.txt to point out that the initial commit will
>   be submitted with you as the author.
> 
> Signed-off-by: Luke Diamand <luke@diamand.org>

Acked-By: Pete Wyckoff <pw@padd.com>

Re: [PATCH/RFC v2 2/2] git-p4: warn if git authorship won't be retained

[Pete Wyckoff]

luke@diamand.org wrote on Mon, 09 May 2011 07:40 +0100:
> If the git commits you are submitting contain changes made by
> other people, the authorship will not be retained. Change git-p4
> to warn of this and to note that --preserve-user can be used
> to solve the problem (if you have suitable permissions).
> The warning can be disabled.
> 
> Add a test case and update documentation.
> 
> Signed-off-by: Luke Diamand <luke@diamand.org>

This addresses Junio's concerns and makes more sense to me too.
It is odd for someone to submit commits not their own, but if
they want to, at least the warning can be hushed.

Two teensy fixes:

> +            if self.checkAuthorship and not self.p4UserIsMe(p4User):
> +                submitTemplate += "######## git author %s does not match your p4 account.\n" % gitEmail
> +                submitTemplate += "######## Use git-p4 option --preserve-user to modify authorship\n"
> +                submitTemplate += "######## Use git-p4 config git-p4.skipUserNameCheck hides this messsage.\n"
> +

s/messsage/message/


>              separatorLine = "######## everything below this line is just the diff #######\n"
>  
>              [handle, fileName] = tempfile.mkstemp()
> @@ -998,6 +1022,11 @@ class P4Submit(Command, P4UserMap):
>              commits.append(line.strip())
>          commits.reverse()
>  
> +        if not self.preserveUser and (gitConfig("git-p4.skipUserNameCheck") != "true"):
> +            self.checkAuthorship = True
> +        else:
> +            self.checkAuthorship = False
> +

Can we have a bit clearer logic?  I get confused with too many
negatives.

	#
	# Issue a warning in the submission template if the
	# author of the patch is different from the submitter.
	# But if preserveUser is used, the authorship will be
	# adjusted, hence no warning.  Also this message can be
	# hushed with a config option.
	#
	self.checkAuthorship = True
	if self.preserveUser:
	    self.checkAuthorship = False
	if gitConfig("git-p4.skipUserNameCheck") != "true":
	    self.checkAuthorship = False

> +make_change_by_user() {
> +    file=$1
> +    name=$2
> +    email=$3
> +	echo "username: a change by $name" >> $file &&
> +	git add $file &&
> +	git commit --author "$name <$email>" -m "a change by $name"
> +}

Tab/space oddities.

> +# If we're *not* using --preserve-user, git-p4 should warn if we're submitting
> +# changes that are not all ours.
> +# Test: user in p4 and user unknown to p4.
> +# Test: warning disabled and user is the same.
> +test_expect_success 'not preserving user with mixed authorship' '
> +	"$GITP4" clone --dest="$git" //depot &&
> +	cd "$git" &&
> +	git config git-p4.skipSubmitEditCheck true

Missing &&.  These are hard to remember, and I've been corrected
before too.

> +	p4_add_user derek Derek &&
> +
> +	make_change_by_user usernamefile3 Derek derek@localhost &&
> +	(P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |
> +		grep "git author derek@localhost does not match") &&
> +	git diff --exit-code HEAD..p4/master &&
> +
> +	make_change_by_user usernamefile3 Charlie charlie@localhost &&
> +	(P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |
> +		grep "git author charlie@localhost does not match") &&
> +	git diff --exit-code HEAD..p4/master &&
> +
> +	make_change_by_user usernamefile3 alice alice@localhost &&
> +	!(P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |
> +		grep "does not match") &&
> +	git diff --exit-code HEAD..p4/master &&
> +
> +	git config git-p4.skipUserNameCheck true &&
> +	make_change_by_user usernamefile3 Charlie charlie@localhost &&
> +	!(P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |
> +		grep "git author charlie@localhost does not match") &&
> +	git diff --exit-code HEAD..p4/master &&

Maybe -q on the greps.  Not a big deal.

> +	p4_check_commit_author usernamefile3 alice &&
> +	cd "$TRASH_DIRECTORY" &&
> +	rm -rf "$git" && mkdir "$git"
> +'

If you're okay on these fixes, go ahead and add my Acked-By for
the resend to Junio.

		-- Pete