Re: [PATCH v4 0/6] Support triangular workflows
Jeff King wrote: [...] So, you're saying: don't test compound statements for failure, since anything in the chain could fail and propagate failure. I should only test simple git-foo commands for failure? 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 Doesn't that change the meaning of the test though? I really like how the original tests read. -- 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
Re: [PATCH v4 0/6] Support triangular workflows
Junio C Hamano wrote: Jeff King p...@peff.net writes: [...] Thanks. That is one of the reasons why we do not want to see too many custom test helper functions. I noticed that you queued my original series without modification in rr/triangle. Should I submit a re-roll with Peff's suggestion incorporated? -- 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
Re: [PATCH v4 0/6] Support triangular workflows
On Mon, Apr 01, 2013 at 02:21:22AM +0530, Ramkumar Ramachandra wrote: Jeff King wrote: [...] So, you're saying: don't test compound statements for failure, since anything in the chain could fail and propagate failure. I should only test simple git-foo commands for failure? Right. 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 Doesn't that change the meaning of the test though? I really like how the original tests read. Does it? I thought the original was: test_must_fail check_push_result up_repo $the_commit heads/master which is checking that we did _not_ push $the_commit to up_repo. Checking that without a negative means confirming that what _used_ to be there is still there, which is $the_first_commit. But I didn't actually run it, so I might be wrong about what is supposed to be there after the (lack of) push. -Peff -- 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
Re: [PATCH v4 0/6] Support triangular workflows
Jeff King p...@peff.net writes: On Mon, Apr 01, 2013 at 02:21:22AM +0530, Ramkumar Ramachandra wrote: Jeff King wrote: [...] So, you're saying: don't test compound statements for failure, since anything in the chain could fail and propagate failure. I should only test simple git-foo commands for failure? Right. I think another thing to keep in mind is that test_must_fail is used only to check for an expected failure from git-foo commands, not for a random command (or sequence of commands). The primary point of test_must_fail is not to declare _any_ failure as Oh, good, I wanted see it to fail and it returned non-zero exit status so I am happy, but exclude uncontrolled failures, by saying Yuck, it returned non-zero exit status, but it died by signal (e.g. SEGV), which is not the way I wanted to see it die. -- 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
Re: [PATCH v4 0/6] Support triangular workflows
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: Jeff King p...@peff.net writes: [...] Thanks. That is one of the reasons why we do not want to see too many custom test helper functions. I noticed that you queued my original series without modification in rr/triangle. Should I submit a re-roll with Peff's suggestion incorporated? If you want the topic to make progress, yes. The only reason a topic is queued in 'pu' is to let me not pay attention to it, without risking to forget about it completely ;-). The topics on 'pu' have potential to be a useful change even though they are far from ready for 'next'. By queuing on 'pu', rather than just letting them sit in the mail archive and relying on the author to send follow-ups, I can send out what happened to this topic? inquiries from time to time, without paying constant attention to them. -- 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
Re: [PATCH v4 0/6] Support triangular workflows
Junio C Hamano gits...@pobox.com writes: The only reason a topic is queued in 'pu' is to let me not pay attention to it, without risking to forget about it completely ;-). The topics on 'pu' have potential to be a useful change even though they are far from ready for 'next'. That's not even though but should have been even when. Sometimes when I feel a topic is already of 'next' quality, the author suggests that a reroll is coming, and I hold such a topic off. Also, inverse is not true, as always. Some topics may not be queued in 'pu' when I push a day's integration results out, but that does not mean they do not have potential to be useful. They may not be in 'pu' because I didn't have enough time to get around to them, or because there was discussion ongoing and I saw the author was actively responding to the comments, relieving me from having to keep an eye on the topic at all until the dust settled without even queueing it on 'pu'. -- 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
Re: [PATCH v4 0/6] Support triangular workflows
Junio C Hamano gits...@pobox.com writes: Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: Jeff King p...@peff.net writes: [...] Thanks. That is one of the reasons why we do not want to see too many custom test helper functions. I noticed that you queued my original series without modification in rr/triangle. Should I submit a re-roll with Peff's suggestion incorporated? If you want the topic to make progress, yes. By the way, this series seems to break a few tests in the test suite, both as a standalone topic branch and as part of the 'pu'. -- 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
Re: [PATCH v4 0/6] Support triangular workflows
Junio C Hamano gits...@pobox.com writes: Junio C Hamano gits...@pobox.com writes: Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: Jeff King p...@peff.net writes: [...] Thanks. That is one of the reasons why we do not want to see too many custom test helper functions. I noticed that you queued my original series without modification in rr/triangle. Should I submit a re-roll with Peff's suggestion incorporated? If you want the topic to make progress, yes. By the way, this series seems to break a few tests in the test suite,... I suspect this could be interaction with push-default change near the tip of 'pu'. Setting push.default explicitly to matching in the test may be necessary. Also the t5516 is involved in in-flight churns, so there could be some merge mixups. -- 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
[PATCH v4 0/6] Support triangular workflows
Hi, The changes in this round are: 1. Peff submitted a patch to squash into [3/6]. Since his patch essentially reverts mine, I've blamed him for the change. 2. Peff suggested a code movement in [5/6] to make things flow more naturally. 3. Jonathan suggested a better test description in [2/6]. 4. Junio suggested a minor documentation update in [6/6]. My build of git has had this feature for two weeks now (since the first iteration), and I'm very happy with it. 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 Documentation/config.txt | 24 +++- builtin/push.c | 2 +- remote.c | 41 -- remote.h | 1 + t/t5516-fetch-push.sh| 316 +++ 5 files changed, 238 insertions(+), 146 deletions(-) -- 1.8.2.141.g3797f84 -- 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
Re: [PATCH v4 0/6] Support triangular workflows
Ramkumar Ramachandra artag...@gmail.com writes: The changes in this round are: 1. Peff submitted a patch to squash into [3/6]. Since his patch essentially reverts mine, I've blamed him for the change. 2. Peff suggested a code movement in [5/6] to make things flow more naturally. 3. Jonathan suggested a better test description in [2/6]. 4. Junio suggested a minor documentation update in [6/6]. My build of git has had this feature for two weeks now (since the first iteration), and I'm very happy with it. Will take a look; thanks for a reroll. We may need a bit of adjustment to the longer term plan for the push.default topic, as I expect we will have this feature a lot sooner (e.g. perhaps in 1.8.4) than we will switch the push default to simple. The description of simple and upstream still is written around the you push to and pull from the same place, and may need to be updated to explain what happens if you are employing a triangular workflow? Or it could turn out that triangular people may be better served by a push.default different from simple or upstream. Or it may turn out that we do not need any change to what is queued on the push-2.0-default-to-simple topic (I haven't thought things through). It is not urgent, but please start thinking about how you can help, now you introduced the triangular stuff. Thanks. 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 Documentation/config.txt | 24 +++- builtin/push.c | 2 +- remote.c | 41 -- remote.h | 1 + t/t5516-fetch-push.sh| 316 +++ 5 files changed, 238 insertions(+), 146 deletions(-) -- 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
Re: [PATCH v4 0/6] Support triangular workflows
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/6] Support triangular workflows
Jeff King p...@peff.net writes: 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. Thanks. That is one of the reasons why we do not want to see too many custom test helper functions. -- 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
