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

Luke Diamand [off-list ref] writes:

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'.