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

From: Jeff King <peff@peff.net>
Date: Mar 28, 2013, 15:35 UTC
Message-ID: <20130328153547.GA3337@sigill.intra.peff.net>
In-Reply-To: <1364477202-5742-1-git-send-email-artagnon@gmail.com>

On Thu, Mar 28, 2013 at 06:56:36PM +0530, Ramkumar Ramachandra wrote:

Show 9 quoted lines

> 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