Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

2014-02-28 Thread Jeff King
On Fri, Feb 28, 2014 at 04:28:38PM +0900, Brian Gesiak wrote: I would be in favor of using test_i18ngrep, but it seems like this test file overwhelmingly uses test_(i18n)cmp, even when inspecting stderr output. We generally prefer cmp checks to grep checks, because they are more rigorous.

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

2014-02-28 Thread Brian Gesiak
If you feel like continuing on this series, converting the warning() into a die() would be a much more productive use of time (but if you don't, I do not see any reason not to take the patches you've posted). I'd be happy to keep working on this. In fact, I think I have a patch for this ready.

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

2014-02-28 Thread Jeff King
On Fri, Feb 28, 2014 at 07:44:10PM +0900, Brian Gesiak wrote: I notice that the warning comes from install_branch_config, which gets used both for branch -u, but also in the side effect case I mentioned above. Is it possible to trigger this as part of such a case? I think maybe git branch

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

2014-02-28 Thread Brian Gesiak
I just don't want to regress somebody else's workflow due to my lack of imagination. This makes a lot of sense to me, although as-is the function emits a warning and returns immediately (although with a successful status code), so I'm also stumped as to what kind of workflow this would be

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

2014-02-28 Thread Jeff King
On Fri, Feb 28, 2014 at 08:16:13PM +0900, Brian Gesiak wrote: I just don't want to regress somebody else's workflow due to my lack of imagination. This makes a lot of sense to me, although as-is the function emits a warning and returns immediately (although with a successful status

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

2014-02-28 Thread Matthieu Moy
Jeff King p...@peff.net writes: Don't die to let the caller finish its job in such case. [...] Matthieu, can you remember anything else that led to that decision? Not at all, unfortunately. I don't remember if I did that in case there's something like some cleanup to do or because I

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

2014-02-28 Thread brian m. carlson
On Fri, Feb 28, 2014 at 01:27:15AM -0500, Jeff King wrote: On Fri, Feb 28, 2014 at 03:17:28PM +0900, Brian Gesiak wrote: Patch is on the way, just waiting for the tests to complete. Thanks for pointing that out! Also, sorry if it's in the Makefile somewhere, but is there an easy way to run

[PATCH 1/2] t3200-branch: test setting branch as own upstream

2014-02-27 Thread Brian Gesiak
From: modocache modoca...@gmail.com No test asserts that git branch -u refs/heads/my-branch my-branch emits a warning. Add a test that does so. Signed-off-by: Brian Gesiak modoca...@gmail.com --- t/t3200-branch.sh | 8 1 file changed, 8 insertions(+) diff --git a/t/t3200-branch.sh

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

2014-02-27 Thread Jeff King
On Fri, Feb 28, 2014 at 12:04:18PM +0900, Brian Gesiak wrote: No test asserts that git branch -u refs/heads/my-branch my-branch emits a warning. Add a test that does so. For an operation like git branch foo origin where setting up the tracking is a side effect, a warning makes sense. But the

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

2014-02-27 Thread Brian Gesiak
For an operation like git branch foo origin where setting up the tracking is a side effect, a warning makes sense. But the sole purpose of the command above is to set the upstream, and we didn't do it; should this warning actually be upgraded to an error? I agree. I originally wrote the test

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

2014-02-27 Thread Jeff King
On Fri, Feb 28, 2014 at 03:17:28PM +0900, Brian Gesiak wrote: For an operation like git branch foo origin where setting up the tracking is a side effect, a warning makes sense. But the sole purpose of the command above is to set the upstream, and we didn't do it; should this warning

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

2014-02-27 Thread Johannes Sixt
Am 2/28/2014 6:37, schrieb Jeff King: On Fri, Feb 28, 2014 at 12:04:18PM +0900, Brian Gesiak wrote: No test asserts that git branch -u refs/heads/my-branch my-branch emits a warning. Add a test that does so. For an operation like git branch foo origin where setting up the tracking is a

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

2014-02-27 Thread Jeff King
On Fri, Feb 28, 2014 at 07:55:25AM +0100, Johannes Sixt wrote: This should use test_i18ncmp, as the string you are matching is internationalized. More generally, stderr output shouldn't be tested with test_cmp or test_i18ncmp at all, but with grep and test_i18ngrep. The reason is that

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

2014-02-27 Thread Jeff King
On Fri, Feb 28, 2014 at 02:14:01AM -0500, Jeff King wrote: I didn't think we bothered to make sh -x work robustly. I don't mind if we do, but git grep -E 'test_(i18n)?cmp .*err shows many potential problem spots. Just for fun: cd t make SHELL_PATH=sh -x prove causes 326 test failures

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

2014-02-27 Thread Brian Gesiak
Hmm. Looks like it is only a problem if you are calling a shell function (since it is the shell function's trace output you are seeing). So this test would be OK as-is Indeed, this test passes when run locally, even using sh -x. I would be in favor of using test_i18ngrep, but it seems like

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

2014-02-27 Thread Johannes Sixt
Am 2/28/2014 8:14, schrieb Jeff King: I didn't think we bothered to make sh -x work robustly. I don't mind if we do, but git grep -E 'test_(i18n)?cmp .*err shows many potential problem spots. Hmm. Looks like it is only a problem if you are calling a shell function (since it is the shell