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

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,

  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.

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to