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

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

[Luke Diamand]

Updated version of git-p4 authorship warning suggested by Junio,
incorporating fixes for problems in v2 pointed out by Pete Wyckoff.

Luke Diamand (1):
  git-p4: warn if git authorship won't be retained

 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(-)

[PATCH] 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.

Requested-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Pete Wyckoff <pw@padd.com>
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..021b985 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 message.\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 self.preserveUser or (gitConfig("git-p4.skipUserNameCheck") == "true"):
+            self.checkAuthorship = False
+        else:
+            self.checkAuthorship = True
+
         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..d6c7d13 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] git-p4: warn if git authorship won't be retained

[Luke Diamand]

On Tue, May 10, 2011 at 8:27 PM, Luke Diamand <luke@diamand.org> wrote:
> 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.
>

Of course, I've forgotten the && in the test case.

Sigh.

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

[Junio C Hamano]

Luke Diamand <luke@diamand.org> writes:

> diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
> index 4fb0e24..d6c7d13 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

	file=$1 name=$2 email=$3 &&

> +	echo "username: a change by $name" >> $file &&
> ...
> @@ -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 &&
> + ...
> +	p4_check_commit_author usernamefile3 alice &&
> +	cd "$TRASH_DIRECTORY" &&
> +	rm -rf "$git" && mkdir "$git"
> +'

When the commands in the && chain fails after 'cd "$git"' near the top but
not before 'cd "$TRASH_DIRECTORY"' near the end, you would end up starting
the next test from somewhere different from "$TRASH_DIRECTORY".

Do it like this instead:

	"$GITP4" clone --dest="$git" //depot &&
        (
        	cd "$git" &&
                ...
	) &&
        rm -fr "$git"

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

[Junio C Hamano]

Luke Diamand <luke@diamand.org> writes:

> 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.
>
> Requested-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Pete Wyckoff <pw@padd.com>
> Signed-off-by: Luke Diamand <luke@diamand.org>

Heh, I didn't really "request" it; I am not a git-p4 user and am in no
position to judge if such a feature is useful.  If you felt it is a good
thing to do, please take full the credit (no need to resend; I am not
complaining).

Thanks.

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

[Luke Diamand]

On Tue, May 10, 2011 at 11:51 PM, Junio C Hamano <gitster@pobox.com> wrote:


<snip>

>
>> + ...
>> +     p4_check_commit_author usernamefile3 alice &&
>> +     cd "$TRASH_DIRECTORY" &&
>> +     rm -rf "$git" && mkdir "$git"
>> +'
>
> When the commands in the && chain fails after 'cd "$git"' near the top but
> not before 'cd "$TRASH_DIRECTORY"' near the end, you would end up starting
> the next test from somewhere different from "$TRASH_DIRECTORY".
>
> Do it like this instead:
>
>        "$GITP4" clone --dest="$git" //depot &&
>        (
>                cd "$git" &&
>                ...
>        ) &&
>        rm -fr "$git"
>

Thanks - that might explain a few oddities I had when testing.

There's a few others of those in the earlier tests; I'll resend this
patch, and send a second patch to fix those if that's ok.

Regards!
Luke