* [PATCH] git-p4: add option to preserve user names
@ 2011-04-19 18:01 Luke Diamand
2011-04-19 18:01 ` Luke Diamand
0 siblings, 1 reply; 6+ messages in thread
From: Luke Diamand @ 2011-04-19 18:01 UTC (permalink / raw)
To: git, git; +Cc: Luke Diamand, Pete Wyckoff
Patches from git passed into p4 end up with the committer
being identified as the person who ran git-p4.
With "submit --preserve-user", git-p4 sets P4USER. If the
submitter has sufficient p4 permissions, the p4 equivalent
of the git email committer will be passed into perforce.
Luke Diamand (1):
git-p4: add option to preserve user names
contrib/fast-import/git-p4 | 33 +++++++++++++++++++++++++++++++++
contrib/fast-import/git-p4.txt | 6 ++++++
2 files changed, 39 insertions(+), 0 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] git-p4: add option to preserve user names
2011-04-19 18:01 Luke Diamand
@ 2011-04-19 18:01 ` Luke Diamand
2011-04-20 0:10 ` Pete Wyckoff
0 siblings, 1 reply; 6+ messages in thread
From: Luke Diamand @ 2011-04-19 18:01 UTC (permalink / raw)
To: git, git; +Cc: Luke Diamand, Pete Wyckoff
Patches from git passed into p4 end up with the committer
being identified as the person who ran git-p4.
With "submit --preserve-user", git-p4 sets P4USER. If the
submitter has sufficient p4 permissions, the p4 equivalent
of the git email committer will be passed into perforce.
Signed-off-by: Luke Diamand <luke@diamand.org>
---
contrib/fast-import/git-p4 | 33 +++++++++++++++++++++++++++++++++
contrib/fast-import/git-p4.txt | 6 ++++++
2 files changed, 39 insertions(+), 0 deletions(-)
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 78e5b3a..7d66aa9 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -561,6 +561,8 @@ class P4Submit(Command):
optparse.make_option("--verbose", dest="verbose", action="store_true"),
optparse.make_option("--origin", dest="origin"),
optparse.make_option("-M", dest="detectRenames", action="store_true"),
+ # preserve the user, assumes relevant p4 permissions available
+ optparse.make_option("--preserve-user", dest="preserveUser", action="store_true"),
]
self.description = "Submit changes from git to the perforce depot."
self.usage += " [name of git branch to submit into perforce depot]"
@@ -568,6 +570,7 @@ class P4Submit(Command):
self.origin = ""
self.detectRenames = False
self.verbose = False
+ self.preserveUser = False
self.isWindows = (platform.system() == "Windows")
def check(self):
@@ -592,6 +595,11 @@ class P4Submit(Command):
else:
continue
else:
+ if self.preserveUser:
+ if self.p4user:
+ if line.startswith("User:"):
+ line = "User: %s" % self.p4user
+
if line.startswith("Description:"):
inDescriptionSection = True
line += "\n"
@@ -602,6 +610,18 @@ class P4Submit(Command):
return result
+ def p4User(self,id):
+ # Return the perforce user for a given git commit id
+ git_email = read_pipe("git log --max-count=1 --format='%%ae' %s" % id)
+ git_email = git_email.strip()
+ if not self.email_to_user.has_key(git_email):
+ print("Cannot find perforce user for email %s in commit %s." %
+ (git_email, id))
+ print("Submitting changelist with default user - fixup later manually!")
+ return None
+ else:
+ return self.email_to_user[git_email]
+
def prepareSubmitTemplate(self):
# remove lines in the Files section that show changes to files outside the depot path we're committing into
template = ""
@@ -631,6 +651,12 @@ class P4Submit(Command):
def applyCommit(self, id):
print "Applying %s" % (read_pipe("git log --max-count=1 --pretty=oneline %s" % id))
+ if self.preserveUser:
+ p4user = self.p4User(id)
+ self.p4user = p4user
+ if p4user:
+ os.putenv('P4USER', p4user)
+
if not self.detectRenames:
# If not explicitly set check the config variable
self.detectRenames = gitConfig("git-p4.detectRenames").lower() == "true"
@@ -847,6 +873,13 @@ class P4Submit(Command):
print "Perforce checkout for depot path %s located at %s" % (self.depotPath, self.clientPath)
self.oldWorkingDirectory = os.getcwd()
+ if self.preserveUser:
+ self.email_to_user = {}
+ for output in p4CmdList("users"):
+ if not output.has_key("User"):
+ continue
+ self.email_to_user[output["Email"]] = output["User"]
+
chdir(self.clientPath)
print "Synchronizing p4 checkout..."
p4_system("sync ...")
diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt
index e09da44..7c3c794 100644
--- a/contrib/fast-import/git-p4.txt
+++ b/contrib/fast-import/git-p4.txt
@@ -110,6 +110,12 @@ 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.
+
If a submit fails you may have to "p4 resolve" and submit manually. You can
continue importing the remaining changes with
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] git-p4: add option to preserve user names
2011-04-19 18:01 ` Luke Diamand
@ 2011-04-20 0:10 ` Pete Wyckoff
0 siblings, 0 replies; 6+ messages in thread
From: Pete Wyckoff @ 2011-04-20 0:10 UTC (permalink / raw)
To: Luke Diamand; +Cc: git
luke@diamand.org wrote on Tue, 19 Apr 2011 19:01 +0100:
> Patches from git passed into p4 end up with the committer
> being identified as the person who ran git-p4.
>
> With "submit --preserve-user", git-p4 sets P4USER. If the
> submitter has sufficient p4 permissions, the p4 equivalent
> of the git email committer will be passed into perforce.
I like the idea. Have often thought about using this approach
myself. Some comments:
1. Can we have a .git/config option too?
2. p4 users output is already cached; you should use
the same loadUserMapFromCache() and friends, or refactor
to share the cache.
3. Andrew will convert your tabs into spaces later, maybe save him
the effort now.
4. Do you need both "P4USER" and "User:" for this to work?
Perhaps just use the "User:" one if so, it is a bit more
concise.
5. Document how "p4 protect" is used to give admin privileges
needed for this to work, but won't tell you if you have them.
I think it may be true that "p4 typemap -o" needs admin
priviliges, and is a way to test if you ever need that.
6. What happens if you try, and don't have admin? Does it
silently just use you instead? And your comment about
"fixup later manually": I've always wondered how actually
to do that with p4, can you advise?
7. Super-bonus points for a test case in t/t9800-git-p4.sh. :)
-- Pete
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH/RFC v2] git-p4: add option to preserve user names
@ 2011-04-21 19:50 Luke Diamand
2011-04-21 19:50 ` [PATCH] " Luke Diamand
0 siblings, 1 reply; 6+ messages in thread
From: Luke Diamand @ 2011-04-21 19:50 UTC (permalink / raw)
To: git, git; +Cc: Pete Wyckoff, Luke Diamand
Patches from git passed into p4 end up with the committer
being identified as the person who ran git-p4.
This patch adds an option --preserve-user. When enabled, git-p4
will modify the changelist after it has been submitted ("p4 change -f")
and set the username to the one matching the git author.
If the person running git-p4 does not have sufficient permissions,
git-p4 will refuse to run (detected using "p4 protects").
It's possible that complicated permissions setups might confuse
git-p4 - it just looks to see if the user has admin or super on
the repo. In theory they might have permissions in some parts
and not in others.
If there are commits with authors who do not have p4 accounts, then
git-p4 will refuse to run unless git-p4.allowMissingP4Users is true,
in which case it falls back to the standard behaviour for those
commits.
The code has to get the p4 changelist number. The way it
does this is by simply doing 'p4 changes -c <client>', which
means if someone else is using the same clientspec at the
same time, there is a potential race hazard. The alternative
is to rewrite the submit logic to submit a properly marshalled
template, which felt a bit too intrusive.
I've hoisted the p4 user name cache to a separate class, since it
gets used in a couple of different places now.
I've added an option git-p4.skipSubmitModTimeCheck so that I can
write a test case without having to jump through hoops with the
editor.
Luke Diamand (1):
git-p4: add option to preserve user names
contrib/fast-import/git-p4 | 179 +++++++++++++++++++++++++++++++---------
contrib/fast-import/git-p4.txt | 29 +++++++
t/t9800-git-p4.sh | 84 +++++++++++++++++++
3 files changed, 254 insertions(+), 38 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] git-p4: add option to preserve user names
2011-04-21 19:50 [PATCH/RFC v2] git-p4: add option to preserve user names Luke Diamand
@ 2011-04-21 19:50 ` Luke Diamand
2011-04-23 11:54 ` Pete Wyckoff
0 siblings, 1 reply; 6+ messages in thread
From: Luke Diamand @ 2011-04-21 19:50 UTC (permalink / raw)
To: git, git; +Cc: Pete Wyckoff, Luke Diamand
Patches from git passed into p4 end up with the committer
being identified as the person who ran git-p4.
With "submit --preserve-user", git-p4 modifies the p4
changelist (after it has been submitted), setting the
p4 author field.
The submitter is required to have sufficient p4 permissions
or git-p4 refuses to proceed. If the git author is not
known to p4, the submit will be abandoned unless
git-p4.allowMissingP4Users is true.
Signed-off-by: Luke Diamand <luke@diamand.org>
---
contrib/fast-import/git-p4 | 179 +++++++++++++++++++++++++++++++---------
contrib/fast-import/git-p4.txt | 29 +++++++
t/t9800-git-p4.sh | 84 +++++++++++++++++++
3 files changed, 254 insertions(+), 38 deletions(-)
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 78e5b3a..36e3d87 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -474,6 +474,47 @@ class Command:
self.usage = "usage: %prog [options]"
self.needsGit = True
+class P4UserMap:
+ def __init__(self):
+ self.userMapFromPerforceServer = False
+
+ def getUserCacheFilename(self):
+ home = os.environ.get("HOME", os.environ.get("USERPROFILE"))
+ return home + "/.gitp4-usercache.txt"
+
+ def getUserMapFromPerforceServer(self):
+ if self.userMapFromPerforceServer:
+ return
+ self.users = {}
+ self.emails = {}
+
+ for output in p4CmdList("users"):
+ if not output.has_key("User"):
+ continue
+ self.users[output["User"]] = output["FullName"] + " <" + output["Email"] + ">"
+ self.emails[output["Email"]] = output["User"]
+
+
+ s = ''
+ for (key, val) in self.users.items():
+ s += "%s\t%s\n" % (key.expandtabs(1), val.expandtabs(1))
+
+ open(self.getUserCacheFilename(), "wb").write(s)
+ self.userMapFromPerforceServer = True
+
+ def loadUserMapFromCache(self):
+ self.users = {}
+ self.userMapFromPerforceServer = False
+ try:
+ cache = open(self.getUserCacheFilename(), "rb")
+ lines = cache.readlines()
+ cache.close()
+ for line in lines:
+ entry = line.strip().split("\t")
+ self.users[entry[0]] = entry[1]
+ except IOError:
+ self.getUserMapFromPerforceServer()
+
class P4Debug(Command):
def __init__(self):
Command.__init__(self)
@@ -554,13 +595,16 @@ class P4RollBack(Command):
return True
-class P4Submit(Command):
+class P4Submit(Command, P4UserMap):
def __init__(self):
Command.__init__(self)
+ P4UserMap.__init__(self)
self.options = [
optparse.make_option("--verbose", dest="verbose", action="store_true"),
optparse.make_option("--origin", dest="origin"),
optparse.make_option("-M", dest="detectRenames", action="store_true"),
+ # preserve the user, requires relevant p4 permissions
+ optparse.make_option("--preserve-user", dest="preserveUser", action="store_true"),
]
self.description = "Submit changes from git to the perforce depot."
self.usage += " [name of git branch to submit into perforce depot]"
@@ -568,6 +612,7 @@ class P4Submit(Command):
self.origin = ""
self.detectRenames = False
self.verbose = False
+ self.preserveUser = gitConfig("git-p4.preserveUser").lower() == "true"
self.isWindows = (platform.system() == "Windows")
def check(self):
@@ -602,6 +647,75 @@ class P4Submit(Command):
return result
+ def p4UserForCommit(self,id):
+ # Return the tuple (perforce user,git email) for a given git commit id
+ self.getUserMapFromPerforceServer()
+ gitEmail = read_pipe("git log --max-count=1 --format='%%ae' %s" % id)
+ gitEmail = gitEmail.strip()
+ if not self.emails.has_key(gitEmail):
+ return (None,gitEmail)
+ else:
+ return (self.emails[gitEmail],gitEmail)
+
+ def checkValidP4Users(self,commits):
+ # check if any git authors cannot be mapped to p4 users
+ for id in commits:
+ (user,email) = self.p4UserForCommit(id)
+ if not user:
+ msg = "Cannot find p4 user for email %s in commit %s." % (email, id)
+ if gitConfig('git-p4.allowMissingP4Users').lower() == "true":
+ print "%s" % msg
+ else:
+ die("Error: %s\nSet git-p4.allowMissingP4Users to true to allow this." % msg)
+
+ def lastP4Changelist(self):
+ # Get back the last changelist number submitted in this client spec. This
+ # then gets used to patch up the username in the change. If the same
+ # client spec is being used by multiple processes then this might go
+ # wrong.
+ results = p4CmdList("client -o") # find the current client
+ client = None
+ for r in results:
+ if r.has_key('Client'):
+ client = r['Client']
+ break
+ if not client:
+ die("could not get client spec")
+ results = p4CmdList("changes -c %s -m 1" % client)
+ for r in results:
+ if r.has_key('change'):
+ return r['change']
+ 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.
+ changes = p4CmdList("change -o %s" % changelist)
+ for c in changes:
+ if c.has_key('User'):
+ c['User'] = newUser
+ input = marshal.dumps(changes[0])
+ result = p4CmdList("change -f -i", stdin=input)
+ for r in result:
+ if r.has_key('code'):
+ if r['code'] == 'error':
+ die("Could not modify user field of changelist %s to %s:%s" % (changelist, newUser, r['data']))
+ if r.has_key('data'):
+ 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)
+ for r in results:
+ if r.has_key('perm'):
+ if r['perm'] == 'admin':
+ return 1
+ if r['perm'] == 'super':
+ return 1
+ return 0
+
def prepareSubmitTemplate(self):
# remove lines in the Files section that show changes to files outside the depot path we're committing into
template = ""
@@ -631,6 +745,9 @@ class P4Submit(Command):
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)
+
if not self.detectRenames:
# If not explicitly set check the config variable
self.detectRenames = gitConfig("git-p4.detectRenames").lower() == "true"
@@ -781,8 +898,13 @@ class P4Submit(Command):
editor = read_pipe("git var GIT_EDITOR").strip()
system(editor + " " + fileName)
+ if gitConfig("git-p4.skipSubmitEditCheck") == "true":
+ checkModTime = False
+ else:
+ checkModTime = True
+
response = "y"
- if os.stat(fileName).st_mtime <= mtime:
+ if checkModTime and (os.stat(fileName).st_mtime <= mtime):
response = "x"
while response != "y" and response != "n":
response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
@@ -795,6 +917,14 @@ class P4Submit(Command):
if self.isWindows:
submitTemplate = submitTemplate.replace("\r\n", "\n")
p4_write_pipe("submit -i", submitTemplate)
+
+ if self.preserveUser:
+ if p4User:
+ # Get last changelist number. Cannot easily get it from
+ # the submit command output as the output is unmarshalled.
+ changelist = self.lastP4Changelist()
+ self.modifyChangelistUser(changelist, p4User)
+
else:
for f in editedFiles:
p4_system("revert \"%s\"" % f);
@@ -831,6 +961,10 @@ class P4Submit(Command):
if len(self.origin) == 0:
self.origin = upstream
+ if self.preserveUser:
+ if not self.canChangeChangelists():
+ die("Cannot preserve user names without p4 super-user or admin permissions")
+
if self.verbose:
print "Origin branch is " + self.origin
@@ -858,6 +992,9 @@ class P4Submit(Command):
commits.append(line.strip())
commits.reverse()
+ if self.preserveUser:
+ self.checkValidP4Users(commits)
+
while len(commits) > 0:
commit = commits[0]
commits = commits[1:]
@@ -877,11 +1014,12 @@ class P4Submit(Command):
return True
-class P4Sync(Command):
+class P4Sync(Command, P4UserMap):
delete_actions = ( "delete", "move/delete", "purge" )
def __init__(self):
Command.__init__(self)
+ P4UserMap.__init__(self)
self.options = [
optparse.make_option("--branch", dest="branch"),
optparse.make_option("--detect-branches", dest="detectBranches", action="store_true"),
@@ -1236,41 +1374,6 @@ class P4Sync(Command):
print ("Tag %s does not match with change %s: file count is different."
% (labelDetails["label"], change))
- def getUserCacheFilename(self):
- home = os.environ.get("HOME", os.environ.get("USERPROFILE"))
- return home + "/.gitp4-usercache.txt"
-
- def getUserMapFromPerforceServer(self):
- if self.userMapFromPerforceServer:
- return
- self.users = {}
-
- for output in p4CmdList("users"):
- if not output.has_key("User"):
- continue
- self.users[output["User"]] = output["FullName"] + " <" + output["Email"] + ">"
-
-
- s = ''
- for (key, val) in self.users.items():
- s += "%s\t%s\n" % (key.expandtabs(1), val.expandtabs(1))
-
- open(self.getUserCacheFilename(), "wb").write(s)
- self.userMapFromPerforceServer = True
-
- def loadUserMapFromCache(self):
- self.users = {}
- self.userMapFromPerforceServer = False
- try:
- cache = open(self.getUserCacheFilename(), "rb")
- lines = cache.readlines()
- cache.close()
- for line in lines:
- entry = line.strip().split("\t")
- self.users[entry[0]] = entry[1]
- except IOError:
- self.getUserMapFromPerforceServer()
-
def getLabels(self):
self.labels = {}
diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt
index e09da44..b6986f0 100644
--- a/contrib/fast-import/git-p4.txt
+++ b/contrib/fast-import/git-p4.txt
@@ -110,6 +110,12 @@ 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'.
+
If a submit fails you may have to "p4 resolve" and submit manually. You can
continue importing the remaining changes with
@@ -196,6 +202,29 @@ able to find the relevant client. This client spec will be used to
both filter the files cloned by git and set the directory layout as
specified in the client (this implies --keep-path style semantics).
+git-p4.skipSubmitModTimeCheck
+
+ git config [--global] git-p4.skipSubmitModTimeCheck false
+
+If true, submit will not check if the p4 change template has been modified.
+
+git-p4.preserveUser
+
+ git config [--global] git-p4.preserveUser false
+
+If true, attempt to preserve user names by modifying the p4 changelists. See
+the "--preserve-user" submit option.
+
+git-p4.allowMissingPerforceUsers
+
+ git config [--global] git-p4.allowMissingP4Users false
+
+If git-p4 is setting the perforce user for a commit (--preserve-user) then
+if there is no perforce user corresponding to the git author, git-p4 will
+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.
+
Implementation Details...
=========================
diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index a523473..fd3204b 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -12,6 +12,8 @@ test_description='git-p4 tests'
GITP4=$GIT_BUILD_DIR/contrib/fast-import/git-p4
P4DPORT=10669
+export P4PORT=localhost:$P4DPORT
+
db="$TRASH_DIRECTORY/db"
cli="$TRASH_DIRECTORY/cli"
git="$TRASH_DIRECTORY/git"
@@ -129,6 +131,88 @@ test_expect_success 'clone bare' '
rm -rf "$git" && mkdir "$git"
'
+p4_add_user() {
+ name=$1
+ fullname=$2
+ p4 user -f -i <<EOF &&
+User: $name
+Email: $name@localhost
+FullName: $fullname
+EOF
+ p4 passwd -P secret $name
+}
+
+p4_grant_admin() {
+ name=$1
+ p4 protect -o |\
+ awk "{print}END{print \" admin user $name * //depot/...\"}" |\
+ p4 protect -i
+}
+
+p4_check_commit_author() {
+ file=$1
+ user=$2
+ if p4 changes -m 1 //depot/$file | grep $user > /dev/null ; then
+ return 0
+ else
+ echo "file $file not modified by user $user" 1>&2
+ return 1
+ fi
+}
+
+# Test username support, submitting as user 'alice'
+test_expect_success 'preserve users' '
+ p4_add_user alice Alice &&
+ p4_add_user bob Bob &&
+ p4_grant_admin alice &&
+ "$GITP4" clone --dest="$git" //depot &&
+ cd "$git" &&
+ echo "username: a change by alice" >> file1 &&
+ echo "username: a change by bob" >> file2 &&
+ git commit --author "Alice <alice@localhost>" -m "a change by alice" file1 &&
+ git commit --author "Bob <bob@localhost>" -m "a change by bob" file2 &&
+ git config git-p4.skipSubmitEditCheck true &&
+ P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user &&
+ p4_check_commit_author file1 alice &&
+ p4_check_commit_author file2 bob &&
+ cd "$TRASH_DIRECTORY" &&
+ rm -rf "$git" && mkdir "$git"
+'
+
+# Test username support, submitting as bob, who lacks admin rights. Should
+# not submit change to p4 (git diff should show deltas).
+test_expect_success 'refuse to preserve users without perms' '
+ "$GITP4" clone --dest="$git" //depot &&
+ cd "$git" &&
+ echo "username-noperms: a change by alice" >> file1 &&
+ git commit --author "Alice <alice@localhost>" -m "perms: a change by alice" file1 &&
+ ! P4EDITOR=touch P4USER=bob P4PASSWD=secret "$GITP4" commit --preserve-user &&
+ ! git diff --exit-code HEAD..p4/master > /dev/null &&
+ cd "$TRASH_DIRECTORY" &&
+ rm -rf "$git" && mkdir "$git"
+'
+
+# What happens with unknown author? Without allowMissingP4Users it should fail.
+test_expect_success 'preserve user where author is unknown to p4' '
+ "$GITP4" clone --dest="$git" //depot &&
+ cd "$git" &&
+ git config git-p4.skipSubmitEditCheck true
+ echo "username-bob: a change by bob" >> file1 &&
+ git commit --author "Bob <bob@localhost>" -m "preserve: a change by bob" file1 &&
+ echo "username-unknown: a change by charlie" >> file1 &&
+ git commit --author "Charlie <charlie@localhost>" -m "preserve: a change by charlie" file1 &&
+ ! P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user &&
+ ! git diff --exit-code HEAD..p4/master > /dev/null &&
+ echo "$0: repeat with allowMissingP4Users enabled" &&
+ git config git-p4.allowMissingP4Users true &&
+ git config git-p4.preserveUser true &&
+ P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit &&
+ git diff --exit-code HEAD..p4/master > /dev/null &&
+ p4_check_commit_author file1 alice &&
+ cd "$TRASH_DIRECTORY" &&
+ rm -rf "$git" && mkdir "$git"
+'
+
test_expect_success 'shutdown' '
pid=`pgrep -f p4d` &&
test -n "$pid" &&
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] git-p4: add option to preserve user names
2011-04-21 19:50 ` [PATCH] " Luke Diamand
@ 2011-04-23 11:54 ` Pete Wyckoff
0 siblings, 0 replies; 6+ messages in thread
From: Pete Wyckoff @ 2011-04-23 11:54 UTC (permalink / raw)
To: Luke Diamand; +Cc: git
luke@diamand.org wrote on Thu, 21 Apr 2011 20:50 +0100:
> Patches from git passed into p4 end up with the committer
> being identified as the person who ran git-p4.
>
> With "submit --preserve-user", git-p4 modifies the p4
> changelist (after it has been submitted), setting the
> p4 author field.
>
> The submitter is required to have sufficient p4 permissions
> or git-p4 refuses to proceed. If the git author is not
> known to p4, the submit will be abandoned unless
> git-p4.allowMissingP4Users is true.
>
> Signed-off-by: Luke Diamand <luke@diamand.org>
I like this patch. Quite impressed with the care you put into
refactoring, handling all the error cases, and documentation too.
Acked-by: Pete Wyckoff <pw@padd.com>
Maybe wait for some more comments, then send it to Junio in a week.
-- Pete
> ---
> contrib/fast-import/git-p4 | 179 +++++++++++++++++++++++++++++++---------
> contrib/fast-import/git-p4.txt | 29 +++++++
> t/t9800-git-p4.sh | 84 +++++++++++++++++++
> 3 files changed, 254 insertions(+), 38 deletions(-)
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index 78e5b3a..36e3d87 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -474,6 +474,47 @@ class Command:
> self.usage = "usage: %prog [options]"
> self.needsGit = True
>
> +class P4UserMap:
> + def __init__(self):
> + self.userMapFromPerforceServer = False
> +
> + def getUserCacheFilename(self):
> + home = os.environ.get("HOME", os.environ.get("USERPROFILE"))
> + return home + "/.gitp4-usercache.txt"
> +
> + def getUserMapFromPerforceServer(self):
> + if self.userMapFromPerforceServer:
> + return
> + self.users = {}
> + self.emails = {}
> +
> + for output in p4CmdList("users"):
> + if not output.has_key("User"):
> + continue
> + self.users[output["User"]] = output["FullName"] + " <" + output["Email"] + ">"
> + self.emails[output["Email"]] = output["User"]
> +
> +
> + s = ''
> + for (key, val) in self.users.items():
> + s += "%s\t%s\n" % (key.expandtabs(1), val.expandtabs(1))
> +
> + open(self.getUserCacheFilename(), "wb").write(s)
> + self.userMapFromPerforceServer = True
> +
> + def loadUserMapFromCache(self):
> + self.users = {}
> + self.userMapFromPerforceServer = False
> + try:
> + cache = open(self.getUserCacheFilename(), "rb")
> + lines = cache.readlines()
> + cache.close()
> + for line in lines:
> + entry = line.strip().split("\t")
> + self.users[entry[0]] = entry[1]
> + except IOError:
> + self.getUserMapFromPerforceServer()
> +
> class P4Debug(Command):
> def __init__(self):
> Command.__init__(self)
> @@ -554,13 +595,16 @@ class P4RollBack(Command):
>
> return True
>
> -class P4Submit(Command):
> +class P4Submit(Command, P4UserMap):
> def __init__(self):
> Command.__init__(self)
> + P4UserMap.__init__(self)
> self.options = [
> optparse.make_option("--verbose", dest="verbose", action="store_true"),
> optparse.make_option("--origin", dest="origin"),
> optparse.make_option("-M", dest="detectRenames", action="store_true"),
> + # preserve the user, requires relevant p4 permissions
> + optparse.make_option("--preserve-user", dest="preserveUser", action="store_true"),
> ]
> self.description = "Submit changes from git to the perforce depot."
> self.usage += " [name of git branch to submit into perforce depot]"
> @@ -568,6 +612,7 @@ class P4Submit(Command):
> self.origin = ""
> self.detectRenames = False
> self.verbose = False
> + self.preserveUser = gitConfig("git-p4.preserveUser").lower() == "true"
> self.isWindows = (platform.system() == "Windows")
>
> def check(self):
> @@ -602,6 +647,75 @@ class P4Submit(Command):
>
> return result
>
> + def p4UserForCommit(self,id):
> + # Return the tuple (perforce user,git email) for a given git commit id
> + self.getUserMapFromPerforceServer()
> + gitEmail = read_pipe("git log --max-count=1 --format='%%ae' %s" % id)
> + gitEmail = gitEmail.strip()
> + if not self.emails.has_key(gitEmail):
> + return (None,gitEmail)
> + else:
> + return (self.emails[gitEmail],gitEmail)
> +
> + def checkValidP4Users(self,commits):
> + # check if any git authors cannot be mapped to p4 users
> + for id in commits:
> + (user,email) = self.p4UserForCommit(id)
> + if not user:
> + msg = "Cannot find p4 user for email %s in commit %s." % (email, id)
> + if gitConfig('git-p4.allowMissingP4Users').lower() == "true":
> + print "%s" % msg
> + else:
> + die("Error: %s\nSet git-p4.allowMissingP4Users to true to allow this." % msg)
> +
> + def lastP4Changelist(self):
> + # Get back the last changelist number submitted in this client spec. This
> + # then gets used to patch up the username in the change. If the same
> + # client spec is being used by multiple processes then this might go
> + # wrong.
> + results = p4CmdList("client -o") # find the current client
> + client = None
> + for r in results:
> + if r.has_key('Client'):
> + client = r['Client']
> + break
> + if not client:
> + die("could not get client spec")
> + results = p4CmdList("changes -c %s -m 1" % client)
> + for r in results:
> + if r.has_key('change'):
> + return r['change']
> + 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.
> + changes = p4CmdList("change -o %s" % changelist)
> + for c in changes:
> + if c.has_key('User'):
> + c['User'] = newUser
> + input = marshal.dumps(changes[0])
> + result = p4CmdList("change -f -i", stdin=input)
> + for r in result:
> + if r.has_key('code'):
> + if r['code'] == 'error':
> + die("Could not modify user field of changelist %s to %s:%s" % (changelist, newUser, r['data']))
> + if r.has_key('data'):
> + 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)
> + for r in results:
> + if r.has_key('perm'):
> + if r['perm'] == 'admin':
> + return 1
> + if r['perm'] == 'super':
> + return 1
> + return 0
> +
> def prepareSubmitTemplate(self):
> # remove lines in the Files section that show changes to files outside the depot path we're committing into
> template = ""
> @@ -631,6 +745,9 @@ class P4Submit(Command):
> 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)
> +
> if not self.detectRenames:
> # If not explicitly set check the config variable
> self.detectRenames = gitConfig("git-p4.detectRenames").lower() == "true"
> @@ -781,8 +898,13 @@ class P4Submit(Command):
> editor = read_pipe("git var GIT_EDITOR").strip()
> system(editor + " " + fileName)
>
> + if gitConfig("git-p4.skipSubmitEditCheck") == "true":
> + checkModTime = False
> + else:
> + checkModTime = True
> +
> response = "y"
> - if os.stat(fileName).st_mtime <= mtime:
> + if checkModTime and (os.stat(fileName).st_mtime <= mtime):
> response = "x"
> while response != "y" and response != "n":
> response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
> @@ -795,6 +917,14 @@ class P4Submit(Command):
> if self.isWindows:
> submitTemplate = submitTemplate.replace("\r\n", "\n")
> p4_write_pipe("submit -i", submitTemplate)
> +
> + if self.preserveUser:
> + if p4User:
> + # Get last changelist number. Cannot easily get it from
> + # the submit command output as the output is unmarshalled.
> + changelist = self.lastP4Changelist()
> + self.modifyChangelistUser(changelist, p4User)
> +
> else:
> for f in editedFiles:
> p4_system("revert \"%s\"" % f);
> @@ -831,6 +961,10 @@ class P4Submit(Command):
> if len(self.origin) == 0:
> self.origin = upstream
>
> + if self.preserveUser:
> + if not self.canChangeChangelists():
> + die("Cannot preserve user names without p4 super-user or admin permissions")
> +
> if self.verbose:
> print "Origin branch is " + self.origin
>
> @@ -858,6 +992,9 @@ class P4Submit(Command):
> commits.append(line.strip())
> commits.reverse()
>
> + if self.preserveUser:
> + self.checkValidP4Users(commits)
> +
> while len(commits) > 0:
> commit = commits[0]
> commits = commits[1:]
> @@ -877,11 +1014,12 @@ class P4Submit(Command):
>
> return True
>
> -class P4Sync(Command):
> +class P4Sync(Command, P4UserMap):
> delete_actions = ( "delete", "move/delete", "purge" )
>
> def __init__(self):
> Command.__init__(self)
> + P4UserMap.__init__(self)
> self.options = [
> optparse.make_option("--branch", dest="branch"),
> optparse.make_option("--detect-branches", dest="detectBranches", action="store_true"),
> @@ -1236,41 +1374,6 @@ class P4Sync(Command):
> print ("Tag %s does not match with change %s: file count is different."
> % (labelDetails["label"], change))
>
> - def getUserCacheFilename(self):
> - home = os.environ.get("HOME", os.environ.get("USERPROFILE"))
> - return home + "/.gitp4-usercache.txt"
> -
> - def getUserMapFromPerforceServer(self):
> - if self.userMapFromPerforceServer:
> - return
> - self.users = {}
> -
> - for output in p4CmdList("users"):
> - if not output.has_key("User"):
> - continue
> - self.users[output["User"]] = output["FullName"] + " <" + output["Email"] + ">"
> -
> -
> - s = ''
> - for (key, val) in self.users.items():
> - s += "%s\t%s\n" % (key.expandtabs(1), val.expandtabs(1))
> -
> - open(self.getUserCacheFilename(), "wb").write(s)
> - self.userMapFromPerforceServer = True
> -
> - def loadUserMapFromCache(self):
> - self.users = {}
> - self.userMapFromPerforceServer = False
> - try:
> - cache = open(self.getUserCacheFilename(), "rb")
> - lines = cache.readlines()
> - cache.close()
> - for line in lines:
> - entry = line.strip().split("\t")
> - self.users[entry[0]] = entry[1]
> - except IOError:
> - self.getUserMapFromPerforceServer()
> -
> def getLabels(self):
> self.labels = {}
>
> diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt
> index e09da44..b6986f0 100644
> --- a/contrib/fast-import/git-p4.txt
> +++ b/contrib/fast-import/git-p4.txt
> @@ -110,6 +110,12 @@ 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'.
> +
> If a submit fails you may have to "p4 resolve" and submit manually. You can
> continue importing the remaining changes with
>
> @@ -196,6 +202,29 @@ able to find the relevant client. This client spec will be used to
> both filter the files cloned by git and set the directory layout as
> specified in the client (this implies --keep-path style semantics).
>
> +git-p4.skipSubmitModTimeCheck
> +
> + git config [--global] git-p4.skipSubmitModTimeCheck false
> +
> +If true, submit will not check if the p4 change template has been modified.
> +
> +git-p4.preserveUser
> +
> + git config [--global] git-p4.preserveUser false
> +
> +If true, attempt to preserve user names by modifying the p4 changelists. See
> +the "--preserve-user" submit option.
> +
> +git-p4.allowMissingPerforceUsers
> +
> + git config [--global] git-p4.allowMissingP4Users false
> +
> +If git-p4 is setting the perforce user for a commit (--preserve-user) then
> +if there is no perforce user corresponding to the git author, git-p4 will
> +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.
> +
> Implementation Details...
> =========================
>
> diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
> index a523473..fd3204b 100755
> --- a/t/t9800-git-p4.sh
> +++ b/t/t9800-git-p4.sh
> @@ -12,6 +12,8 @@ test_description='git-p4 tests'
> GITP4=$GIT_BUILD_DIR/contrib/fast-import/git-p4
> P4DPORT=10669
>
> +export P4PORT=localhost:$P4DPORT
> +
> db="$TRASH_DIRECTORY/db"
> cli="$TRASH_DIRECTORY/cli"
> git="$TRASH_DIRECTORY/git"
> @@ -129,6 +131,88 @@ test_expect_success 'clone bare' '
> rm -rf "$git" && mkdir "$git"
> '
>
> +p4_add_user() {
> + name=$1
> + fullname=$2
> + p4 user -f -i <<EOF &&
> +User: $name
> +Email: $name@localhost
> +FullName: $fullname
> +EOF
> + p4 passwd -P secret $name
> +}
> +
> +p4_grant_admin() {
> + name=$1
> + p4 protect -o |\
> + awk "{print}END{print \" admin user $name * //depot/...\"}" |\
> + p4 protect -i
> +}
> +
> +p4_check_commit_author() {
> + file=$1
> + user=$2
> + if p4 changes -m 1 //depot/$file | grep $user > /dev/null ; then
> + return 0
> + else
> + echo "file $file not modified by user $user" 1>&2
> + return 1
> + fi
> +}
> +
> +# Test username support, submitting as user 'alice'
> +test_expect_success 'preserve users' '
> + p4_add_user alice Alice &&
> + p4_add_user bob Bob &&
> + p4_grant_admin alice &&
> + "$GITP4" clone --dest="$git" //depot &&
> + cd "$git" &&
> + echo "username: a change by alice" >> file1 &&
> + echo "username: a change by bob" >> file2 &&
> + git commit --author "Alice <alice@localhost>" -m "a change by alice" file1 &&
> + git commit --author "Bob <bob@localhost>" -m "a change by bob" file2 &&
> + git config git-p4.skipSubmitEditCheck true &&
> + P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user &&
> + p4_check_commit_author file1 alice &&
> + p4_check_commit_author file2 bob &&
> + cd "$TRASH_DIRECTORY" &&
> + rm -rf "$git" && mkdir "$git"
> +'
> +
> +# Test username support, submitting as bob, who lacks admin rights. Should
> +# not submit change to p4 (git diff should show deltas).
> +test_expect_success 'refuse to preserve users without perms' '
> + "$GITP4" clone --dest="$git" //depot &&
> + cd "$git" &&
> + echo "username-noperms: a change by alice" >> file1 &&
> + git commit --author "Alice <alice@localhost>" -m "perms: a change by alice" file1 &&
> + ! P4EDITOR=touch P4USER=bob P4PASSWD=secret "$GITP4" commit --preserve-user &&
> + ! git diff --exit-code HEAD..p4/master > /dev/null &&
> + cd "$TRASH_DIRECTORY" &&
> + rm -rf "$git" && mkdir "$git"
> +'
> +
> +# What happens with unknown author? Without allowMissingP4Users it should fail.
> +test_expect_success 'preserve user where author is unknown to p4' '
> + "$GITP4" clone --dest="$git" //depot &&
> + cd "$git" &&
> + git config git-p4.skipSubmitEditCheck true
> + echo "username-bob: a change by bob" >> file1 &&
> + git commit --author "Bob <bob@localhost>" -m "preserve: a change by bob" file1 &&
> + echo "username-unknown: a change by charlie" >> file1 &&
> + git commit --author "Charlie <charlie@localhost>" -m "preserve: a change by charlie" file1 &&
> + ! P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user &&
> + ! git diff --exit-code HEAD..p4/master > /dev/null &&
> + echo "$0: repeat with allowMissingP4Users enabled" &&
> + git config git-p4.allowMissingP4Users true &&
> + git config git-p4.preserveUser true &&
> + P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit &&
> + git diff --exit-code HEAD..p4/master > /dev/null &&
> + p4_check_commit_author file1 alice &&
> + cd "$TRASH_DIRECTORY" &&
> + rm -rf "$git" && mkdir "$git"
> +'
> +
> test_expect_success 'shutdown' '
> pid=`pgrep -f p4d` &&
> test -n "$pid" &&
> --
> 1.7.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-04-23 11:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-04-21 19:50 [PATCH/RFC v2] git-p4: add option to preserve user names Luke Diamand
2011-04-21 19:50 ` [PATCH] " Luke Diamand
2011-04-23 11:54 ` Pete Wyckoff
-- strict thread matches above, loose matches on Subject: below --
2011-04-19 18:01 Luke Diamand
2011-04-19 18:01 ` Luke Diamand
2011-04-20 0:10 ` Pete Wyckoff
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).