Re: [PATCH v4 0/6] Support triangular workflows

[Jeff King <peff@peff.net>]

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