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

From: Junio C Hamano <gitster@pobox.com>
Date: May 13, 2011, 05:29 UTC
Message-ID: <7vaaerb224.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1305176756-27046-2-git-send-email-luke@diamand.org>

Luke Diamand <luke@diamand.org> writes:

Show 16 quoted lines

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

Show 8 quoted lines

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

Show 6 quoted lines

> +	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."

Show 8 quoted lines

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