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

[PATCH/RFC v5] 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 Peter Wyckoff.

Compared to v4, adds better cd'ing for the authorship warning test
as suggested by Junio. There's a second patch to come that fixes all the
cd'ing in the git-p4 test script. Without this, if a test bails out,
subsequent tests will be in the wrong directory.

I didn't put the "-q" into the grep commands as it makes python
explode; instead they now redirect to /dev/null. I guess this is a
bug that needs to be investigated at some point...

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              |   47 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+), 2 deletions(-)

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

Acked-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              |   47 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 85 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..888ad54 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,44 @@ 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" > /dev/null) &&
+	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" > /dev/null) &&
+	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" > /dev/null) &&
+	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" > /dev/null) &&
+	git diff --exit-code HEAD..p4/master &&
+
+	p4_check_commit_author usernamefile3 alice
+	) &&
+	rm -rf "$git" && mkdir "$git"
+'
+
+
 test_expect_success 'shutdown' '
 	pid=`pgrep -f p4d` &&
 	test -n "$pid" &&
-- 
1.7.1

Re: [PATCH/RFC v5] 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..888ad54 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"
> +}

This should be:

        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"
        }

Points to keep in mind:

 - An assignment is just an ordinary stmt. Make it a habit to keep it as
   part of && chain without having to even think about it, so that you
   won't forget to do so when you need to assign in the middle of a
   sequence.

 - Recent bash seems to give unnecessary warning to redirecting to a
   file whose name is given as a variable. Use a(n unnecessary) double
   quotes to avoid it.

 - Our coding convention is not to have an extra SP after redirection.

 - Double quote your variables where syntactically necessary to avoid them
   from getting split by the shell when they contain IFS whitespaces.

> +# 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" &&

I'd prefer to see this block inside the "(" ... ")" indented one level
deeper, i.e.

	"$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" > /dev/null) &&

Are you expecting this '"$GITP4" commit' to succeed (i.e. exit with 0
status), or to fail (i.e. exit with non-zero status)?  By having the
command on the upstream side of a pipe, you cannot check its exit status.

Either

	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
        grep "git author ..." actual &&

or if you expect the '"$GITP4" commit' to fail:

	! P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
        grep "git author ..." actual &&

Do you really need to discard the output from grep?  When the test is run
without "-v", the output would not be shown, and when it is run with "-v",
showing the output would help you diagnose bugs in the test, no?

If you indeed need to squelch grep, "grep -q" should be usable.  It is
used in many other tests.

I also do not see a need for using a subshell for this case, either.

> +	git diff --exit-code HEAD..p4/master &&

Is it possibile for the working tree files to have changed from p4/master,
and if so is it an error you may want to catch?  Your answer could be "No,
$GITP4 never touches the working tree, so that is unnecessary to check."

> +	make_change_by_user usernamefile3 Charlie charlie@localhost &&
> +	(P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |
> +		grep "git author charlie@localhost does not match" > /dev/null) &&
> +	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" > /dev/null) &&

Again, either

	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
        ! grep "does not match" actual

or

	! P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
        ! grep "does not match" actual

depending on what exit status you are expecting from '"$GITP4" commit'.