{"thread":{"id":"35985","subject":"[PATCH 1/2] t3200-branch: test setting branch as own upstream","startedAt":"2014-02-28T03:04:18Z","lastAt":"2014-03-03T19:54:32Z","messageCount":22,"participants":["Brian Gesiak","Jeff King","Johannes Sixt","Matthieu Moy","brian m. carlson","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"235533","messageId":"1393556659-32717-1-git-send-email-modocache@gmail.com","threadId":"35985","inReplyTo":null,"subject":"[PATCH 1/2] t3200-branch: test setting branch as own upstream","fromName":"Brian Gesiak","fromEmail":"modocache@gmail.com","sentAt":"2014-02-28T03:04:18Z","receivedAt":"2014-02-28T03:04:18Z","isPatch":true,"sender":{"key":"modocache@gmail.com","avatar":"https://avatars.githubusercontent.com/u/552921?v=4"},"body":"From: modocache \n\nNo test asserts that \"git branch -u refs/heads/my-branch my-branch\"\nemits a warning. Add a test that does so.\n\nSigned-off-by: Brian Gesiak \n---\n t/t3200-branch.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex fcdb867..f70b96f 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -507,6 +507,14 @@ EOF\n \ttest_cmp expected actual\n '\n \n+test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' '\n+\tgit branch --set-upstream-to refs/heads/my13 my13 2>actual &&\n+\tcat >expected <expect < 1117150200 +0000\tbranch: Created from master\n-- \n1.8.3.4 (Apple Git-47)\n"},{"id":"235534","messageId":"1393556659-32717-2-git-send-email-modocache@gmail.com","threadId":"35985","inReplyTo":"1393556659-32717-1-git-send-email-modocache@gmail.com","subject":"[PATCH 2/2] branch: use skip_prefix","fromName":"Brian Gesiak","fromEmail":"modocache@gmail.com","sentAt":"2014-02-28T03:04:19Z","receivedAt":"2014-02-28T03:04:19Z","isPatch":true,"sender":{"key":"modocache@gmail.com","avatar":"https://avatars.githubusercontent.com/u/552921?v=4"},"body":"From: modocache \n\nThe install_branch_config function reimplemented the skip_prefix\nfunction inline. Use skip_prefix function instead for brevity.\n\nSigned-off-by: Brian Gesiak \nReported-by: Michael Haggerty \n---\n branch.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..e163f3c 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -1,3 +1,4 @@\n+#include \"git-compat-util.h\"\n #include \"cache.h\"\n #include \"branch.h\"\n #include \"refs.h\"\n@@ -49,12 +50,11 @@ static int should_setup_rebase(const char *origin)\n \n void install_branch_config(int flag, const char *local, const char *origin, const char *remote)\n {\n-\tconst char *shortname = remote + 11;\n-\tint remote_is_branch = starts_with(remote, \"refs/heads/\");\n+\tconst char *shortname = skip_prefix(remote, \"refs/heads/\");\n \tstruct strbuf key = STRBUF_INIT;\n \tint rebasing = should_setup_rebase(origin);\n \n-\tif (remote_is_branch\n+\tif (shortname\n \t && !strcmp(local, shortname)\n \t && !origin) {\n \t\twarning(_(\"Not setting branch %s as its own upstream.\"),\n@@ -77,29 +77,29 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n \tstrbuf_release(&key);\n \n \tif (flag & BRANCH_CONFIG_VERBOSE) {\n-\t\tif (remote_is_branch && origin)\n+\t\tif (shortname && origin)\n \t\t\tprintf_ln(rebasing ?\n \t\t\t\t _(\"Branch %s set up to track remote branch %s from %s by rebasing.\") :\n \t\t\t\t _(\"Branch %s set up to track remote branch %s from %s.\"),\n \t\t\t\t local, shortname, origin);\n-\t\telse if (remote_is_branch && !origin)\n+\t\telse if (shortname && !origin)\n \t\t\tprintf_ln(rebasing ?\n \t\t\t\t _(\"Branch %s set up to track local branch %s by rebasing.\") :\n \t\t\t\t _(\"Branch %s set up to track local branch %s.\"),\n \t\t\t\t local, shortname);\n-\t\telse if (!remote_is_branch && origin)\n+\t\telse if (!shortname && origin)\n \t\t\tprintf_ln(rebasing ?\n \t\t\t\t _(\"Branch %s set up to track remote ref %s by rebasing.\") :\n \t\t\t\t _(\"Branch %s set up to track remote ref %s.\"),\n \t\t\t\t local, remote);\n-\t\telse if (!remote_is_branch && !origin)\n+\t\telse if (!shortname && !origin)\n \t\t\tprintf_ln(rebasing ?\n \t\t\t\t _(\"Branch %s set up to track local ref %s by rebasing.\") :\n \t\t\t\t _(\"Branch %s set up to track local ref %s.\"),\n \t\t\t\t local, remote);\n \t\telse\n-\t\t\tdie(\"BUG: impossible combination of %d and %p\",\n-\t\t\t remote_is_branch, origin);\n+\t\t\tdie(\"BUG: impossible combination of %p and %p\",\n+\t\t\t shortname, origin);\n \t}\n }\n \n-- \n1.8.3.4 (Apple Git-47)\n"},{"id":"235537","messageId":"20140228053703.GA32556@sigill.intra.peff.net","threadId":"35985","inReplyTo":"1393556659-32717-1-git-send-email-modocache@gmail.com","subject":"Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-28T05:37:03Z","receivedAt":"2014-02-28T05:37:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 28, 2014 at 12:04:18PM +0900, Brian Gesiak wrote:\n\n> No test asserts that \"git branch -u refs/heads/my-branch my-branch\"\n> emits a warning. Add a test that does so.\n\nFor an operation like \"git branch foo origin\" where setting up the\ntracking is a side effect, a warning makes sense. But the sole purpose\nof the command above is to set the upstream, and we didn't do it; should\nthis warning actually be upgraded to an error?\n\n> +test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' '\n> +\tgit branch --set-upstream-to refs/heads/my13 my13 2>actual &&\n> +\tcat >expected < +warning: Not setting branch my13 as its own upstream.\n> +EOF\n> +\ttest_cmp expected actual\n> +'\n\nThis should use test_i18ncmp, as the string you are matching is\ninternationalized.\n\n-Peff\n"},{"id":"235538","messageId":"20140228054613.GB32556@sigill.intra.peff.net","threadId":"35985","inReplyTo":"1393556659-32717-2-git-send-email-modocache@gmail.com","subject":"Re: [PATCH 2/2] branch: use skip_prefix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-28T05:46:14Z","receivedAt":"2014-02-28T05:46:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 28, 2014 at 12:04:19PM +0900, Brian Gesiak wrote:\n\n> From: modocache \n\nBoth your emailed patches have this, which is due to your author name\nnot matching your sending identity. You probably want to set user.name,\nor if you already have (which it looks like you might have from your\nSigned-off-by), use \"git commit --amend --reset-author\" to update the\nauthor information.\n\n> The install_branch_config function reimplemented the skip_prefix\n> function inline. Use skip_prefix function instead for brevity.\n> \n> Signed-off-by: Brian Gesiak \n> Reported-by: Michael Haggerty \n\nIt's a minor thing, but usually these footer lines try to follow a\nchronological order. So the report would come before the signoff (and a\nfurther signoff from the maintainer would go after yours).\n\n> diff --git a/branch.c b/branch.c\n> index 723a36b..e163f3c 100644\n> --- a/branch.c\n> +++ b/branch.c\n> [...]\n\nThe patch itself looks OK to me.\n\n-Peff\n"},{"id":"235540","messageId":"CAN7MxmWP9N==0DnoE-0=Xr7NWkNMSGBC+yiz1a3wS5EbHigvKg@mail.gmail.com","threadId":"35985","inReplyTo":"20140228053703.GA32556@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream","fromName":"Brian Gesiak","fromEmail":"modocache@gmail.com","sentAt":"2014-02-28T06:17:28Z","receivedAt":"2014-02-28T06:17:28Z","isPatch":true,"sender":{"key":"modocache@gmail.com","avatar":"https://avatars.githubusercontent.com/u/552921?v=4"},"body":"> For an operation like \"git branch foo origin\" where setting up the\n> tracking is a side effect, a warning makes sense. But the sole purpose\n> of the command above is to set the upstream, and we didn't do it; should\n> this warning actually be upgraded to an error?\n\nI agree. I originally wrote the test using test_expect_failure--imagine my\nsurprise when the exit status was 0, despite the fact that the upstream wasn't\nset!\n\n> This should use test_i18ncmp, as the string you are matching is\n> internationalized.\n\nPatch is on the way, just waiting for the tests to complete. Thanks for pointing\nthat out! Also, sorry if it's in the Makefile somewhere, but is there\nan easy way\nto run just a single test file in the t directory?\n\n- Brian Gesiak\n"},{"id":"235541","messageId":"20140228062715.GE32556@sigill.intra.peff.net","threadId":"35985","inReplyTo":"CAN7MxmWP9N==0DnoE-0=Xr7NWkNMSGBC+yiz1a3wS5EbHigvKg@mail.gmail.com","subject":"Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-28T06:27:15Z","receivedAt":"2014-02-28T06:27:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 28, 2014 at 03:17:28PM +0900, Brian Gesiak wrote:\n\n> > For an operation like \"git branch foo origin\" where setting up the\n> > tracking is a side effect, a warning makes sense. But the sole purpose\n> > of the command above is to set the upstream, and we didn't do it; should\n> > this warning actually be upgraded to an error?\n> \n> I agree. I originally wrote the test using test_expect_failure--imagine my\n> surprise when the exit status was 0, despite the fact that the upstream wasn't\n> set!\n\nFor reference, you don't want test_expect_failure here; it is about \"we\nwant this to succeed, but git is currently buggy and we know it, so mark\nit as a failing test\". You want test_must_fail to indicate a command\ninside a test that must exit non-zero:\n\n test_expect_success 'pointing upstream to itself fails' '\n test_must_fail git branch -u ...\n '\n\nI notice that the warning comes from install_branch_config, which gets\nused both for \"branch -u\", but also in the \"side effect\" case I\nmentioned above. Is it possible to trigger this as part of such a case?\nI think maybe \"git branch -f --track foo foo\" would do it. If so, we\nshould perhaps include a test that it does not break if we upgrade the\n\"-u\" case to an error.\n\n> Patch is on the way, just waiting for the tests to complete. Thanks\n> for pointing that out! Also, sorry if it's in the Makefile somewhere,\n> but is there an easy way to run just a single test file in the t\n> directory?\n\nYou can run \"./tXXXX-....sh\" explicitly.\n\n-Peff\n"},{"id":"235546","messageId":"531032DD.9000904@viscovery.net","threadId":"35985","inReplyTo":"20140228053703.GA32556@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-02-28T06:55:25Z","receivedAt":"2014-02-28T06:55:25Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 2/28/2014 6:37, schrieb Jeff King:\n> On Fri, Feb 28, 2014 at 12:04:18PM +0900, Brian Gesiak wrote:\n> \n>> No test asserts that \"git branch -u refs/heads/my-branch my-branch\"\n>> emits a warning. Add a test that does so.\n> \n> For an operation like \"git branch foo origin\" where setting up the\n> tracking is a side effect, a warning makes sense. But the sole purpose\n> of the command above is to set the upstream, and we didn't do it; should\n> this warning actually be upgraded to an error?\n> \n>> +test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' '\n>> +\tgit branch --set-upstream-to refs/heads/my13 my13 2>actual &&\n>> +\tcat >expected <> +warning: Not setting branch my13 as its own upstream.\n>> +EOF\n>> +\ttest_cmp expected actual\n>> +'\n> \n> This should use test_i18ncmp, as the string you are matching is\n> internationalized.\n\nMore generally, stderr output shouldn't be tested with test_cmp or\ntest_i18ncmp at all, but with grep and test_i18ngrep. The reason is that\nwhen you run the test with 'sh -x t3200* -v -i', the trace output is also\nin stderr, and the test_cmp/test_i18ncmp fails due to the unexpected extra\ntext.\n\n-- Hannes\n"},{"id":"235548","messageId":"20140228071401.GA1229@sigill.intra.peff.net","threadId":"35985","inReplyTo":"531032DD.9000904@viscovery.net","subject":"Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-28T07:14:01Z","receivedAt":"2014-02-28T07:14:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 28, 2014 at 07:55:25AM +0100, Johannes Sixt wrote:\n\n> > This should use test_i18ncmp, as the string you are matching is\n> > internationalized.\n> \n> More generally, stderr output shouldn't be tested with test_cmp or\n> test_i18ncmp at all, but with grep and test_i18ngrep. The reason is that\n> when you run the test with 'sh -x t3200* -v -i', the trace output is also\n> in stderr, and the test_cmp/test_i18ncmp fails due to the unexpected extra\n> text.\n\nI didn't think we bothered to make \"sh -x\" work robustly. I don't mind\nif we do, but \"git grep -E 'test_(i18n)?cmp .*err\" shows many potential\nproblem spots.\n\nHmm. Looks like it is only a problem if you are calling a shell function\n(since it is the shell function's trace output you are seeing). So this\ntest would be OK as-is, but testing for an error, like:\n\n test_must_fail git branch -u foo foo 2>stderr\n\nwould not be, because we see the trace from test_must_fail. So some of\nthe callsites found by my grep are actually probably fine.\n\n-Peff\n"},{"id":"235549","messageId":"20140228072606.GA622@sigill.intra.peff.net","threadId":"35985","inReplyTo":"20140228071401.GA1229@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-28T07:26:06Z","receivedAt":"2014-02-28T07:26:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 28, 2014 at 02:14:01AM -0500, Jeff King wrote:\n\n> I didn't think we bothered to make \"sh -x\" work robustly. I don't mind\n> if we do, but \"git grep -E 'test_(i18n)?cmp .*err\" shows many potential\n> problem spots.\n\nJust for fun:\n\n cd t\n make SHELL_PATH=\"sh -x\" prove\n\ncauses 326 test failures across 43 scripts. That's slightly misleading,\nbecause 200 of the failures are all in t0008, and updating one function\nwould probably clear up all of them.\n\n-Peff\n"},{"id":"235550","messageId":"CAN7MxmXOvkrWPDEUH_Bqz5RVZUS7kuWkwi9kWmdwuwySszd=YA@mail.gmail.com","threadId":"35985","inReplyTo":"20140228072606.GA622@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream","fromName":"Brian Gesiak","fromEmail":"modocache@gmail.com","sentAt":"2014-02-28T07:28:38Z","receivedAt":"2014-02-28T07:28:38Z","isPatch":true,"sender":{"key":"modocache@gmail.com","avatar":"https://avatars.githubusercontent.com/u/552921?v=4"},"body":"> Hmm. Looks like it is only a problem if you are calling a shell function\n> (since it is the shell function's trace output you are seeing). So this\n> test would be OK as-is\n\nIndeed, this test passes when run locally, even using \"sh -x\".\n\nI would be in favor of using test_i18ngrep, but it seems like this\ntest file overwhelmingly uses test_(i18n)cmp, even when inspecting\nstderr output. Making double-sure that all tests pass when run with\n\"sh -x\" seems like a larger endeavor.\n\nOf course, I'd be happy to submit several patches if there's support\nfor such a change. But as Peff points out it will be a large diff.\n\n- Brian Gesiak\n\nOn Fri, Feb 28, 2014 at 4:26 PM, Jeff King wrote:\n> On Fri, Feb 28, 2014 at 02:14:01AM -0500, Jeff King wrote:\n>\n>> I didn't think we bothered to make \"sh -x\" work robustly. I don't mind\n>> if we do, but \"git grep -E 'test_(i18n)?cmp .*err\" shows many potential\n>> problem spots.\n>\n> Just for fun:\n>\n> cd t\n> make SHELL_PATH=\"sh -x\" prove\n>\n> causes 326 test failures across 43 scripts. That's slightly misleading,\n> because 200 of the failures are all in t0008, and updating one function\n> would probably clear up all of them.\n>\n> -Peff\n"},{"id":"235553","messageId":"53103D6F.9050602@viscovery.net","threadId":"35985","inReplyTo":"20140228071401.GA1229@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-02-28T07:40:31Z","receivedAt":"2014-02-28T07:40:31Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 2/28/2014 8:14, schrieb Jeff King:\n> I didn't think we bothered to make \"sh -x\" work robustly. I don't mind\n> if we do, but \"git grep -E 'test_(i18n)?cmp .*err\" shows many potential\n> problem spots.\n> \n> Hmm. Looks like it is only a problem if you are calling a shell function\n> (since it is the shell function's trace output you are seeing). So this\n> test would be OK as-is, but testing for an error, like:\n> \n> test_must_fail git branch -u foo foo 2>stderr\n> \n> would not be, because we see the trace from test_must_fail. So some of\n> the callsites found by my grep are actually probably fine.\n\nYeah, your assessment is correct: only shell function output is affected.\n\nSome time (years?) ago, I used to run the tests on Windows with sh -x,\nredirected to a log file to be able to trace intermittent failures. It was\ndistracting to find many false positives. Today, I don't do that anymore,\nand it is not a big deal for me, just like for anybody else ;-)\n\nConsider the topic settled.\n\n-- Hannes\n"},{"id":"235556","messageId":"20140228083718.GA11480@sigill.intra.peff.net","threadId":"35985","inReplyTo":"CAN7MxmXOvkrWPDEUH_Bqz5RVZUS7kuWkwi9kWmdwuwySszd=YA@mail.gmail.com","subject":"Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-28T08:37:19Z","receivedAt":"2014-02-28T08:37:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 28, 2014 at 04:28:38PM +0900, Brian Gesiak wrote:\n\n> I would be in favor of using test_i18ngrep, but it seems like this\n> test file overwhelmingly uses test_(i18n)cmp, even when inspecting\n> stderr output.\n\nWe generally prefer \"cmp\" checks to \"grep\" checks, because they are more\nrigorous. However, when testing human-readable output which may change,\nsometimes being too specific can simply make the tests brittle and\nannoying. Using a forgiving regex that matches keywords can be helpful.\nSo there's definitely some room for judgement.\n\nI think what you posted as v2 looks OK.\n\n> Making double-sure that all tests pass when run with \"sh -x\" seems\n> like a larger endeavor.\n> \n> Of course, I'd be happy to submit several patches if there's support\n> for such a change. But as Peff points out it will be a large diff.\n\nYeah, I don't think it's worth the effort.\n\nIf you feel like continuing on this series, converting the warning()\ninto a die() would be a much more productive use of time (but if you\ndon't, I do not see any reason not to take the patches you've posted).\n\n-Peff\n"},{"id":"235589","messageId":"CAN7MxmUUoz-HGFiTfgpvLpkXhvjBn_dYje7unoHE-Y=k3A0aqg@mail.gmail.com","threadId":"35985","inReplyTo":"20140228083718.GA11480@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream","fromName":"Brian Gesiak","fromEmail":"modocache@gmail.com","sentAt":"2014-02-28T10:44:10Z","receivedAt":"2014-02-28T10:44:10Z","isPatch":true,"sender":{"key":"modocache@gmail.com","avatar":"https://avatars.githubusercontent.com/u/552921?v=4"},"body":"> If you feel like continuing on this series, converting the warning()\n> into a die() would be a much more productive use of time (but if you\n> don't, I do not see any reason not to take the patches you've posted).\n\nI'd be happy to keep working on this. In fact, I think I have a patch\nfor this ready. But just to clarify:\n\n> I notice that the warning comes from install_branch_config, which gets\n> used both for \"branch -u\", but also in the \"side effect\" case I\n> mentioned above. Is it possible to trigger this as part of such a case?\n> I think maybe \"git branch -f --track foo foo\" would do it. If so, we\n> should perhaps include a test that it does not break if we upgrade the\n> \"-u\" case to an error.\n\nDo you mean that install_branch_config should continue to emit a\nwarning in the \"side effect\" case? I'm not sure I agree--how is \"git\nbranch -f --track foo foo\" less erroneous than \"git branch -u foo\nrefs/heads/foo\"? Perhaps I'm missing some insight on how \"--track\" is\nused.\n\nThe tests appear to already cover all instances in which\ninstall_branch_config is called, and bumping the warning to an error\ndoes not cause any test failures.\n\n- Brian Gesiak\n"},{"id":"235591","messageId":"20140228105929.GA18960@sigill.intra.peff.net","threadId":"35985","inReplyTo":"CAN7MxmUUoz-HGFiTfgpvLpkXhvjBn_dYje7unoHE-Y=k3A0aqg@mail.gmail.com","subject":"Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-28T10:59:29Z","receivedAt":"2014-02-28T10:59:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 28, 2014 at 07:44:10PM +0900, Brian Gesiak wrote:\n\n> > I notice that the warning comes from install_branch_config, which gets\n> > used both for \"branch -u\", but also in the \"side effect\" case I\n> > mentioned above. Is it possible to trigger this as part of such a case?\n> > I think maybe \"git branch -f --track foo foo\" would do it. If so, we\n> > should perhaps include a test that it does not break if we upgrade the\n> > \"-u\" case to an error.\n> \n> Do you mean that install_branch_config should continue to emit a\n> warning in the \"side effect\" case? I'm not sure I agree--how is \"git\n> branch -f --track foo foo\" less erroneous than \"git branch -u foo\n> refs/heads/foo\"? Perhaps I'm missing some insight on how \"--track\" is\n> used.\n\nI'd be more worried about triggering it via the config. E.g.:\n\n git config branch.autosetupmerge always\n git branch -f foo foo\n\nShould the second command die? I admit I'm having a hard time coming up\nwith a feasible reason why anyone would do \"branch -f foo foo\" in the\nfirst place. I just don't want to regress somebody else's workflow due\nto my lack of imagination.\n\n-Peff\n"},{"id":"235593","messageId":"CAN7MxmVxYhF1VXWZNAXyfF8iBNQaqqwoyjmDAP_9_bc5NUUU6A@mail.gmail.com","threadId":"35985","inReplyTo":"20140228105929.GA18960@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream","fromName":"Brian Gesiak","fromEmail":"modocache@gmail.com","sentAt":"2014-02-28T11:16:13Z","receivedAt":"2014-02-28T11:16:13Z","isPatch":true,"sender":{"key":"modocache@gmail.com","avatar":"https://avatars.githubusercontent.com/u/552921?v=4"},"body":"> I just don't want to regress somebody else's workflow due\n> to my lack of imagination.\n\nThis makes a lot of sense to me, although as-is the function emits a\nwarning and returns immediately (although with a successful status\ncode), so I'm also stumped as to what kind of workflow this would be\nincluded in.\n\nIn any case, if the jury's out on this one, I suppose the two patches\nI submitted are good to merge? Part of me thinks the bump from warning\nto error belongs in its own patch anyway.\n\n- Brian Gesiak\n"},{"id":"235605","messageId":"20140228130340.GB23448@sigill.intra.peff.net","threadId":"35985","inReplyTo":"CAN7MxmVxYhF1VXWZNAXyfF8iBNQaqqwoyjmDAP_9_bc5NUUU6A@mail.gmail.com","subject":"Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-28T13:03:41Z","receivedAt":"2014-02-28T13:03:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 28, 2014 at 08:16:13PM +0900, Brian Gesiak wrote:\n\n> > I just don't want to regress somebody else's workflow due\n> > to my lack of imagination.\n> \n> This makes a lot of sense to me, although as-is the function emits a\n> warning and returns immediately (although with a successful status\n> code), so I'm also stumped as to what kind of workflow this would be\n> included in.\n\nI'm cc-ing Matthieu, who wrote 85e2233, which introduces the check. Its\ncommit message says:\n\n branch: warn and refuse to set a branch as a tracking branch of\n itself.\n\n Previous patch allows commands like \"git branch --set-upstream foo\n foo\", which doesn't make much sense. Warn the user and don't change\n the configuration in this case. Don't die to let the caller finish its\n job in such case.\n\nFor those just joining us, we are focused on the final statement, and\ndeciding whether we should die() in this case. But I am not clear what\njob it would want to be finishing (creating the branch, I guess, but you\ncannot be doing anything useful, since by definition the branch already\nexists and you are not changing its tip). There wasn't any relevant\ndiscussion on the list[1]. Matthieu, can you remember anything else that\nled to that decision?\n\n> In any case, if the jury's out on this one, I suppose the two patches\n> I submitted are good to merge? Part of me thinks the bump from warning\n> to error belongs in its own patch anyway.\n\nYeah, it should definitely be a separate patch on top.\n\n-Peff\n\n[1] Threads are:\n\n http://thread.gmane.org/gmane.comp.version-control.git/137297/focus=137299\n\n and\n\n http://thread.gmane.org/gmane.comp.version-control.git/137401/focus=137403\n\n but the discussion focused on the first part of the series.\n"},{"id":"235623","messageId":"vpqa9db1e0l.fsf@anie.imag.fr","threadId":"35985","inReplyTo":"20140228130340.GB23448@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-02-28T14:57:14Z","receivedAt":"2014-02-28T14:57:14Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeff King writes:\n\n> Don't die to let the caller finish its\n> job in such case.\n\n[...]\n\n> Matthieu, can you remember anything else that\n> led to that decision?\n\nNot at all, unfortunately. I don't remember if I did that \"in case\nthere's something like some cleanup to do\" or because I had something\nmore precise in mind.\n\nA case to be carefull about is if you're using the same \"git branch\"\ncommand for multiple actions (trying --set-upstream in combination with\nother options). But I do not see a case where this would be possible.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"235673","messageId":"20140228221505.GA681289@vauxhall.crustytoothpaste.net","threadId":"35985","inReplyTo":"20140228062715.GE32556@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-02-28T22:15:05Z","receivedAt":"2014-02-28T22:15:05Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, Feb 28, 2014 at 01:27:15AM -0500, Jeff King wrote:\n> On Fri, Feb 28, 2014 at 03:17:28PM +0900, Brian Gesiak wrote:\n> > Patch is on the way, just waiting for the tests to complete. Thanks\n> > for pointing that out! Also, sorry if it's in the Makefile somewhere,\n> > but is there an easy way to run just a single test file in the t\n> > directory?\n> \n> You can run \"./tXXXX-....sh\" explicitly.\n\nYou can also use Perl's prove command, which provides some nice-to-have\nfeatures, such as exiting abnormally if your tests abort prematurely.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"235715","messageId":"1393676356-67203-1-git-send-email-modocache@gmail.com","threadId":"35985","inReplyTo":"vpqa9db1e0l.fsf@anie.imag.fr","subject":"[PATCH v2] branch: die when setting branch as own upstream","fromName":"Brian Gesiak","fromEmail":"modocache@gmail.com","sentAt":"2014-03-01T12:19:16Z","receivedAt":"2014-03-01T12:19:16Z","isPatch":true,"sender":{"key":"modocache@gmail.com","avatar":"https://avatars.githubusercontent.com/u/552921?v=4"},"body":"From: modocache \n\nBranch set as own upstream using one of the following commands returns\nimmediately with an exit code of 0:\n\n- `git branch --set-upstream-to foo refs/heads/foo`\n- `git branch --force --track foo foo`\n\nSince neither of these actions currently set the upstream, an exit code\nof 0 is misleading. Instead, exit with a status code indicating failure\nby using the die function.\n\nSigned-off-by: Brian Gesiak \n---\n branch.c | 9 ++-------\n t/t3200-branch.sh | 6 +++---\n 2 files changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex e163f3c..9bac8b5 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -54,13 +54,8 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n \tstruct strbuf key = STRBUF_INIT;\n \tint rebasing = should_setup_rebase(origin);\n \n-\tif (shortname\n-\t && !strcmp(local, shortname)\n-\t && !origin) {\n-\t\twarning(_(\"Not setting branch %s as its own upstream.\"),\n-\t\t\tlocal);\n-\t\treturn;\n-\t}\n+\tif (shortname && !strcmp(local, shortname) && !origin)\n+\t\tdie(_(\"Not setting branch %s as its own upstream.\"), local);\n \n \tstrbuf_addf(&key, \"branch.%s.remote\", local);\n \tgit_config_set(key.buf, origin ? origin : \".\");\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 6164126..3ac493f 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -507,10 +507,10 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' '\n-\tgit branch --set-upstream-to refs/heads/my13 my13 2>actual &&\n+test_expect_success '--set-upstream-to fails if used to set branch as own upstream' '\n+\ttest_must_fail git branch --set-upstream-to refs/heads/my13 my13 2>actual &&\n \tcat >expected <\n---\n branch.c | 9 ++-------\n t/t3200-branch.sh | 6 +++---\n 2 files changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex e163f3c..9bac8b5 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -54,13 +54,8 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n \tstruct strbuf key = STRBUF_INIT;\n \tint rebasing = should_setup_rebase(origin);\n \n-\tif (shortname\n-\t && !strcmp(local, shortname)\n-\t && !origin) {\n-\t\twarning(_(\"Not setting branch %s as its own upstream.\"),\n-\t\t\tlocal);\n-\t\treturn;\n-\t}\n+\tif (shortname && !strcmp(local, shortname) && !origin)\n+\t\tdie(_(\"Not setting branch %s as its own upstream.\"), local);\n \n \tstrbuf_addf(&key, \"branch.%s.remote\", local);\n \tgit_config_set(key.buf, origin ? origin : \".\");\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 6164126..3ac493f 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -507,10 +507,10 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' '\n-\tgit branch --set-upstream-to refs/heads/my13 my13 2>actual &&\n+test_expect_success '--set-upstream-to fails if used to set branch as own upstream' '\n+\ttest_must_fail git branch --set-upstream-to refs/heads/my13 my13 2>actual &&\n \tcat >expected < wrote:\n> Branch set as own upstream using one of the following commands returns\n> immediately with an exit code of 0:\n>\n> - `git branch --set-upstream-to foo refs/heads/foo`\n> - `git branch --force --track foo foo`\n>\n> Since neither of these actions currently set the upstream, an exit code\n> of 0 is misleading. Instead, exit with a status code indicating failure\n> by using the die function.\n>\n> Signed-off-by: Brian Gesiak \n> ---\n> branch.c | 9 ++-------\n> t/t3200-branch.sh | 6 +++---\n> 2 files changed, 5 insertions(+), 10 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index e163f3c..9bac8b5 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -54,13 +54,8 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n> struct strbuf key = STRBUF_INIT;\n> int rebasing = should_setup_rebase(origin);\n>\n> - if (shortname\n> - && !strcmp(local, shortname)\n> - && !origin) {\n> - warning(_(\"Not setting branch %s as its own upstream.\"),\n> - local);\n> - return;\n> - }\n> + if (shortname && !strcmp(local, shortname) && !origin)\n> + die(_(\"Not setting branch %s as its own upstream.\"), local);\n>\n> strbuf_addf(&key, \"branch.%s.remote\", local);\n> git_config_set(key.buf, origin ? origin : \".\");\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index 6164126..3ac493f 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -507,10 +507,10 @@ EOF\n> test_cmp expected actual\n> '\n>\n> -test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' '\n> - git branch --set-upstream-to refs/heads/my13 my13 2>actual &&\n> +test_expect_success '--set-upstream-to fails if used to set branch as own upstream' '\n> + test_must_fail git branch --set-upstream-to refs/heads/my13 my13 2>actual &&\n> cat >expected < -warning: Not setting branch my13 as its own upstream.\n> +fatal: Not setting branch my13 as its own upstream.\n> EOF\n> test_i18ncmp expected actual\n> '\n> --\n> 1.8.3.4 (Apple Git-47)\n>\n"},{"id":"235914","messageId":"xmqq4n3foy6f.fsf@gitster.dls.corp.google.com","threadId":"35985","inReplyTo":"20140228054613.GB32556@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] branch: use skip_prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-03T19:54:32Z","receivedAt":"2014-03-03T19:54:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Feb 28, 2014 at 12:04:19PM +0900, Brian Gesiak wrote:\n>\n>> From: modocache \n>\n> Both your emailed patches have this, which is due to your author name\n> not matching your sending identity. You probably want to set user.name,\n> or if you already have (which it looks like you might have from your\n> Signed-off-by), use \"git commit --amend --reset-author\" to update the\n> author information.\n>\n>> The install_branch_config function reimplemented the skip_prefix\n>> function inline. Use skip_prefix function instead for brevity.\n>> \n>> Signed-off-by: Brian Gesiak \n>> Reported-by: Michael Haggerty \n>\n> It's a minor thing, but usually these footer lines try to follow a\n> chronological order. So the report would come before the signoff (and a\n> further signoff from the maintainer would go after yours).\n>\n>> diff --git a/branch.c b/branch.c\n>> index 723a36b..e163f3c 100644\n>> --- a/branch.c\n>> +++ b/branch.c\n>> [...]\n>\n> The patch itself looks OK to me.\n>\n> -Peff\n\nThanks. Queued and pushed out on 'pu' with fixups already.\n"}]}