{"thread":{"id":"48655","subject":"[PATCH] t3200-branch.sh: use \"--set-upstream-to\" in test","startedAt":"2018-06-05T11:17:04Z","lastAt":"2018-06-17T11:56:54Z","messageCount":8,"participants":["Robert P. J. Day","SZEDER Gábor","Kaartic Sivaraam","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"349333","messageId":"alpine.LFD.2.21.1806050712270.10467@localhost.localdomain","threadId":"48655","inReplyTo":null,"subject":"[PATCH] t3200-branch.sh: use \"--set-upstream-to\" in test","fromName":"Robert P. J. Day","fromEmail":"rpjday@crashcourse.ca","sentAt":"2018-06-05T11:14:45Z","receivedAt":"2018-06-05T11:17:04Z","isPatch":true,"sender":{"key":"rpjday@crashcourse.ca","avatar":"https://avatars.githubusercontent.com/u/226084077?v=4"},"body":"\nChange deprecated \"--set-upstream\" branch option to\npreferred \"--set-upstream-to\".\n\nSigned-off-by: Robert P. J. Day \n\n---\n\n i'm assuming this should use \"--set-upstream-to\" as do all the\nothers.\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 69ea8202f4..ef887a0b32 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -885,8 +885,8 @@ test_expect_success 'test --unset-upstream on a particular branch' '\n \ttest_must_fail git config branch.my14.merge\n '\n\n-test_expect_success '--set-upstream fails' '\n- test_must_fail git branch --set-upstream origin/master\n+test_expect_success '--set-upstream-to fails' '\n+ test_must_fail git branch --set-upstream-to origin/master\n '\n\n test_expect_success '--set-upstream-to notices an error to set branch as own upstream' '\n\n-- \n\n========================================================================\nRobert P. J. Day Ottawa, Ontario, CANADA\n http://crashcourse.ca/dokuwiki\n\nTwitter: http://twitter.com/rpjday\nLinkedIn: http://ca.linkedin.com/in/rpjday\n========================================================================\n"},{"id":"349334","messageId":"20180605112446.17016-1-szeder.dev@gmail.com","threadId":"48655","inReplyTo":"alpine.LFD.2.21.1806050712270.10467@localhost.localdomain","subject":"Re: [PATCH] t3200-branch.sh: use \"--set-upstream-to\" in test","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-06-05T11:24:46Z","receivedAt":"2018-06-05T11:25:06Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> Change deprecated \"--set-upstream\" branch option to\n> preferred \"--set-upstream-to\".\n> \n> Signed-off-by: Robert P. J. Day \n> \n> ---\n> \n> i'm assuming this should use \"--set-upstream-to\" as do all the\n> others.\n\nI don't think so, see 52668846ea (builtin/branch: stop supporting the\n\"--set-upstream\" option, 2017-08-17).\n\nThough arguably the test name could be more descriptive and tell why\nit should fail.\n\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index 69ea8202f4..ef887a0b32 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -885,8 +885,8 @@ test_expect_success 'test --unset-upstream on a particular branch' '\n> \ttest_must_fail git config branch.my14.merge\n> '\n> \n> -test_expect_success '--set-upstream fails' '\n> - test_must_fail git branch --set-upstream origin/master\n> +test_expect_success '--set-upstream-to fails' '\n> + test_must_fail git branch --set-upstream-to origin/master\n> '\n> \n> test_expect_success '--set-upstream-to notices an error to set branch as own upstream' '\n> \n> -- \n> \n> ========================================================================\n> Robert P. J. Day Ottawa, Ontario, CANADA\n> http://crashcourse.ca/dokuwiki\n> \n> Twitter: http://twitter.com/rpjday\n> LinkedIn: http://ca.linkedin.com/in/rpjday\n> ========================================================================\n> \n"},{"id":"349335","messageId":"alpine.LFD.2.21.1806050734070.11943@localhost.localdomain","threadId":"48655","inReplyTo":"20180605112446.17016-1-szeder.dev@gmail.com","subject":"Re: [PATCH] t3200-branch.sh: use \"--set-upstream-to\" in test","fromName":"Robert P. J. Day","fromEmail":"rpjday@crashcourse.ca","sentAt":"2018-06-05T11:34:35Z","receivedAt":"2018-06-05T11:36:51Z","isPatch":true,"sender":{"key":"rpjday@crashcourse.ca","avatar":"https://avatars.githubusercontent.com/u/226084077?v=4"},"body":"On Tue, 5 Jun 2018, SZEDER Gábor wrote:\n\n> > Change deprecated \"--set-upstream\" branch option to\n> > preferred \"--set-upstream-to\".\n> >\n> > Signed-off-by: Robert P. J. Day \n> >\n> > ---\n> >\n> > i'm assuming this should use \"--set-upstream-to\" as do all the\n> > others.\n>\n> I don't think so, see 52668846ea (builtin/branch: stop supporting\n> the \"--set-upstream\" option, 2017-08-17).\n\n yes, you're right, i didn't look at the context enough. my bad.\n\nrday\n\n-- \n\n========================================================================\nRobert P. J. Day Ottawa, Ontario, CANADA\n http://crashcourse.ca/dokuwiki\n\nTwitter: http://twitter.com/rpjday\nLinkedIn: http://ca.linkedin.com/in/rpjday\n========================================================================"},{"id":"349506","messageId":"8af183f2-7ca5-86ba-b189-6b6885158453@gmail.com","threadId":"48655","inReplyTo":"20180605112446.17016-1-szeder.dev@gmail.com","subject":"Re: [PATCH] t3200-branch.sh: use \"--set-upstream-to\" in test","fromName":"Kaartic Sivaraam","fromEmail":"kaarticsivaraam91196@gmail.com","sentAt":"2018-06-06T15:50:17Z","receivedAt":"2018-06-06T15:50:32Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Tuesday 05 June 2018 04:54 PM, SZEDER Gábor wrote:\n> \n> Though arguably the test name could be more descriptive and tell why\n> it should fail.\n> \n\nThat's arguable, indeed. I was about to send a patch that gives a better\ndescription for the test. I didn't do it as I started wondering, Is it\neven worth testing whether a removed option fails? Is this done for\nother options that have been removed in the past? Should we just remove\nthe test completely?\n\n-- \nSivaraam\n\nQUOTE:\n\n“The three principal virtues of a programmer are Laziness, Impatience,\nand Hubris.”\n\n\t- Camel book\n\nSivaraam?\n\nYou possibly might have noticed that my signature recently changed from\n'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the\nnew signature to be better for several reasons one of which is that the\nformer signature has a lot of ambiguities in the place I live as it is a\ncommon name (NOTE: it's not a common spelling, just a common name). So,\nI switched signatures before it's too late.\n\nThat said, I won't mind you calling me 'Kaartic' if you like it [of\ncourse ;-)]. You can always call me using either of the names.\n\n\nKIND NOTE TO THE NATIVE ENGLISH SPEAKER:\n\nAs I'm not a native English speaker myself, there might be mistaeks in\nmy usage of English. I apologise for any mistakes that I make.\n\nIt would be \"helpful\" if you take the time to point out the mistakes.\n\nIt would be \"super helpful\" if you could provide suggestions about how\nto correct those mistakes.\n\nThanks in advance!\n\n"},{"id":"350153","messageId":"20180614140641.13752-1-kaartic.sivaraam@gmail.com","threadId":"48655","inReplyTo":"20180605112446.17016-1-szeder.dev@gmail.com","subject":"[PATCH] t3200: clarify description of --set-upstream test","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2018-06-14T14:06:41Z","receivedAt":"2018-06-14T14:46:59Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"Support for the --set-upstream option was removed in 52668846ea\n(builtin/branch: stop supporting the \"--set-upstream\" option,\n2017-08-17). The change did not completely remove the command\ndue to an issue noted in the commit's log message.\n\nSo, a test was added to ensure that a command which uses the\n'--set-upstream' option fails and doesn't silently act as an alias\nfor the '--set-upstream-to' option due to option parsing features.\n\nTo avoid confusion, clarify that the option is an unsupported one\nin the corresponding test description.\n\nSigned-off-by: Kaartic Sivaraam \n---\n t/t3200-branch.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 6c0b7ea4a..d14de82ba 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -884,7 +884,7 @@ test_expect_success 'test --unset-upstream on a particular branch' '\n \ttest_must_fail git config branch.my14.merge\n '\n \n-test_expect_success '--set-upstream fails' '\n+test_expect_success 'unsupported option --set-upstream fails' '\n test_must_fail git branch --set-upstream origin/master\n '\n \n-- \n2.18.0.rc1.242.g61856ae69\n\n"},{"id":"350174","messageId":"xmqqin6l461s.fsf@gitster-ct.c.googlers.com","threadId":"48655","inReplyTo":"20180614140641.13752-1-kaartic.sivaraam@gmail.com","subject":"Re: [PATCH] t3200: clarify description of --set-upstream test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-14T17:43:59Z","receivedAt":"2018-06-14T17:44:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kaartic Sivaraam writes:\n\n> Support for the --set-upstream option was removed in 52668846ea\n> (builtin/branch: stop supporting the \"--set-upstream\" option,\n> 2017-08-17). The change did not completely remove the command\n> due to an issue noted in the commit's log message.\n>\n> So, a test was added to ensure that a command which uses the\n> '--set-upstream' option fails and doesn't silently act as an alias\n> for the '--set-upstream-to' option due to option parsing features.\n>\n> To avoid confusion, clarify that the option is an unsupported one\n> in the corresponding test description.\n>\n> Signed-off-by: Kaartic Sivaraam \n> ---\n> t/t3200-branch.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nThe above description is much clearer than what the test title after\nthe patch gives its readers.\n\nIt is technically correct to call --set-upstream \"unsupported\", but\nthe reason why we want to see it fail is not because it is\nunsupported, but because we actively interfere with the usual\n\"unique prefix\" logic parse-options API gives its users and make it\nnot to trigger the longer-and-unique --set-upstream-to logic.\n\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index 6c0b7ea4a..d14de82ba 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -884,7 +884,7 @@ test_expect_success 'test --unset-upstream on a particular branch' '\n> \ttest_must_fail git config branch.my14.merge\n> '\n> \n> -test_expect_success '--set-upstream fails' '\n> +test_expect_success 'unsupported option --set-upstream fails' '\n\nIn other words, I am wondering if s/unsupported/disabled/ makes it\neven more clear what is going on here.\n\n> test_must_fail git branch --set-upstream origin/master\n> '\n\n"},{"id":"350352","messageId":"d009c6e5-884c-cac0-3c36-110fad3e802d@gmail.com","threadId":"48655","inReplyTo":"xmqqin6l461s.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] t3200: clarify description of --set-upstream test","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2018-06-17T08:53:27Z","receivedAt":"2018-06-17T08:53:40Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Thursday 14 June 2018 11:13 PM, Junio C Hamano wrote:\n> It is technically correct to call --set-upstream \"unsupported\", but\n> the reason why we want to see it fail is not because it is\n> unsupported, but because we actively interfere with the usual\n> \"unique prefix\" logic parse-options API gives its users and make it\n> not to trigger the longer-and-unique --set-upstream-to logic.\n> \n\nThat sounds right.\n\n\n>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n>> index 6c0b7ea4a..d14de82ba 100755\n>> --- a/t/t3200-branch.sh\n>> +++ b/t/t3200-branch.sh\n>> @@ -884,7 +884,7 @@ test_expect_success 'test --unset-upstream on a particular branch' '\n>> \ttest_must_fail git config branch.my14.merge\n>> '\n>> \n>> -test_expect_success '--set-upstream fails' '\n>> +test_expect_success 'unsupported option --set-upstream fails' '\n> \n> In other words, I am wondering if s/unsupported/disabled/ makes it\n> even more clear what is going on here.\n>\n\nI guess it would :-) Thanks for the better wording. I actually thought\nof asking for a better wording for the test message while sending the\npatch but somehow forgot to mention it. It seems I've got better\nwordings, regardless.\n\nI'll send a v2 with the correction. I'm still open to an alternative\ntest description in case that make things even more clearer :-)\n\n\nThanks,\nSivaraam\n\nQUOTE:\n\n“The three principal virtues of a programmer are Laziness, Impatience,\nand Hubris.”\n\n\t- Camel book\n\nSivaraam?\n\nYou possibly might have noticed that my signature recently changed from\n'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the\nnew signature to be better for several reasons one of which is that the\nformer signature has a lot of ambiguities in the place I live as it is a\ncommon name (NOTE: it's not a common spelling, just a common name). So,\nI switched signatures before it's too late.\n\nThat said, I won't mind you calling me 'Kaartic' if you like it [of\ncourse ;-)]. You can always call me using either of the names.\n\n\nKIND NOTE TO THE NATIVE ENGLISH SPEAKER:\n\nAs I'm not a native English speaker myself, there might be mistaeks in\nmy usage of English. I apologise for any mistakes that I make.\n\nIt would be \"helpful\" if you take the time to point out the mistakes.\n\nIt would be \"super helpful\" if you could provide suggestions about how\nto correct those mistakes.\n\nThanks in advance!\n\n"},{"id":"350355","messageId":"20180617115627.32524-1-kaartic.sivaraam@gmail.com","threadId":"48655","inReplyTo":"xmqqin6l461s.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2] t3200: clarify description of --set-upstream test","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2018-06-17T11:56:27Z","receivedAt":"2018-06-17T11:56:54Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"Support for the --set-upstream option was removed in 52668846ea\n(builtin/branch: stop supporting the \"--set-upstream\" option,\n2017-08-17). The change did not completely remove the command\ndue to an issue noted in the commit's log message.\n\nSo, a test was added to ensure that a command which uses the\n'--set-upstream' option fails instead of silently acting as an alias\nfor the '--set-upstream-to' option due to option parsing features.\n\nTo avoid confusion, clarify that the option is disabled intentionally\nin the corresponding test description.\n\nThe test is expected to be around as long as we intentionally fail\non seeing the '--set-upstream' option which in turn we expect to\ndo for a period of time after which we can be sure that existing\nusers of '--set-upstream' are aware that the option is no\nlonger supported.\n\nSigned-off-by: Kaartic Sivaraam \n---\nChanges since v1:\n\n\t- update the test description as suggested by Junio\n\n\t- updated the commit message to be clear about period upto\n\t which we should be testing this.\n\n t/t3200-branch.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 6c0b7ea4a..e98dbfc1a 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -884,7 +884,7 @@ test_expect_success 'test --unset-upstream on a particular branch' '\n \ttest_must_fail git config branch.my14.merge\n '\n \n-test_expect_success '--set-upstream fails' '\n+test_expect_success 'disabled option --set-upstream fails' '\n test_must_fail git branch --set-upstream origin/master\n '\n \n-- \n2.18.0.rc1.242.g61856ae69\n\n"}]}