Luke Diamand <luke@xxxxxxxxxxx> 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'.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html