Re: [PATCH v3] git-p4: add option to preserve user names

[Junio C Hamano <gitster@pobox.com>]

Pete Wyckoff <pw@padd.com> writes:

> luke@diamand.org wrote on Fri, 06 May 2011 06:25 +0100:
>> On 06/05/11 06:07, Junio C Hamano wrote:
>> >Luke Diamand<luke@diamand.org>  writes:
>> >
>> >>This is version 3 of my patch.
>> >
>> >The previous one from Apr 21st is already on "next" with Ack from Pete.
>> 
>> Ah, sorry.
>> 
>> Should I submit a patch against that then?
>
> Yes, your changes look good and fix bugs.
>
> To the diff from v2 to v3:
>
> Acked-by: Pete Wyckoff <pw@padd.com>

So the only thing lacking at this point is the commit log message?

I am not sure if the "actual user is luke" message you give when (and only
when) preserveUser is used is a good "reminder".  Isn't it that the user
needs reminder when the user should have used but forgot to use this
option, not the other way around like your patch does?

I suspect that the message would show an unexpected name only when the new
codepath is buggy or the P4 changes the code is interacting are formatted
in ways that the new codepath is not expecting (well, they amount to the
same thing after all, no?), and having such a message may prevent users
from submitting the changeset under an incorrect name, but at that point
what recourse do they have?

It looks to me that the message is not helping the users, even though it
may help as a debugging aid for git-p4 developers.

-- >8 --
Subject: git-p4 --preserve-user: finishing touches

The earlier round unnecessarily updated the user field that is already
correct.

Add a message with the P4 user name used in the submit template to
remind the user when this option is given.

Form the change spec string correctly, without relying on the way
Python happens to order the dictionary contents.

Signed-off-by: Luke Diamand <luke@diamand.org>
Acked-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4 |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 36e3d87..6018507 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -688,12 +688,18 @@ class P4Submit(Command, P4UserMap):
         die("Could not get changelist number for last submit - cannot patch up user details")
 
     def modifyChangelistUser(self, changelist, newUser):
-        # fixup the user field of a changelist after it has been submitted.
+        # fixup the user field of a single changelist after it has been submitted.
         changes = p4CmdList("change -o %s" % changelist)
+        input = ''
         for c in changes:
             if c.has_key('User'):
+                if c['User'] == newUser:
+                    # nothing to do here
+                    return
                 c['User'] = newUser
-        input = marshal.dumps(changes[0])
+
+            input = input + marshal.dumps(c)
+
         result = p4CmdList("change -f -i", stdin=input)
         for r in result:
             if r.has_key('code'):
@@ -703,11 +709,10 @@ class P4Submit(Command, P4UserMap):
                 print("Updated user field for changelist %s to %s" % (changelist, newUser))
                 return
         die("Could not modify user field of changelist %s to %s" % (changelist, newUser))
-
     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,10 @@ class P4Submit(Command, P4UserMap):
 
         if self.interactive:
             submitTemplate = self.prepareLogMessage(template, logMessage)
+
+            if self.preserveUser:
+               submitTemplate = submitTemplate + ("\n######## Actual user %s, modified after commit\n" % p4User)
+
             if os.environ.has_key("P4DIFF"):
                 del(os.environ["P4DIFF"])
             diff = ""