{"thread":{"id":"35942","subject":"[BUG] remote.pushdefault and branch..pushremote definition order","startedAt":"2014-02-24T05:10:04Z","lastAt":"2014-02-24T22:45:52Z","messageCount":7,"participants":["Jack Nagel","Jeff King","Junio C Hamano","Ramkumar Ramachandra"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"235219","messageId":"CAMYxyaUwPXXdvGv786_p5n7-biMLOFbur-gQ1pxXOttc0R0=eg@mail.gmail.com","threadId":"35942","inReplyTo":null,"subject":"[BUG] remote.pushdefault and branch..pushremote definition order","fromName":"Jack Nagel","fromEmail":"jacknagel@gmail.com","sentAt":"2014-02-24T05:10:04Z","receivedAt":"2014-02-24T05:10:04Z","isPatch":false,"sender":{"key":"jacknagel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/568243?v=4"},"body":"There seems to be a difference in the behavior of \"git push\" depending\non whether remote.pushdefault is defined before or after\nbranch..pushremote in .git/config.\n\nIf remote.pushdefault is defined to be \"origin\", and later in the\nfile, branch.master.pushremote is defined to be \"upstream\", then a\nplain \"git push\" from master errors out because I haven't provided a\nrefspec or configured push.default. This makes sense.\n\nHowever, if the order of the two in the file is reversed, then a plain\n\"git push\" pushes to the \"origin\" repository, even though I have set\nthe pushremote for master to \"upstream\". This appears to be a bug.\n\nI would expect the order that things are defined in the config file to\nhave no effect on the behavior of \"git push\".\n\nI have reproduced this using git 1.9.0 and 1.8.3.4.\n\nThanks,\nJack\n"},{"id":"235233","messageId":"20140224085903.GA10698@sigill.intra.peff.net","threadId":"35942","inReplyTo":"CAMYxyaUwPXXdvGv786_p5n7-biMLOFbur-gQ1pxXOttc0R0=eg@mail.gmail.com","subject":"[PATCH] remote: handle pushremote config in any order order","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-24T08:59:03Z","receivedAt":"2014-02-24T08:59:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 24, 2014 at 12:10:04AM -0500, Jack Nagel wrote:\n\n> There seems to be a difference in the behavior of \"git push\" depending\n> on whether remote.pushdefault is defined before or after\n> branch..pushremote in .git/config.\n> [...]\n> I would expect the order that things are defined in the config file to\n> have no effect on the behavior of \"git push\".\n\nYes, with a few exceptions, we usually try to make the ordering in the\nconfig file irrelevant. This is a bug. The patch below should fix it.\n\n-- >8 --\nSubject: remote: handle pushremote config in any order\n\nThe remote we push can be defined either by\nremote.pushdefault or by branch.*.pushremote for the current\nbranch. The order in which they appear in the config file\nshould not matter to precedence (which should be to prefer\nthe branch-specific config).\n\nThe current code parses the config linearly and uses a\nsingle string to store both values, overwriting any\nprevious value. Thus, config like:\n\n [branch \"master\"]\n pushremote = foo\n [remote]\n pushdefault = bar\n\nerroneously ends up pushing to \"bar\" from the master branch.\n\nWe can fix this by storing both values and resolving the\ncorrect value after all config is read.\n\nSigned-off-by: Jeff King \n---\n remote.c | 7 ++++++-\n t/t5516-fetch-push.sh | 12 ++++++++++++\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex e41251e..7232a33 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -49,6 +49,7 @@ static int branches_nr;\n \n static struct branch *current_branch;\n static const char *default_remote_name;\n+static const char *branch_pushremote_name;\n static const char *pushremote_name;\n static int explicit_default_remote_name;\n \n@@ -352,7 +353,7 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t\t}\n \t\t} else if (!strcmp(subkey, \".pushremote\")) {\n \t\t\tif (branch == current_branch)\n-\t\t\t\tif (git_config_string(&pushremote_name, key, value))\n+\t\t\t\tif (git_config_string(&branch_pushremote_name, key, value))\n \t\t\t\t\treturn -1;\n \t\t} else if (!strcmp(subkey, \".merge\")) {\n \t\t\tif (!value)\n@@ -492,6 +493,10 @@ static void read_config(void)\n \t\t\tmake_branch(head_ref + strlen(\"refs/heads/\"), 0);\n \t}\n \tgit_config(handle_config, NULL);\n+\tif (branch_pushremote_name) {\n+\t\tfree(pushremote_name);\n+\t\tpushremote_name = branch_pushremote_name;\n+\t}\n \talias_all_urls();\n }\n \ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 926e7f6..1309c4d 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -536,6 +536,18 @@ test_expect_success 'push with config branch.*.pushremote' '\n \tcheck_push_result down_repo $the_commit heads/master\n '\n \n+test_expect_success 'branch.*.pushremote config order is irrelevant' '\n+\tmk_test one_repo heads/master &&\n+\tmk_test two_repo heads/master &&\n+\ttest_config remote.one.url one_repo &&\n+\ttest_config remote.two.url two_repo &&\n+\ttest_config branch.master.pushremote two_repo &&\n+\ttest_config remote.pushdefault one_repo &&\n+\tgit push &&\n+\tcheck_push_result one_repo $the_first_commit heads/master &&\n+\tcheck_push_result two_repo $the_commit heads/master\n+'\n+\n test_expect_success 'push with dry-run' '\n \n \tmk_test testrepo heads/master &&\n-- \n1.8.5.2.500.g8060133\n"},{"id":"235273","messageId":"xmqqvbw49z0f.fsf@gitster.dls.corp.google.com","threadId":"35942","inReplyTo":"20140224085903.GA10698@sigill.intra.peff.net","subject":"Re: [PATCH] remote: handle pushremote config in any order order","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-24T17:55:12Z","receivedAt":"2014-02-24T17:55:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yes, with a few exceptions, we usually try to make the ordering in the\n> config file irrelevant. This is a bug. The patch below should fix it.\n\nLooks good. Thanks.\n\n> -- >8 --\n> Subject: remote: handle pushremote config in any order\n>\n> The remote we push can be defined either by\n> remote.pushdefault or by branch.*.pushremote for the current\n> branch. The order in which they appear in the config file\n> should not matter to precedence (which should be to prefer\n> the branch-specific config).\n>\n> The current code parses the config linearly and uses a\n> single string to store both values, overwriting any\n> previous value. Thus, config like:\n>\n> [branch \"master\"]\n> pushremote = foo\n> [remote]\n> pushdefault = bar\n>\n> erroneously ends up pushing to \"bar\" from the master branch.\n>\n> We can fix this by storing both values and resolving the\n> correct value after all config is read.\n>\n> Signed-off-by: Jeff King \n> ---\n> remote.c | 7 ++++++-\n> t/t5516-fetch-push.sh | 12 ++++++++++++\n> 2 files changed, 18 insertions(+), 1 deletion(-)\n>\n> diff --git a/remote.c b/remote.c\n> index e41251e..7232a33 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -49,6 +49,7 @@ static int branches_nr;\n> \n> static struct branch *current_branch;\n> static const char *default_remote_name;\n> +static const char *branch_pushremote_name;\n> static const char *pushremote_name;\n> static int explicit_default_remote_name;\n> \n> @@ -352,7 +353,7 @@ static int handle_config(const char *key, const char *value, void *cb)\n> \t\t\t}\n> \t\t} else if (!strcmp(subkey, \".pushremote\")) {\n> \t\t\tif (branch == current_branch)\n> -\t\t\t\tif (git_config_string(&pushremote_name, key, value))\n> +\t\t\t\tif (git_config_string(&branch_pushremote_name, key, value))\n> \t\t\t\t\treturn -1;\n> \t\t} else if (!strcmp(subkey, \".merge\")) {\n> \t\t\tif (!value)\n> @@ -492,6 +493,10 @@ static void read_config(void)\n> \t\t\tmake_branch(head_ref + strlen(\"refs/heads/\"), 0);\n> \t}\n> \tgit_config(handle_config, NULL);\n> +\tif (branch_pushremote_name) {\n> +\t\tfree(pushremote_name);\n> +\t\tpushremote_name = branch_pushremote_name;\n> +\t}\n> \talias_all_urls();\n> }\n> \n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 926e7f6..1309c4d 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -536,6 +536,18 @@ test_expect_success 'push with config branch.*.pushremote' '\n> \tcheck_push_result down_repo $the_commit heads/master\n> '\n> \n> +test_expect_success 'branch.*.pushremote config order is irrelevant' '\n> +\tmk_test one_repo heads/master &&\n> +\tmk_test two_repo heads/master &&\n> +\ttest_config remote.one.url one_repo &&\n> +\ttest_config remote.two.url two_repo &&\n> +\ttest_config branch.master.pushremote two_repo &&\n> +\ttest_config remote.pushdefault one_repo &&\n> +\tgit push &&\n> +\tcheck_push_result one_repo $the_first_commit heads/master &&\n> +\tcheck_push_result two_repo $the_commit heads/master\n> +'\n> +\n> test_expect_success 'push with dry-run' '\n> \n> \tmk_test testrepo heads/master &&\n"},{"id":"235294","messageId":"xmqq7g8k8d5r.fsf@gitster.dls.corp.google.com","threadId":"35942","inReplyTo":"xmqqvbw49z0f.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] remote: handle pushremote config in any order order","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-24T20:32:32Z","receivedAt":"2014-02-24T20:32:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> Yes, with a few exceptions, we usually try to make the ordering in the\n>> config file irrelevant. This is a bug. The patch below should fix it.\n>\n> Looks good. Thanks.\n\n>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n>> index 926e7f6..1309c4d 100755\n>> --- a/t/t5516-fetch-push.sh\n>> +++ b/t/t5516-fetch-push.sh\n>> @@ -536,6 +536,18 @@ test_expect_success 'push with config branch.*.pushremote' '\n>> \tcheck_push_result down_repo $the_commit heads/master\n>> '\n>> \n>> +test_expect_success 'branch.*.pushremote config order is irrelevant' '\n>> +\tmk_test one_repo heads/master &&\n>> +\tmk_test two_repo heads/master &&\n>> +\ttest_config remote.one.url one_repo &&\n>> +\ttest_config remote.two.url two_repo &&\n>> +\ttest_config branch.master.pushremote two_repo &&\n>> +\ttest_config remote.pushdefault one_repo &&\n>> +\tgit push &&\n>> +\tcheck_push_result one_repo $the_first_commit heads/master &&\n>> +\tcheck_push_result two_repo $the_commit heads/master\n>> +'\n>> +\n\nThis test however does not pass in the Git 2.0 world, without having\nthis line:\n\n test_config push.default matching &&\n\nimmediately before \"git push\".\n\nAm I missing something?\n"},{"id":"235297","messageId":"20140224203946.GB25506@sigill.intra.peff.net","threadId":"35942","inReplyTo":"xmqq7g8k8d5r.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] remote: handle pushremote config in any order order","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-24T20:39:47Z","receivedAt":"2014-02-24T20:39:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 24, 2014 at 12:32:32PM -0800, Junio C Hamano wrote:\n\n> >> +test_expect_success 'branch.*.pushremote config order is irrelevant' '\n> >> +\tmk_test one_repo heads/master &&\n> >> +\tmk_test two_repo heads/master &&\n> >> +\ttest_config remote.one.url one_repo &&\n> >> +\ttest_config remote.two.url two_repo &&\n> >> +\ttest_config branch.master.pushremote two_repo &&\n> >> +\ttest_config remote.pushdefault one_repo &&\n> >> +\tgit push &&\n> >> +\tcheck_push_result one_repo $the_first_commit heads/master &&\n> >> +\tcheck_push_result two_repo $the_commit heads/master\n> >> +'\n> >> +\n> \n> This test however does not pass in the Git 2.0 world, without having\n> this line:\n> \n> test_config push.default matching &&\n> \n> immediately before \"git push\".\n> \n> Am I missing something?\n\nNo, you are not missing anything. I was copying and paring down the\npushremote test above, and I accidentally pared out the push.default\nsetting. It should definitely have a\n\n test_config push.default matching &&\n\nbefore the \"git push\" line, as the test above does. Can you mark it up\nas you apply?\n\n-Peff\n"},{"id":"235298","messageId":"xmqq38j88c66.fsf@gitster.dls.corp.google.com","threadId":"35942","inReplyTo":"20140224203946.GB25506@sigill.intra.peff.net","subject":"Re: [PATCH] remote: handle pushremote config in any order order","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-24T20:53:53Z","receivedAt":"2014-02-24T20:53:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Feb 24, 2014 at 12:32:32PM -0800, Junio C Hamano wrote:\n>\n>> >> +test_expect_success 'branch.*.pushremote config order is irrelevant' '\n>> >> +\tmk_test one_repo heads/master &&\n>> >> +\tmk_test two_repo heads/master &&\n>> >> +\ttest_config remote.one.url one_repo &&\n>> >> +\ttest_config remote.two.url two_repo &&\n>> >> +\ttest_config branch.master.pushremote two_repo &&\n>> >> +\ttest_config remote.pushdefault one_repo &&\n>> >> +\tgit push &&\n>> >> +\tcheck_push_result one_repo $the_first_commit heads/master &&\n>> >> +\tcheck_push_result two_repo $the_commit heads/master\n>> >> +'\n>> >> +\n>> \n>> This test however does not pass in the Git 2.0 world, without having\n>> this line:\n>> \n>> test_config push.default matching &&\n>> \n>> immediately before \"git push\".\n>> \n>> Am I missing something?\n>\n> No, you are not missing anything. I was copying and paring down the\n> pushremote test above, and I accidentally pared out the push.default\n> setting. It should definitely have a\n>\n> test_config push.default matching &&\n>\n> before the \"git push\" line, as the test above does. Can you mark it up\n> as you apply?\n\nGladly ;-)\n\nI wasn't thinking straight and thought push.default was somehow\naffecting the logic to read the configuration files you fixed, which\nwas a complete nonsense. The selection of which remote to push to\nis affected by the branch.*.pushremote and remote.pushdefault, but\nthis \"git push\" still expects that the way the branches are chosen\nto be pushed follow the matching semantics, not the simple\nsemantics, so we need that configuration there.\n"},{"id":"235303","messageId":"CALkWK0kVavvJDgn0_YdqexpUP=pAhbPAooXWjcYxNf5fxMyiVQ@mail.gmail.com","threadId":"35942","inReplyTo":"20140224085903.GA10698@sigill.intra.peff.net","subject":"Re: [PATCH] remote: handle pushremote config in any order order","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2014-02-24T22:45:52Z","receivedAt":"2014-02-24T22:45:52Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Jeff King wrote:\n> Thus, config like:\n>\n> [branch \"master\"]\n> pushremote = foo\n> [remote]\n> pushdefault = bar\n>\n> erroneously ends up pushing to \"bar\" from the master branch.\n\nOh, ouch. Thanks for fixing this.\n"}]}