On Thu, Mar 28, 2013 at 06:56:36PM +0530, Ramkumar Ramachandra wrote:
> Jeff King (1):
> t5516 (fetch-push): drop implicit arguments from helper functions
>
> Ramkumar Ramachandra (5):
> remote.c: simplify a bit of code using git_config_string()
> t5516 (fetch-push): update test description
> remote.c: introduce a way to have different remotes for fetch/push
> remote.c: introduce remote.pushdefault
> remote.c: introduce branch.<name>.pushremote
Thanks, this iteration looks pretty good. I have one minor nit, which is
that the tests in patches 5 and 6 do things like:
> + git push &&
> + test_must_fail check_push_result up_repo $the_commit heads/master &&
> + check_push_result down_repo $the_commit heads/master
Using test_must_fail here caught my eye, because usually it is meant to
check failure of a single git command. When it (or "!", for that matter)
is used with a compound function, you end up losing robustness in corner
cases.
For example, imagine your function is:
check_foo() {
cd "$1" &&
git foo
}
and you expect in some cases that "git foo" will succeed and in some
cases it will fail. In the affirmative case (running "check_foo"), this
is robust; if any of the steps fails, the test fails.
But if you run the negative case ("test_must_fail check_foo"), you will
also fail if any of the preparatory steps fail. I.e., you wanted to say:
cd "$1" && test_must_fail git foo
but you actually said (applying De Morgan's laws):
test_must_fail cd "$1" || test_must_fail git foo
Now we probably don't expect the "cd" to fail here, but of course the
other steps can be more complicated, too. In your case, I think the
effect you are looking for is that "heads/master != $the_commit". But
note that we would also fail if "git fsck" fails in the pushed
repository, which is not what we want.
Sometimes it's annoyingly verbose to break down a compound function. But
I think in this case, you can make your tests more robust by just
checking the affirmative that the ref is still where we expect it to be,
like:
check_push_result up_repo $the_first_commit heads/master
Sorry if that was a bit long-winded. I think that practically speaking,
it is not a likely source of problems in this case. But it's an
anti-pattern in our tests that I think is worth mentioning.
-Peff
--
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