{"thread":{"id":"34260","subject":"[PATCH 1/6] t/t5528-push-default: remove redundant test_config lines","startedAt":"2013-06-24T04:33:02Z","lastAt":"2013-06-24T18:23:45Z","messageCount":36,"participants":["Junio C Hamano","Johan Herland","Ramkumar Ramachandra","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"221751","messageId":"1372048388-16742-1-git-send-email-gitster@pobox.com","threadId":"34260","inReplyTo":null,"subject":"[PATCH 0/6] Reroll of rr/triangular-push-fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T04:33:02Z","receivedAt":"2013-06-24T04:33:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This is mostly Ram's triangular-push-fix series, but the fix-up\ncommits I queued on top of it when the series was queued to 'pu'\nhave been squashed in.\n\n [PATCH 1/6] t/t5528-push-default: remove redundant test_config lines\n\n No changes, as posted by Ram.\n\n [PATCH 2/6] config doc: rewrite push.default section\n\n Reorganization (including moving of 'matching' to the end) is Ram's,\n but otherwise moderately rewritten.\n\n [PATCH 3/6] push: change `simple` to accommodate triangular workflows\n\n Sqaushed in the fix to keep the semantics of \"simple\" when used in\n the centralized workflow the same as before.\n\n [PATCH 4/6] t/t5528-push-default: generalize test_push_*\n\n As posted by Ram.\n\n [PATCH 5/6] t/t5528-push-default: test pushdefault workflows\n\n A style fix from the review, and comment tweaks to describe what\n each tests mean better.\n\n [PATCH 6/6] push: honor branch.*.push\n\n This is new. It probably needs tests and docs.\n\nI am trying this myself primarily because this changes the plan for\nGit 2.0 and jc/push-2.0-default-to-simple topic needs to be redone,\nbut before we can do so, we need to see this topic solidify enough.\n\nJunio C Hamano (1):\n push: honor branch.*.push\n\nRamkumar Ramachandra (5):\n t/t5528-push-default: remove redundant test_config lines\n config doc: rewrite push.default section\n push: change `simple` to accommodate triangular workflows\n t/t5528-push-default: generalize test_push_*\n t/t5528-push-default: test pushdefault workflows\n\n Documentation/config.txt | 80 ++++++++++++++++++++++++++++++------------------\n builtin/push.c | 59 +++++++++++++++++++++++++++--------\n remote.c | 5 +++\n remote.h | 2 ++\n t/t5528-push-default.sh | 65 ++++++++++++++++++++++++++++++++++++---\n 5 files changed, 165 insertions(+), 46 deletions(-)\n\n-- \n1.8.3.1-721-g0a353d3\n"},{"id":"221750","messageId":"1372048388-16742-2-git-send-email-gitster@pobox.com","threadId":"34260","inReplyTo":"1372048388-16742-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/6] t/t5528-push-default: remove redundant test_config lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T04:33:03Z","receivedAt":"2013-06-24T04:33:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Ramkumar Ramachandra \n\nThe line\n\n test_config push.default upstream\n\nappears unnecessarily in two tests, as the final test_push_failure sets\npush.default before pushing anyway.\n\nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Junio C Hamano \n---\n t/t5528-push-default.sh | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh\nindex 4736da8..69ce6bf 100755\n--- a/t/t5528-push-default.sh\n+++ b/t/t5528-push-default.sh\n@@ -48,7 +48,6 @@ test_expect_success '\"upstream\" pushes to configured upstream' '\n test_expect_success '\"upstream\" does not push on unconfigured remote' '\n \tgit checkout master &&\n \ttest_unconfig branch.master.remote &&\n-\ttest_config push.default upstream &&\n \ttest_commit three &&\n \ttest_push_failure upstream\n '\n@@ -57,7 +56,6 @@ test_expect_success '\"upstream\" does not push on unconfigured branch' '\n \tgit checkout master &&\n \ttest_config branch.master.remote parent1 &&\n \ttest_unconfig branch.master.merge &&\n-\ttest_config push.default upstream\n \ttest_commit four &&\n \ttest_push_failure upstream\n '\n-- \n1.8.3.1-721-g0a353d3\n"},{"id":"221755","messageId":"1372048388-16742-3-git-send-email-gitster@pobox.com","threadId":"34260","inReplyTo":"1372048388-16742-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/6] config doc: rewrite push.default section","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T04:33:04Z","receivedAt":"2013-06-24T04:33:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Ramkumar Ramachandra \n\n4d35924e (Merge branch 'rr/triangle', 2013-04-07) introduced support\nfor triangular workflows, but the push.default values still assume\ncentral workflows.\n\nRewrite the descriptions of `nothing`, `current`, `upstream` and\n`matching` for greater clarity, and explicitly explain how they\nshould behave in triangular workflows.\n\nLeave `simple` as it is for the moment, as we plan to change its\nmeaning to accommodate triangular workflows in a later patch.\n\nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Junio C Hamano \n---\n Documentation/config.txt | 72 +++++++++++++++++++++++++++++-------------------\n 1 file changed, 44 insertions(+), 28 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 7fd4035..5d8ff1a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1826,39 +1826,55 @@ pull.twohead::\n \tThe default merge strategy to use when pulling a single branch.\n \n push.default::\n-\tDefines the action `git push` should take if no refspec is given\n-\ton the command line, no refspec is configured in the remote, and\n-\tno refspec is implied by any of the options given on the command\n-\tline. Possible values are:\n+\tDefines the action `git push` should take if no refspec is\n+\texplicitly given. Different values are well-suited for\n+\tspecific workflows; for instance, in a purely central workflow\n+\t(i.e. the fetch source is equal to the push destination),\n+\t`upstream` is probably what you want. Possible values are:\n +\n --\n-* `nothing` - do not push anything.\n-* `matching` - push all branches having the same name in both ends.\n- This is for those who prepare all the branches into a publishable\n- shape and then push them out with a single command. It is not\n- appropriate for pushing into a repository shared by multiple users,\n- since locally stalled branches will attempt a non-fast forward push\n- if other users updated the branch.\n- +\n- This is currently the default, but Git 2.0 will change the default\n- to `simple`.\n-* `upstream` - push the current branch to its upstream branch\n- (`tracking` is a deprecated synonym for this).\n- With this, `git push` will update the same remote ref as the one which\n- is merged by `git pull`, making `push` and `pull` symmetrical.\n- See \"branch..merge\" for how to configure the upstream branch.\n+\n+* `nothing` - do not push anything (error out) unless a refspec is\n+ explicitly given. This is primarily meant for people who want to\n+ avoid mistakes by always being explicit.\n+\n+* `current` - push the current branch to update a branch with the same\n+ name on the receiving end. Works in both central and non-central\n+ workflows.\n+\n+* `upstream` - push the current branch back to the branch whose\n+ changes are usually integrated into the current branch (which is\n+ called `@{upstream}`). This mode only makes sense if you are\n+ pushing to the same repository you would normally pull from\n+ (i.e. central workflow).\n+\n * `simple` - like `upstream`, but refuses to push if the upstream\n branch's name is different from the local one. This is the safest\n- option and is well-suited for beginners. It will become the default\n- in Git 2.0.\n-* `current` - push the current branch to a branch of the same name.\n---\n+ option and is well-suited for beginners.\n +\n-The `simple`, `current` and `upstream` modes are for those who want to\n-push out a single branch after finishing work, even when the other\n-branches are not yet ready to be pushed out. If you are working with\n-other people to push into the same shared repository, you would want\n-to use one of these.\n+This mode will become the default in Git 2.0.\n+\n+* `matching` - push all branches having the same name on both ends.\n+ This makes the repository you are pushing to remember the set of\n+ branches that will be pushed out (e.g. if you always push 'maint'\n+ and 'master' there and no other branches, the repository you push\n+ to will have these two branches, and your local 'maint' and\n+ 'master' will be pushed there).\n++\n+To use this mode effectively, you have to make sure _all_ the\n+branches you would push out are ready to be pushed out before\n+running 'git push', as the whole point of this mode is to allow you\n+to push all of the branches in one go. If you usually finish work\n+on only one branch and push out the result, while other branches are\n+unfinished, this mode is not for you. Also this mode is not\n+suitable for pushing into a shared central repository, as other\n+people may add new branches there, or update the tip of existing\n+branches outside your control.\n++\n+This is currently the default, but Git 2.0 will change the default\n+to `simple`.\n+\n+--\n \n rebase.stat::\n \tWhether to show a diffstat of what changed upstream since the last\n-- \n1.8.3.1-721-g0a353d3\n"},{"id":"221756","messageId":"1372048388-16742-4-git-send-email-gitster@pobox.com","threadId":"34260","inReplyTo":"1372048388-16742-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/6] push: change `simple` to accommodate triangular workflows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T04:33:05Z","receivedAt":"2013-06-24T04:33:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Ramkumar Ramachandra \n\nWhen remote.pushdefault or branch..pushremote is set (a triangular\nworkflow feature), master@{u} != origin, and push.default is set to\n`upstream` or `simple`:\n\n $ git push\n fatal: You are pushing to remote 'origin', which is not the upstream of\n your current branch 'master', without telling me what to push\n to update which remote branch.\n\nThe very name of \"upstream\" indicates that it is only suitable for\nuse in central workflows; let us not even attempt to give it a new\nmeaning in triangular workflows, and error out as usual.\n\nHowever, the `simple` does not have this problem: it is poised to be\nthe default for Git 2.0, and we would definitely like it to do\nsomething sensible in triangular workflows.\n\nRedefine \"simple\" as \"safer upstream\" for centralized workflow as\nbefore, but work as \"current\" for triangular workflow.\n\nAn earlier round of this change by mistake broke the safety for\n\"simple\" mode we have had since day 1 of that mode to make sure that\nthe branch in the repository we update is set to be the one we fetch\nand integrate with, but it has been fixed.\n\nReported-by: Leandro Lucarella \nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Junio C Hamano \n---\n Documentation/config.txt | 10 +++++++---\n builtin/push.c | 43 +++++++++++++++++++++++++++++++------------\n 2 files changed, 38 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 5d8ff1a..cae6870 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1848,9 +1848,13 @@ push.default::\n pushing to the same repository you would normally pull from\n (i.e. central workflow).\n \n-* `simple` - like `upstream`, but refuses to push if the upstream\n- branch's name is different from the local one. This is the safest\n- option and is well-suited for beginners.\n+* `simple` - in centralized workflow, work like `upstream` with an\n+ added safety to refuse to push if the upstream branch's name is\n+ different from the local one.\n++\n+When pushing to a remote that is different from the remote you normally\n+pull from, work as `current`. This is the safest option and is suited\n+for beginners.\n +\n This mode will become the default in Git 2.0.\n \ndiff --git a/builtin/push.c b/builtin/push.c\nindex 2d84d10..f6c8047 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -120,10 +120,11 @@ static const char message_detached_head_die[] =\n \t \"\\n\"\n \t \" git push %s HEAD:\\n\");\n \n-static void setup_push_upstream(struct remote *remote, int simple)\n+static void setup_push_upstream(struct remote *remote, struct branch *branch,\n+\t\t\t\tint triangular)\n {\n \tstruct strbuf refspec = STRBUF_INIT;\n-\tstruct branch *branch = branch_get(NULL);\n+\n \tif (!branch)\n \t\tdie(_(message_detached_head_die), remote->name);\n \tif (!branch->merge_nr || !branch->merge || !branch->remote_name)\n@@ -137,18 +138,29 @@ static void setup_push_upstream(struct remote *remote, int simple)\n \tif (branch->merge_nr != 1)\n \t\tdie(_(\"The current branch %s has multiple upstream branches, \"\n \t\t \"refusing to push.\"), branch->name);\n-\tif (strcmp(branch->remote_name, remote->name))\n+\tif (triangular)\n \t\tdie(_(\"You are pushing to remote '%s', which is not the upstream of\\n\"\n \t\t \"your current branch '%s', without telling me what to push\\n\"\n \t\t \"to update which remote branch.\"),\n \t\t remote->name, branch->name);\n-\tif (simple && strcmp(branch->refname, branch->merge[0]->src))\n-\t\tdie_push_simple(branch, remote);\n+\n+\tif (push_default == PUSH_DEFAULT_SIMPLE) {\n+\t\t/* Additional safety */\n+\t\tif (strcmp(branch->refname, branch->merge[0]->src))\n+\t\t\tdie_push_simple(branch, remote);\n+\t}\n \n \tstrbuf_addf(&refspec, \"%s:%s\", branch->name, branch->merge[0]->src);\n \tadd_refspec(refspec.buf);\n }\n \n+static void setup_push_current(struct remote *remote, struct branch *branch)\n+{\n+\tif (!branch)\n+\t\tdie(_(message_detached_head_die), remote->name);\n+\tadd_refspec(branch->name);\n+}\n+\n static char warn_unspecified_push_default_msg[] =\n N_(\"push.default is unset; its implicit value is changing in\\n\"\n \"Git 2.0 from 'matching' to 'simple'. To squelch this message\\n\"\n@@ -173,9 +185,16 @@ static void warn_unspecified_push_default_configuration(void)\n \twarning(\"%s\\n\", _(warn_unspecified_push_default_msg));\n }\n \n+static int is_workflow_triagular(struct remote *remote)\n+{\n+\tstruct remote *fetch_remote = remote_get(NULL);\n+\treturn (fetch_remote && fetch_remote != remote);\n+}\n+\n static void setup_default_push_refspecs(struct remote *remote)\n {\n-\tstruct branch *branch;\n+\tstruct branch *branch = branch_get(NULL);\n+\tint triangular = is_workflow_triagular(remote);\n \n \tswitch (push_default) {\n \tdefault:\n@@ -188,18 +207,18 @@ static void setup_default_push_refspecs(struct remote *remote)\n \t\tbreak;\n \n \tcase PUSH_DEFAULT_SIMPLE:\n-\t\tsetup_push_upstream(remote, 1);\n+\t\tif (triangular)\n+\t\t\tsetup_push_current(remote, branch);\n+\t\telse\n+\t\t\tsetup_push_upstream(remote, branch, triangular);\n \t\tbreak;\n \n \tcase PUSH_DEFAULT_UPSTREAM:\n-\t\tsetup_push_upstream(remote, 0);\n+\t\tsetup_push_upstream(remote, branch, triangular);\n \t\tbreak;\n \n \tcase PUSH_DEFAULT_CURRENT:\n-\t\tbranch = branch_get(NULL);\n-\t\tif (!branch)\n-\t\t\tdie(_(message_detached_head_die), remote->name);\n-\t\tadd_refspec(branch->name);\n+\t\tsetup_push_current(remote, branch);\n \t\tbreak;\n \n \tcase PUSH_DEFAULT_NOTHING:\n-- \n1.8.3.1-721-g0a353d3\n"},{"id":"221754","messageId":"1372048388-16742-5-git-send-email-gitster@pobox.com","threadId":"34260","inReplyTo":"1372048388-16742-1-git-send-email-gitster@pobox.com","subject":"[PATCH 4/6] t/t5528-push-default: generalize test_push_*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T04:33:06Z","receivedAt":"2013-06-24T04:33:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Ramkumar Ramachandra \n\nThe setup creates two bare repositories: repo1 and repo2, but\ntest_push_commit() hard-codes checking in repo1 for the actual output.\nGeneralize it and its caller, test_push_success(), to optionally accept\na third argument to specify the name of the repository to check for\nactual output. We will use this in the next patch.\n\nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Junio C Hamano \n---\n t/t5528-push-default.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh\nindex 69ce6bf..db58e7f 100755\n--- a/t/t5528-push-default.sh\n+++ b/t/t5528-push-default.sh\n@@ -15,17 +15,19 @@ test_expect_success 'setup bare remotes' '\n \n # $1 = local revision\n # $2 = remote revision (tested to be equal to the local one)\n+# $3 = [optional] repo to check for actual output (repo1 by default)\n check_pushed_commit () {\n \tgit log -1 --format='%h %s' \"$1\" >expect &&\n-\tgit --git-dir=repo1 log -1 --format='%h %s' \"$2\" >actual &&\n+\tgit --git-dir=\"${3:-repo1}\" log -1 --format='%h %s' \"$2\" >actual &&\n \ttest_cmp expect actual\n }\n \n # $1 = push.default value\n # $2 = expected target branch for the push\n+# $3 = [optional] repo to check for actual output (repo1 by default)\n test_push_success () {\n \tgit -c push.default=\"$1\" push &&\n-\tcheck_pushed_commit HEAD \"$2\"\n+\tcheck_pushed_commit HEAD \"$2\" \"$3\"\n }\n \n # $1 = push.default value\n-- \n1.8.3.1-721-g0a353d3\n"},{"id":"221752","messageId":"1372048388-16742-6-git-send-email-gitster@pobox.com","threadId":"34260","inReplyTo":"1372048388-16742-1-git-send-email-gitster@pobox.com","subject":"[PATCH 5/6] t/t5528-push-default: test pushdefault workflows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T04:33:07Z","receivedAt":"2013-06-24T04:33:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Ramkumar Ramachandra \n\nIntroduce test_pushdefault_workflows(), and test that all push.default\nmodes work with central and triangular workflows as expected.\n\nSigned-off-by: Ramkumar Ramachandra \nSigned-off-by: Junio C Hamano \n---\n t/t5528-push-default.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 57 insertions(+)\n\ndiff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh\nindex db58e7f..6a5ac3a 100755\n--- a/t/t5528-push-default.sh\n+++ b/t/t5528-push-default.sh\n@@ -39,6 +39,26 @@ test_push_failure () {\n \ttest_cmp expect actual\n }\n \n+# $1 = success or failure\n+# $2 = push.default value\n+# $3 = branch to check for actual output (master or foo)\n+# $4 = [optional] switch to triangular workflow\n+test_pushdefault_workflow () {\n+\tworkflow=central\n+\tpushdefault=parent1\n+\tif test -n \"${4-}\"; then\n+\t\tworkflow=triangular\n+\t\tpushdefault=parent2\n+\tfi\n+\ttest_expect_success \"push.default = $2 $1 in $workflow workflows\" \"\n+\t\ttest_config branch.master.remote parent1 &&\n+\t\ttest_config branch.master.merge refs/heads/foo &&\n+\t\ttest_config remote.pushdefault $pushdefault &&\n+\t\ttest_commit commit-for-$2${4+-triangular} &&\n+\t\ttest_push_$1 $2 $3 ${4+repo2}\n+\t\"\n+}\n+\n test_expect_success '\"upstream\" pushes to configured upstream' '\n \tgit checkout master &&\n \ttest_config branch.master.remote parent1 &&\n@@ -115,4 +135,41 @@ test_expect_success 'push to existing branch, upstream configured with different\n \ttest_cmp expect-other-name actual-other-name\n '\n \n+# We are on 'master', which integrates with 'foo' from parent1\n+# remote (set in test_pushdefault_workflow helper). Push to\n+# parent1 in centralized, and push to parent2 in triangular workflow.\n+# The parent1 repository has 'master' and 'foo' branches, while\n+# the parent2 repository has only 'master' branch.\n+#\n+# test_pushdefault_workflow() arguments:\n+# $1 = success or failure\n+# $2 = push.default value\n+# $3 = branch to check for actual output (master or foo)\n+# $4 = [optional] switch to triangular workflow\n+\n+# update parent1's master (which is not our upstream)\n+test_pushdefault_workflow success current master\n+\n+# update parent1's foo (which is our upstream)\n+test_pushdefault_workflow success upstream foo\n+\n+# upsream is foo which is not the name of the current branch\n+test_pushdefault_workflow failure simple master\n+\n+# master and foo are updated\n+test_pushdefault_workflow success matching master\n+\n+# master is updated\n+test_pushdefault_workflow success current master triangular\n+\n+# upstream mode cannot be used in triangular\n+test_pushdefault_workflow failure upstream foo triangular\n+\n+# in triangular, 'simple' works as 'current' and update the branch\n+# with the same name.\n+test_pushdefault_workflow success simple master triangular\n+\n+# master is updated (parent2 does not have foo)\n+test_pushdefault_workflow success matching master triangular\n+\n test_done\n-- \n1.8.3.1-721-g0a353d3\n"},{"id":"221753","messageId":"1372048388-16742-7-git-send-email-gitster@pobox.com","threadId":"34260","inReplyTo":"1372048388-16742-1-git-send-email-gitster@pobox.com","subject":"[PATCH 6/6] push: honor branch.*.push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T04:33:08Z","receivedAt":"2013-06-24T04:33:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When branch.*.push configuration variable is defined for the current\nbranch, a lazy-typing \"git push\" (and \"git push there\") will push\nthe commit at the tip of the current branch to the destination and\nupdate the branch named by that variable.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/push.c | 18 +++++++++++++++++-\n remote.c | 5 +++++\n remote.h | 2 ++\n 3 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex f6c8047..a140b8e 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -185,6 +185,15 @@ static void warn_unspecified_push_default_configuration(void)\n \twarning(\"%s\\n\", _(warn_unspecified_push_default_msg));\n }\n \n+static void setup_per_branch_push(struct branch *branch)\n+{\n+\tstruct strbuf refspec = STRBUF_INIT;\n+\n+\tstrbuf_addf(&refspec, \"%s:%s\",\n+\t\t branch->name, branch->push_name);\n+\tadd_refspec(refspec.buf);\n+}\n+\n static int is_workflow_triagular(struct remote *remote)\n {\n \tstruct remote *fetch_remote = remote_get(NULL);\n@@ -194,7 +203,14 @@ static int is_workflow_triagular(struct remote *remote)\n static void setup_default_push_refspecs(struct remote *remote)\n {\n \tstruct branch *branch = branch_get(NULL);\n-\tint triangular = is_workflow_triagular(remote);\n+\tint triangular;\n+\n+\tif (branch->push_name) {\n+\t\tsetup_per_branch_push(branch);\n+\t\treturn;\n+\t}\n+\n+\ttriangular = is_workflow_triagular(remote);\n \n \tswitch (push_default) {\n \tdefault:\ndiff --git a/remote.c b/remote.c\nindex e71f66d..e033fef 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -372,6 +372,11 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n \t\t\tadd_merge(branch, xstrdup(value));\n+\t\t} else if (!strcmp(subkey, \".push\")) {\n+\t\t\tif (!value)\n+\t\t\t\treturn config_error_nonbool(key);\n+\t\t\tfree(branch->push_name);\n+\t\t\tbranch->push_name = xstrdup(value);\n \t\t}\n \t\treturn 0;\n \t}\ndiff --git a/remote.h b/remote.h\nindex cf56724..84e0f72 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -138,6 +138,8 @@ struct branch {\n \tstruct refspec **merge;\n \tint merge_nr;\n \tint merge_alloc;\n+\n+\tchar *push_name;\n };\n \n struct branch *branch_get(const char *name);\n-- \n1.8.3.1-721-g0a353d3\n"},{"id":"221758","messageId":"CALKQrgfAT9GhD-_tZHr9wRA6R6g7ttDWTsmUQXdWUhG-gwNBqQ@mail.gmail.com","threadId":"34260","inReplyTo":"1372048388-16742-4-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 3/6] push: change `simple` to accommodate triangular workflows","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-06-24T06:58:23Z","receivedAt":"2013-06-24T06:58:23Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, Jun 24, 2013 at 6:33 AM, Junio C Hamano wrote:\n> From: Ramkumar Ramachandra \n>\n> When remote.pushdefault or branch..pushremote is set (a triangular\n> workflow feature), master@{u} != origin, and push.default is set to\n> `upstream` or `simple`:\n>\n> $ git push\n> fatal: You are pushing to remote 'origin', which is not the upstream of\n> your current branch 'master', without telling me what to push\n> to update which remote branch.\n>\n> The very name of \"upstream\" indicates that it is only suitable for\n> use in central workflows; let us not even attempt to give it a new\n> meaning in triangular workflows, and error out as usual.\n>\n> However, the `simple` does not have this problem: it is poised to be\n> the default for Git 2.0, and we would definitely like it to do\n> something sensible in triangular workflows.\n>\n> Redefine \"simple\" as \"safer upstream\" for centralized workflow as\n> before, but work as \"current\" for triangular workflow.\n>\n> An earlier round of this change by mistake broke the safety for\n> \"simple\" mode we have had since day 1 of that mode to make sure that\n> the branch in the repository we update is set to be the one we fetch\n> and integrate with, but it has been fixed.\n\nShouldn't there be an acompanying test to demonstrate this mistake being fixed?\n\n> Reported-by: Leandro Lucarella \n> Signed-off-by: Ramkumar Ramachandra \n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/config.txt | 10 +++++++---\n> builtin/push.c | 43 +++++++++++++++++++++++++++++++------------\n> 2 files changed, 38 insertions(+), 15 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 5d8ff1a..cae6870 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1848,9 +1848,13 @@ push.default::\n> pushing to the same repository you would normally pull from\n> (i.e. central workflow).\n>\n> -* `simple` - like `upstream`, but refuses to push if the upstream\n> - branch's name is different from the local one. This is the safest\n> - option and is well-suited for beginners.\n> +* `simple` - in centralized workflow, work like `upstream` with an\n> + added safety to refuse to push if the upstream branch's name is\n> + different from the local one.\n> ++\n> +When pushing to a remote that is different from the remote you normally\n> +pull from, work as `current`. This is the safest option and is suited\n> +for beginners.\n> +\n> This mode will become the default in Git 2.0.\n>\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 2d84d10..f6c8047 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -120,10 +120,11 @@ static const char message_detached_head_die[] =\n> \"\\n\"\n> \" git push %s HEAD:\\n\");\n>\n> -static void setup_push_upstream(struct remote *remote, int simple)\n> +static void setup_push_upstream(struct remote *remote, struct branch *branch,\n> + int triangular)\n> {\n> struct strbuf refspec = STRBUF_INIT;\n> - struct branch *branch = branch_get(NULL);\n> +\n> if (!branch)\n> die(_(message_detached_head_die), remote->name);\n> if (!branch->merge_nr || !branch->merge || !branch->remote_name)\n> @@ -137,18 +138,29 @@ static void setup_push_upstream(struct remote *remote, int simple)\n> if (branch->merge_nr != 1)\n> die(_(\"The current branch %s has multiple upstream branches, \"\n> \"refusing to push.\"), branch->name);\n> - if (strcmp(branch->remote_name, remote->name))\n> + if (triangular)\n> die(_(\"You are pushing to remote '%s', which is not the upstream of\\n\"\n> \"your current branch '%s', without telling me what to push\\n\"\n> \"to update which remote branch.\"),\n> remote->name, branch->name);\n> - if (simple && strcmp(branch->refname, branch->merge[0]->src))\n> - die_push_simple(branch, remote);\n> +\n> + if (push_default == PUSH_DEFAULT_SIMPLE) {\n> + /* Additional safety */\n> + if (strcmp(branch->refname, branch->merge[0]->src))\n> + die_push_simple(branch, remote);\n> + }\n>\n> strbuf_addf(&refspec, \"%s:%s\", branch->name, branch->merge[0]->src);\n> add_refspec(refspec.buf);\n> }\n>\n> +static void setup_push_current(struct remote *remote, struct branch *branch)\n> +{\n> + if (!branch)\n> + die(_(message_detached_head_die), remote->name);\n> + add_refspec(branch->name);\n\nHere (and above) we add a refspec to tell Git exactly what to push\nfrom the local end, and into what on the remote end. Is it possible to\nend up with multiple simultaneous refspecs matching the same local\nref, but mapping to different remote refs? If so, which will win, and\ndoes that make sense?\n\n> +}\n> +\n> static char warn_unspecified_push_default_msg[] =\n> N_(\"push.default is unset; its implicit value is changing in\\n\"\n> \"Git 2.0 from 'matching' to 'simple'. To squelch this message\\n\"\n> @@ -173,9 +185,16 @@ static void warn_unspecified_push_default_configuration(void)\n> warning(\"%s\\n\", _(warn_unspecified_push_default_msg));\n> }\n>\n> +static int is_workflow_triagular(struct remote *remote)\n\ns/triagular/triangular/\n\n> +{\n> + struct remote *fetch_remote = remote_get(NULL);\n> + return (fetch_remote && fetch_remote != remote);\n\nThis changed from a strcmp() to a pointer compare. That might be safe,\ndepending on the sources of the two struct remote *, but I'm not sure.\n\n> +}\n> +\n> static void setup_default_push_refspecs(struct remote *remote)\n> {\n> - struct branch *branch;\n> + struct branch *branch = branch_get(NULL);\n> + int triangular = is_workflow_triagular(remote);\n>\n> switch (push_default) {\n> default:\n> @@ -188,18 +207,18 @@ static void setup_default_push_refspecs(struct remote *remote)\n> break;\n>\n> case PUSH_DEFAULT_SIMPLE:\n> - setup_push_upstream(remote, 1);\n> + if (triangular)\n> + setup_push_current(remote, branch);\n> + else\n> + setup_push_upstream(remote, branch, triangular);\n> break;\n>\n> case PUSH_DEFAULT_UPSTREAM:\n> - setup_push_upstream(remote, 0);\n> + setup_push_upstream(remote, branch, triangular);\n> break;\n>\n> case PUSH_DEFAULT_CURRENT:\n> - branch = branch_get(NULL);\n> - if (!branch)\n> - die(_(message_detached_head_die), remote->name);\n> - add_refspec(branch->name);\n> + setup_push_current(remote, branch);\n> break;\n>\n> case PUSH_DEFAULT_NOTHING:\n\nOtherwise, this looks good to me.\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"221759","messageId":"CALKQrgeSbY3spjTp_m7kgt4TkFi8B5pNpLQ8wvDD+qkCYa+Mww@mail.gmail.com","threadId":"34260","inReplyTo":"1372048388-16742-5-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 4/6] t/t5528-push-default: generalize test_push_*","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-06-24T06:58:31Z","receivedAt":"2013-06-24T06:58:31Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, Jun 24, 2013 at 6:33 AM, Junio C Hamano wrote:\n> From: Ramkumar Ramachandra \n>\n> The setup creates two bare repositories: repo1 and repo2, but\n> test_push_commit() hard-codes checking in repo1 for the actual output.\n> Generalize it and its caller, test_push_success(), to optionally accept\n> a third argument to specify the name of the repository to check for\n> actual output. We will use this in the next patch.\n>\n> Signed-off-by: Ramkumar Ramachandra \n> Signed-off-by: Junio C Hamano \n> ---\n> t/t5528-push-default.sh | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh\n> index 69ce6bf..db58e7f 100755\n> --- a/t/t5528-push-default.sh\n> +++ b/t/t5528-push-default.sh\n> @@ -15,17 +15,19 @@ test_expect_success 'setup bare remotes' '\n>\n> # $1 = local revision\n> # $2 = remote revision (tested to be equal to the local one)\n> +# $3 = [optional] repo to check for actual output (repo1 by default)\n> check_pushed_commit () {\n> git log -1 --format='%h %s' \"$1\" >expect &&\n> - git --git-dir=repo1 log -1 --format='%h %s' \"$2\" >actual &&\n> + git --git-dir=\"${3:-repo1}\" log -1 --format='%h %s' \"$2\" >actual &&\n\nIsn't ${3:-repo1} a bashism?\n\n> test_cmp expect actual\n> }\n>\n> # $1 = push.default value\n> # $2 = expected target branch for the push\n> +# $3 = [optional] repo to check for actual output (repo1 by default)\n> test_push_success () {\n> git -c push.default=\"$1\" push &&\n> - check_pushed_commit HEAD \"$2\"\n> + check_pushed_commit HEAD \"$2\" \"$3\"\n> }\n>\n> # $1 = push.default value\n> --\n> 1.8.3.1-721-g0a353d3\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n\n\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"221760","messageId":"CALKQrgebusbcDOTR+bFccoT4qzDJ8E=2LULutL0egEN9YY1v_g@mail.gmail.com","threadId":"34260","inReplyTo":"1372048388-16742-7-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 6/6] push: honor branch.*.push","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-06-24T06:58:40Z","receivedAt":"2013-06-24T06:58:40Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, Jun 24, 2013 at 6:33 AM, Junio C Hamano wrote:\n> When branch.*.push configuration variable is defined for the current\n> branch, a lazy-typing \"git push\" (and \"git push there\") will push\n> the commit at the tip of the current branch to the destination and\n> update the branch named by that variable.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/push.c | 18 +++++++++++++++++-\n> remote.c | 5 +++++\n> remote.h | 2 ++\n> 3 files changed, 24 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/push.c b/builtin/push.c\n> index f6c8047..a140b8e 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -185,6 +185,15 @@ static void warn_unspecified_push_default_configuration(void)\n> warning(\"%s\\n\", _(warn_unspecified_push_default_msg));\n> }\n>\n> +static void setup_per_branch_push(struct branch *branch)\n> +{\n> + struct strbuf refspec = STRBUF_INIT;\n> +\n> + strbuf_addf(&refspec, \"%s:%s\",\n> + branch->name, branch->push_name);\n> + add_refspec(refspec.buf);\n\nThis goes back to the question I raised in 3/6: If this code path adds\nrefspec \"foo:bar\", and - say - setup_push_current() has already added\nrefspec \"foo:foo\" (or simply \"foo\"), then do we end up pushing into\n\"foo\" or \"bar\"? To me, \"branch.*.push\" feels more specific than\n\"push.default = current\", so it would make sense that \"foo:bar\"\noverrides \"foo:foo\", but that is not obvious from the refspec alone.\nIMHO, this definitely needs some tests.\n\n> +}\n> +\n> static int is_workflow_triagular(struct remote *remote)\n> {\n> struct remote *fetch_remote = remote_get(NULL);\n> @@ -194,7 +203,14 @@ static int is_workflow_triagular(struct remote *remote)\n> static void setup_default_push_refspecs(struct remote *remote)\n> {\n> struct branch *branch = branch_get(NULL);\n> - int triangular = is_workflow_triagular(remote);\n> + int triangular;\n> +\n> + if (branch->push_name) {\n> + setup_per_branch_push(branch);\n> + return;\n\nI guess this return ensures that branch.*.push overrides push.default,\nbut might there be other sources of add_refspec() that would\ncomplicate things?\n\n> + }\n> +\n> + triangular = is_workflow_triagular(remote);\n>\n> switch (push_default) {\n> default:\n> diff --git a/remote.c b/remote.c\n> index e71f66d..e033fef 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -372,6 +372,11 @@ static int handle_config(const char *key, const char *value, void *cb)\n> if (!value)\n> return config_error_nonbool(key);\n> add_merge(branch, xstrdup(value));\n> + } else if (!strcmp(subkey, \".push\")) {\n> + if (!value)\n> + return config_error_nonbool(key);\n> + free(branch->push_name);\n> + branch->push_name = xstrdup(value);\n> }\n> return 0;\n> }\n> diff --git a/remote.h b/remote.h\n> index cf56724..84e0f72 100644\n> --- a/remote.h\n> +++ b/remote.h\n> @@ -138,6 +138,8 @@ struct branch {\n> struct refspec **merge;\n> int merge_nr;\n> int merge_alloc;\n> +\n> + char *push_name;\n> };\n>\n> struct branch *branch_get(const char *name);\n\nOtherwise, this patch, and the entire series looks good to me.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"221762","messageId":"CALkWK0kDwMq-ADRPShOdCy2Vh+zj9J9A0A4CFv21aKZFeiMs+w@mail.gmail.com","threadId":"34260","inReplyTo":"1372048388-16742-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 0/6] Reroll of rr/triangular-push-fix","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T07:21:24Z","receivedAt":"2013-06-24T07:21:24Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> [PATCH 3/6] push: change `simple` to accommodate triangular workflows\n>\n> Sqaushed in the fix to keep the semantics of \"simple\" when used in\n> the centralized workflow the same as before.\n\nYeah, I'm worried about this as I pointed out earlier. I don't like\nerroring out when no branch.$branch.merge is not explicitly set, when\nthe 95% usecase is not naming local branches differently from remote\nbranches (oh, and I already pointed out how difficult it is to set the\ndamn thing). So, I'm working on a series to make\nbranch.$branch.merge default to refs/heads/$branch. Yes, I'm aware of\nyour \"argument\":\n\n> We already have a sane default, which is to error out. We do not\n> need your broken default.\n\nI hope (perhaps foolishly) to persuade you nevertheless. I fear that\nif this series solidifies before I get there, we'll be stuck with this\nstupid erroring-out behavior forever.\n"},{"id":"221763","messageId":"7v7ghkdl2p.fsf@alter.siamese.dyndns.org","threadId":"34260","inReplyTo":"CALKQrgeSbY3spjTp_m7kgt4TkFi8B5pNpLQ8wvDD+qkCYa+Mww@mail.gmail.com","subject":"Re: [PATCH 4/6] t/t5528-push-default: generalize test_push_*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T07:28:14Z","receivedAt":"2013-06-24T07:28:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n>> + git --git-dir=\"${3:-repo1}\" log -1 --format='%h %s' \"$2\" >actual &&\n>\n> Isn't ${3:-repo1} a bashism?\n\nI do not think so. But now I looked at it again, I think I would\nuse ${3-repo1} form in this case myself. No caller passes an empty\nstring to the third place.\n"},{"id":"221765","messageId":"7v38s8dkd8.fsf@alter.siamese.dyndns.org","threadId":"34260","inReplyTo":"CALKQrgfAT9GhD-_tZHr9wRA6R6g7ttDWTsmUQXdWUhG-gwNBqQ@mail.gmail.com","subject":"Re: [PATCH 3/6] push: change `simple` to accommodate triangular workflows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T07:43:31Z","receivedAt":"2013-06-24T07:43:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n>> +static void setup_push_current(struct remote *remote, struct branch *branch)\n>> +{\n>> + if (!branch)\n>> + die(_(message_detached_head_die), remote->name);\n>> + add_refspec(branch->name);\n>\n> Here (and above) we add a refspec to tell Git exactly what to push\n> from the local end, and into what on the remote end.\n\nCorrect.\n\n> Is it possible to end up with multiple simultaneous refspecs\n> matching the same local ref, but mapping to different remote refs?\n\nSorry, I don't follow. If you say \"push.default = current\" and you\ndo not give any other stronger clue (e.g. \"git push origin master\"\non the command line, or \"git push [origin]\" with remote.origin.push\nconfigured), the above function is called and sets up your current\nbranch to be pushed to the same.\n\nIt is a bit more interesting for \"push.default = upstream\", which is\nfor centralized workflow. If you forked frotz and nitfol branches\nboth from their master, e.g.\n\n\t$ git checkout -t -b frotz origin/master\n\t$ git checkout -t -b nitfol origin/master\n\nafter having worked on one of the branches, when you want to push it\nback, the result of working on the topic branch goes back to master,\nbut I think that is what you want in the centralized workflow. If\nit fast-forwards, you are fine, and if it does not, you will fetch\nyour upstream, i.e. their master, integrate your work with it, and\nthen push it back. At that point, you are playing the role of the\nintegrator of the shared master branch, because what you do on your\ntopic branch when you integrate others' work from master is exactly\nthat---you are not perfecting the theme you wanted to achieve on\nyour topic branch, but are integrating that result into shared\nmaster to advance the overall state of the project. So pushing the\nresult back to 'master' makes perfect sense. After that, when you\nhave to restart your work on the other branch, you may first \"pull\n--rebase\" before continuing, or you may just keep going with your\nwork based on a tad old origin/master. But when you finish working\non that topic and are about to push it out, you would be doing the\nsame \"tentatively don the central integrator's hat\", and again it\nmakes sense to push the result to 'master'.\n\nSo in that sense, it is not \"which one wins\". It is more like \"you\ncan push only after you become up to date, so there isn't one branch\noverwriting the other one.\"\n\nThat is how I view it, anyway.\n\n cf. http://git-blame.blogspot.com/2013/06/fun-with-various-workflows-1.html\n\n>> +static int is_workflow_triagular(struct remote *remote)\n>\n> s/triagular/triangular/\n\nThanks.\n\n>\n>> +{\n>> + struct remote *fetch_remote = remote_get(NULL);\n>> + return (fetch_remote && fetch_remote != remote);\n>\n> This changed from a strcmp() to a pointer compare. That might be safe,\n> depending on the sources of the two struct remote *, but I'm not sure.\n\nGiven the way remote_get() works, it should be correct, I think.\n"},{"id":"221767","messageId":"CALkWK0mAyDb3AjH0s3j2gRRDckx=a2nr9MR+O6gEtwX2MSJrtw@mail.gmail.com","threadId":"34260","inReplyTo":"CALKQrgfAT9GhD-_tZHr9wRA6R6g7ttDWTsmUQXdWUhG-gwNBqQ@mail.gmail.com","subject":"Re: [PATCH 3/6] push: change `simple` to accommodate triangular workflows","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T07:46:47Z","receivedAt":"2013-06-24T07:46:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Johan Herland wrote:\n>> An earlier round of this change by mistake broke the safety for\n>> \"simple\" mode we have had since day 1 of that mode to make sure that\n>> the branch in the repository we update is set to be the one we fetch\n>> and integrate with, but it has been fixed.\n>\n> Shouldn't there be an acompanying test to demonstrate this mistake being fixed?\n\nRead \"earlier iteration\": it didn't get merged.\n\n>> +static void setup_push_current(struct remote *remote, struct branch *branch)\n>> +{\n>> + if (!branch)\n>> + die(_(message_detached_head_die), remote->name);\n>> + add_refspec(branch->name);\n>\n> Here (and above) we add a refspec to tell Git exactly what to push\n> from the local end, and into what on the remote end.\n\nNope, we add the refspec \"foo\", without the :destination part. The\nremote end is unspecified (and defaults to \"foo\", but that is in the\ntransport layer).\n\n> Is it possible to\n> end up with multiple simultaneous refspecs matching the same local\n> ref, but mapping to different remote refs? If so, which will win, and\n> does that make sense?\n\nIt is impossible. We either:\n\n- Get an explicit refspec from the user and never run\nsetup_default_push_refspecs() to begin with.\n\n- Run setup_push_refspecs() and add *one* refspec depending on the\npush.default value.\n"},{"id":"221766","messageId":"7vy5a0c5mu.fsf@alter.siamese.dyndns.org","threadId":"34260","inReplyTo":"CALKQrgebusbcDOTR+bFccoT4qzDJ8E=2LULutL0egEN9YY1v_g@mail.gmail.com","subject":"Re: [PATCH 6/6] push: honor branch.*.push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T07:47:05Z","receivedAt":"2013-06-24T07:47:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n>> +static void setup_per_branch_push(struct branch *branch)\n>> +{\n>> + struct strbuf refspec = STRBUF_INIT;\n>> +\n>> + strbuf_addf(&refspec, \"%s:%s\",\n>> + branch->name, branch->push_name);\n>> + add_refspec(refspec.buf);\n>\n> This goes back to the question I raised in 3/6: If this code path adds\n> refspec \"foo:bar\", and - say - setup_push_current() has already added\n> refspec \"foo:foo\" (or simply \"foo\"), then do we end up pushing into\n> \"foo\" or \"bar\"?\n\nI think you answered your own question below with the \"return\".\n\n>> @@ -194,7 +203,14 @@ static int is_workflow_triagular(struct remote *remote)\n>> static void setup_default_push_refspecs(struct remote *remote)\n>> {\n>> struct branch *branch = branch_get(NULL);\n>> - int triangular = is_workflow_triagular(remote);\n>> + int triangular;\n>> +\n>> + if (branch->push_name) {\n>> + setup_per_branch_push(branch);\n>> + return;\n>\n> I guess this return ensures that branch.*.push overrides push.default,\n> but might there be other sources of add_refspec() that would\n> complicate things?\n\nThe default-push-refspecs is meant to be used only when there is no\nother stronger clue given by the user (e.g. refspec on the command\nline, e.g. \"git push there master\", pushing with configured refspecs\non remote.$name.push), so I think it is a bug if somebody calls this\nfunction when there is other source.\n"},{"id":"221770","messageId":"CALkWK0=aDkCMB3ROSuH1crTmQKjCFyCmvJfvOTiQnBo4+NckBA@mail.gmail.com","threadId":"34260","inReplyTo":"1372048388-16742-7-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 6/6] push: honor branch.*.push","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T07:58:09Z","receivedAt":"2013-06-24T07:58:09Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> @@ -194,7 +203,14 @@ static int is_workflow_triagular(struct remote *remote)\n> static void setup_default_push_refspecs(struct remote *remote)\n> {\n> struct branch *branch = branch_get(NULL);\n> - int triangular = is_workflow_triagular(remote);\n> + int triangular;\n> +\n> + if (branch->push_name) {\n> + setup_per_branch_push(branch);\n> + return;\n> + }\n\nThe most obvious question comes first: what result can I expect when\nthis interacts with remote..push?\n\nWhy did you design this feature like this? Will the user _not_ want\nrefspec mapping except when pushing out the current branch with a\nplain \"git push\"?\n\nAlso, you managed to throw out all safety out the window. What\nhappens when the user does:\n\n # on branch master, derived from origin\n $ git push ram\n\nAnd branch.master.push is set to next? Will you let her shoot herself\nin the foot like this?\n"},{"id":"221771","messageId":"7vppvcc51j.fsf@alter.siamese.dyndns.org","threadId":"34260","inReplyTo":"CALKQrgfAT9GhD-_tZHr9wRA6R6g7ttDWTsmUQXdWUhG-gwNBqQ@mail.gmail.com","subject":"Re: [PATCH 3/6] push: change `simple` to accommodate triangular workflows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T07:59:52Z","receivedAt":"2013-06-24T07:59:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n>> An earlier round of this change by mistake broke the safety for\n>> \"simple\" mode we have had since day 1 of that mode to make sure that\n>> the branch in the repository we update is set to be the one we fetch\n>> and integrate with, but it has been fixed.\n>\n> Shouldn't there be an acompanying test to demonstrate this mistake being fixed?\n\nAn operation that has to expect failure due to safety was disabled\nby the broken version. The squashed end result reverts that change\nto the test, to make sure we did not break the safety.\n"},{"id":"221772","messageId":"7vli60c4gu.fsf@alter.siamese.dyndns.org","threadId":"34260","inReplyTo":"CALkWK0kDwMq-ADRPShOdCy2Vh+zj9J9A0A4CFv21aKZFeiMs+w@mail.gmail.com","subject":"Re: [PATCH 0/6] Reroll of rr/triangular-push-fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T08:12:17Z","receivedAt":"2013-06-24T08:12:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> I hope (perhaps foolishly) to persuade you nevertheless. I fear that\n> if this series solidifies before I get there, we'll be stuck with this\n> stupid erroring-out behavior forever.\n\nI do not think it is \"stupid\" at all.\n\n'simple' is supposed to be an easy and safe default to help new\npeople. Your original patch to change its semantics for the central\nworkflow from the current 'make sure upstream is set and set to the\nsame name' to 'anything goes' is making the mode more dangerous than\nthe corresponding 'upstream'. Such a mode may have its place, but\nlabelling such a mode with rough edges as 'simple' and forcing it on\nnew people _is_ stupid, IMHO.\n\nIn any case, the good news is, if you start strict, and if it turns\nout to be stricter than necessary, it is easier to loosen it later,\nbecause nobody would be relying on an operation to _fail_.\n\nIf you start too loose without safety, however, it is a lot harder\nto tighten it later when it turns out that safety helps new people.\n\nSince the beginning of this series, our working assumption for\ntriangular-simple' has been that it can just turn into a straight\n'current'. But given that 'simple' is supposed to be an easy and\nsafe default for new people, I suspect that it should be a bit more\nstrict.\n\nFor example, if you are on a random topic branch and say \"git push\",\nalways pushing it out may not be a very sensible thing to do, and it\nmight be safer if we restrict 'triangular-simple' to push out the\ncurrent branch to the branch of the same name, but only when such a\nbranch already exists at the remote end, or something.\n"},{"id":"221774","messageId":"7vhagndis4.fsf@alter.siamese.dyndns.org","threadId":"34260","inReplyTo":"CALkWK0=aDkCMB3ROSuH1crTmQKjCFyCmvJfvOTiQnBo4+NckBA@mail.gmail.com","subject":"Re: [PATCH 6/6] push: honor branch.*.push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T08:17:47Z","receivedAt":"2013-06-24T08:17:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Junio C Hamano wrote:\n>> @@ -194,7 +203,14 @@ static int is_workflow_triagular(struct remote *remote)\n>> static void setup_default_push_refspecs(struct remote *remote)\n>> {\n>> struct branch *branch = branch_get(NULL);\n>> - int triangular = is_workflow_triagular(remote);\n>> + int triangular;\n>> +\n>> + if (branch->push_name) {\n>> + setup_per_branch_push(branch);\n>> + return;\n>> + }\n>\n> The most obvious question comes first: what result can I expect when\n> this interacts with remote..push?\n\nNow you bring it up, the branch.*.push may want to be more specific\n(when I am on _this_ branch, do this) than remote.*.push (when I am\npushing to that remote, I want this to happen in general), but this\ndefault codepath would not be exercised when you have remote.*.push,\nso the logic may need to be moved higher up in the foodchain.\n\n> Also, you managed to throw out all safety out the window. What\n> happens when the user does:\n>\n> # on branch master, derived from origin\n> $ git push ram\n>\n> And branch.master.push is set to next? Will you let her shoot herself\n> in the foot like this?\n\nIt is not shooting in the foot, if branch.master.push is explicitly\nset to update next. I do not see any issue in that part.\n\nBut the relative strength betweenh branch.*.push and remote.*.push\nmay need to be thought out. I haven't.\n"},{"id":"221775","messageId":"CALKQrge4Nac4nk1RSnOFwE4-BNtHRNDSZHh9ihPY7ERd5jQ2Ng@mail.gmail.com","threadId":"34260","inReplyTo":"7v7ghkdl2p.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/6] t/t5528-push-default: generalize test_push_*","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-06-24T08:33:10Z","receivedAt":"2013-06-24T08:33:10Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, Jun 24, 2013 at 9:28 AM, Junio C Hamano wrote:\n> Johan Herland writes:\n>\n>>> + git --git-dir=\"${3:-repo1}\" log -1 --format='%h %s' \"$2\" >actual &&\n>>\n>> Isn't ${3:-repo1} a bashism?\n>\n> I do not think so. But now I looked at it again, I think I would\n> use ${3-repo1} form in this case myself. No caller passes an empty\n> string to the third place.\n\nOk, I have to admit that I'm not at all sure where the line between sh\nand bash goes when it comes to ${magic}... Is there any good\ndocumentation on what is in sh and what is not?\n\n...Johan\n\n\n\n-- \nJohan Herland, \nwww.herland.net\nc\n"},{"id":"221776","messageId":"CAPig+cRgN7yMGWc6zM8RiLrFxOcsMcia_BKiYT6fgQX_toA_QQ@mail.gmail.com","threadId":"34260","inReplyTo":"CALKQrge4Nac4nk1RSnOFwE4-BNtHRNDSZHh9ihPY7ERd5jQ2Ng@mail.gmail.com","subject":"Re: [PATCH 4/6] t/t5528-push-default: generalize test_push_*","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-06-24T08:44:43Z","receivedAt":"2013-06-24T08:44:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 24, 2013 at 4:33 AM, Johan Herland wrote:\n> On Mon, Jun 24, 2013 at 9:28 AM, Junio C Hamano wrote:\n>> Johan Herland writes:\n>>\n>>>> + git --git-dir=\"${3:-repo1}\" log -1 --format='%h %s' \"$2\" >actual &&\n>>>\n>>> Isn't ${3:-repo1} a bashism?\n>>\n>> I do not think so. But now I looked at it again, I think I would\n>> use ${3-repo1} form in this case myself. No caller passes an empty\n>> string to the third place.\n>\n> Ok, I have to admit that I'm not at all sure where the line between sh\n> and bash goes when it comes to ${magic}... Is there any good\n> documentation on what is in sh and what is not?\n\nPOSIX: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02\n"},{"id":"221777","messageId":"CALKQrgfCR10nfP9tVuHyhCPOfe3XVvBzzQayhM7b9Ra5ZDGnMw@mail.gmail.com","threadId":"34260","inReplyTo":"CALkWK0mAyDb3AjH0s3j2gRRDckx=a2nr9MR+O6gEtwX2MSJrtw@mail.gmail.com","subject":"Re: [PATCH 3/6] push: change `simple` to accommodate triangular workflows","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-06-24T08:48:16Z","receivedAt":"2013-06-24T08:48:16Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, Jun 24, 2013 at 9:46 AM, Ramkumar Ramachandra\n wrote:\n> Johan Herland wrote:\n>>> +static void setup_push_current(struct remote *remote, struct branch *branch)\n>>> +{\n>>> + if (!branch)\n>>> + die(_(message_detached_head_die), remote->name);\n>>> + add_refspec(branch->name);\n>>\n>> Here (and above) we add a refspec to tell Git exactly what to push\n>> from the local end, and into what on the remote end.\n>\n> Nope, we add the refspec \"foo\", without the :destination part. The\n> remote end is unspecified (and defaults to \"foo\", but that is in the\n> transport layer).\n\nOk, so \"foo\" is not always semantically equivalent to \"foo:foo\", and\nwhen adding \"foo:bar\" it is always considered more specific than (and\nsuperior to) \"foo\". I think that makes sense.\n\n>> Is it possible to\n>> end up with multiple simultaneous refspecs matching the same local\n>> ref, but mapping to different remote refs? If so, which will win, and\n>> does that make sense?\n>\n> It is impossible. We either:\n>\n> - Get an explicit refspec from the user and never run\n> setup_default_push_refspecs() to begin with.\n>\n> - Run setup_push_refspecs() and add *one* refspec depending on the\n> push.default value.\n\nThanks, that's what I wanted to hear. But then, does it make sense to\nsay that we will only ever have exactly _one_ push refspec in the\ncurrent context, and we should therefore replace the \"static const\nchar **refspec;\" string array with a single \"static const char\n*refspec;\" string? That would make it obvious that there is no room\nfor ambiguity with overlapping refspecs.\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"221778","messageId":"CALKQrgda0h+FuxepbZfKjRob9kXwU=GPNCLT_OCSps_2ggAmbg@mail.gmail.com","threadId":"34260","inReplyTo":"7vppvcc51j.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/6] push: change `simple` to accommodate triangular workflows","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-06-24T08:48:37Z","receivedAt":"2013-06-24T08:48:37Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, Jun 24, 2013 at 9:59 AM, Junio C Hamano wrote:\n> Johan Herland writes:\n>>> An earlier round of this change by mistake broke the safety for\n>>> \"simple\" mode we have had since day 1 of that mode to make sure that\n>>> the branch in the repository we update is set to be the one we fetch\n>>> and integrate with, but it has been fixed.\n>>\n>> Shouldn't there be an acompanying test to demonstrate this mistake being fixed?\n>\n> An operation that has to expect failure due to safety was disabled\n> by the broken version. The squashed end result reverts that change\n> to the test, to make sure we did not break the safety.\n\nOk, thanks.\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"221779","messageId":"CALKQrgdEszpXO3noA3xAp=Q3b6bL=XUd00wRoR4K1H9dJ3beKg@mail.gmail.com","threadId":"34260","inReplyTo":"CAPig+cRgN7yMGWc6zM8RiLrFxOcsMcia_BKiYT6fgQX_toA_QQ@mail.gmail.com","subject":"Re: [PATCH 4/6] t/t5528-push-default: generalize test_push_*","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-06-24T09:45:59Z","receivedAt":"2013-06-24T09:45:59Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, Jun 24, 2013 at 10:44 AM, Eric Sunshine wrote:\n> On Mon, Jun 24, 2013 at 4:33 AM, Johan Herland wrote:\n>> On Mon, Jun 24, 2013 at 9:28 AM, Junio C Hamano wrote:\n>>> Johan Herland writes:\n>>>\n>>>>> + git --git-dir=\"${3:-repo1}\" log -1 --format='%h %s' \"$2\" >actual &&\n>>>>\n>>>> Isn't ${3:-repo1} a bashism?\n>>>\n>>> I do not think so. But now I looked at it again, I think I would\n>>> use ${3-repo1} form in this case myself. No caller passes an empty\n>>> string to the third place.\n>>\n>> Ok, I have to admit that I'm not at all sure where the line between sh\n>> and bash goes when it comes to ${magic}... Is there any good\n>> documentation on what is in sh and what is not?\n>\n> POSIX: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02\n\nThanks! Learn something new every day, I guess. :)\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"221791","messageId":"CALkWK0=L4X=5UrCPygoUeLo9byvRKh4bez0DVB5iqatLvdEEfA@mail.gmail.com","threadId":"34260","inReplyTo":"7vli60c4gu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/6] Reroll of rr/triangular-push-fix","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T13:51:05Z","receivedAt":"2013-06-24T13:51:05Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> 'simple' is supposed to be an easy and safe default to help new\n> people. Your original patch to change its semantics for the central\n> workflow from the current 'make sure upstream is set and set to the\n> same name' to 'anything goes' is making the mode more dangerous than\n> the corresponding 'upstream'. Such a mode may have its place, but\n> labelling such a mode with rough edges as 'simple' and forcing it on\n> new people _is_ stupid, IMHO.\n\nOh, I agree that \"anything goes\" was the wrong approach. However, I\nthink a sane default for branch.$branch.merge is a good way forward.\n\n> In any case, the good news is, if you start strict, and if it turns\n> out to be stricter than necessary, it is easier to loosen it later,\n> because nobody would be relying on an operation to _fail_.\n\nOkay, I will quote you if you raise issues about \"preserving how it\nhas historically been functioning\" when I complete the upstream-fix\ntopic.\n\nThere's no need to stall this series then: [1/6] to [5/6] largely look\ngood-to-merge; drop [6/6], as it needs more thought.\n"},{"id":"221794","messageId":"CALkWK0mC_SDR=wdPHkM5YBUXi3ntfBXfmVF468Nt_Ssembuntg@mail.gmail.com","threadId":"34260","inReplyTo":"CALKQrgfCR10nfP9tVuHyhCPOfe3XVvBzzQayhM7b9Ra5ZDGnMw@mail.gmail.com","subject":"Re: [PATCH 3/6] push: change `simple` to accommodate triangular workflows","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T14:13:12Z","receivedAt":"2013-06-24T14:13:12Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Johan Herland wrote:\n> But then, does it make sense to\n> say that we will only ever have exactly _one_ push refspec in the\n> current context, and we should therefore replace the \"static const\n> char **refspec;\" string array with a single \"static const char\n> *refspec;\" string? That would make it obvious that there is no room\n> for ambiguity with overlapping refspecs.\n\nMultiple refspecs can be specified on the command-line; set_refspecs()\nis responsible for calling add_refspec() multiple times for each\nrefspec, and _that_ is the primary use of the \"refspec\" variable. The\nsingle add_refspec() invocation in the push.default switch is a\nspecial case that reuses the variable.\n"},{"id":"221796","messageId":"CALkWK0kqyV+MTW8jDDBt-qEB1R7yvD+n4nTxwvW0QjA13J2=2A@mail.gmail.com","threadId":"34260","inReplyTo":"7vhagndis4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/6] push: honor branch.*.push","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T14:19:38Z","receivedAt":"2013-06-24T14:19:38Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n>> # on branch master, derived from origin\n>> $ git push ram\n>>\n>> And branch.master.push is set to next? Will you let her shoot herself\n>> in the foot like this?\n>\n> It is not shooting in the foot, if branch.master.push is explicitly\n> set to update next. I do not see any issue in that part.\n\nThe question does not pertain to master being mapped to next; it\npertains to central-workflow versus triangular-workflow: origin versus\nram. If the user has set push.default to upstream, she _expects_\ntriangular pushes to always be denied, and this is the first violation\nof that rule. I'm tilting towards building a dependency between\nbranch..push and push.default.\n"},{"id":"221819","messageId":"CALKQrgdQRtfGmfaq9sS=VbezV2RVs9WJwUQBqVF+1Ozn7FxdJw@mail.gmail.com","threadId":"34260","inReplyTo":"CALkWK0kqyV+MTW8jDDBt-qEB1R7yvD+n4nTxwvW0QjA13J2=2A@mail.gmail.com","subject":"Re: [PATCH 6/6] push: honor branch.*.push","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-06-24T15:23:58Z","receivedAt":"2013-06-24T15:23:58Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, Jun 24, 2013 at 4:19 PM, Ramkumar Ramachandra\n wrote:\n> Junio C Hamano wrote:\n>>> # on branch master, derived from origin\n>>> $ git push ram\n>>>\n>>> And branch.master.push is set to next? Will you let her shoot herself\n>>> in the foot like this?\n>>\n>> It is not shooting in the foot, if branch.master.push is explicitly\n>> set to update next. I do not see any issue in that part.\n>\n> The question does not pertain to master being mapped to next; it\n> pertains to central-workflow versus triangular-workflow: origin versus\n> ram. If the user has set push.default to upstream, she _expects_\n> triangular pushes to always be denied, and this is the first violation\n> of that rule. I'm tilting towards building a dependency between\n> branch..push and push.default.\n\nI haven't followed this topic closely, and the concern described below\nis probably a consequence of that. Please ignore if my worries are\nunfounded.\n\nIt seems to me like we're adding (or have already added) quite a bit\nof config variables and command-line options for specifying (at\nvarying levels of specificity and overridability) either the remote to\npush to ($remote), or what ref to push into on the remote\n($remote_ref). It worries me that we allow $remote and $remote_ref to\nbe set _separately_ and at separate levels of specificity. I wonder if\nthis too easily allows users to shoot themselves in the foot by\nspecifying $remote in one place (e.g. on the command line), then (in\ntheir mind - unrelated) specifying $remote_ref in another place (e.g.\nbranch.foo.push), and then being negatively surprised when the two\nconspire to push into the \"wrong\" $remote and/or $remote_ref.\n\nI haven't yet dug deep enough to figure out an obvious failure mode\n(and I probably should not have sent this email until I'd found one),\nbut I wonder if we'd be better off forcing the $remote and $remote_ref\nconfiguration for a given branch to appear as more of a single unit.\n\nWhat if, when setting up tracking for a given branch, we immediately\nspecified its complete pull and push targets?\n\nFor example, when in a centralized workflow (e.g. push.default =\nupstream) and we're checking out local branch foo from origin's foo,\nwe could set up the following configuration [1]:\n\n[branch \"foo\"]\n pull = origin/foo\n push = origin/foo\n\nIn a triangular workflow (assuming we had configuration to specify\nsuch, and also a default push remote), we could then instead set up\nthe following config:\n\n[branch \"foo\"]\n pull = origin/foo\n push = my_public/foo\n\nThis leaves no ambiguity for even the most novice user as to the pull\nand push targets for a given branch, and it's also easy to change it,\neither by editing the config file directly, or by using hypothetical\ncommands:\n\n git branch foo --pulls-from=origin/bar\n git branch foo --pushes-to=other_repo/foo\n\nAny \"git pull\" without arguments will fail if branch..pull is\nunset or invalid. Likewise any \"git push\" without arguments will fail\nif branch..push is unset or invalid.\n\nObviously, specifying the remote and/or refspec on the command-line\nwould still override, as it does today, but for the argument-less\nforms of \"git pull\" and \"git push\", the hierarchy of options and\ndefaults being consulted to figure out $remote and $remote_ref would\nbe small and easily understandable.\n\n\n...Johan\n\n\n[1]: I do realize that I'm abusing the \"foo/bar\" notation to mean\n\"$remote/$ref\", and that this does not work in the general case where\nboth remote names and ref names may contain slashes, or when remote\nnames don't correspond to eponymous ref namespaces...\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"221821","messageId":"7vy59zbjns.fsf@alter.siamese.dyndns.org","threadId":"34260","inReplyTo":"CALkWK0kqyV+MTW8jDDBt-qEB1R7yvD+n4nTxwvW0QjA13J2=2A@mail.gmail.com","subject":"Re: [PATCH 6/6] push: honor branch.*.push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T15:41:43Z","receivedAt":"2013-06-24T15:41:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Junio C Hamano wrote:\n>>> # on branch master, derived from origin\n>>> $ git push ram\n>>>\n>>> And branch.master.push is set to next? Will you let her shoot herself\n>>> in the foot like this?\n>>\n>> It is not shooting in the foot, if branch.master.push is explicitly\n>> set to update next. I do not see any issue in that part.\n>\n> The question does not pertain to master being mapped to next; it\n> pertains to central-workflow versus triangular-workflow: origin versus\n> ram. If the user has set push.default to upstream, she _expects_\n> triangular pushes to always be denied,...\n\nIf the user said \"git push\" without an explicit request to push to\n\"ram\", and if branch.master.pushremote was not set to \"ram\", and\nstill the command \"git push\" pushed the branch to \"ram\", then I\nwould understand what you are worried about, but otherwise I do not\nsee how what you are saying makes sense.\n\nA safety valve is different from a straight-jacket. If you are\nworking largely with a central repository and have push.default set\nto upstream, are you now disallowed to push out things to other\nplaces to ask help from your colleague to check your wip? Why?\n\nOr are you saying that with push.default set to upstream, only these\ntwo forms should be allowed?\n\n $ git push ;# no destination, no refspec\n $ git push there ref:spec ;# both explicitly specified\n"},{"id":"221823","messageId":"7vtxknbifp.fsf@alter.siamese.dyndns.org","threadId":"34260","inReplyTo":"CALKQrgdQRtfGmfaq9sS=VbezV2RVs9WJwUQBqVF+1Ozn7FxdJw@mail.gmail.com","subject":"Re: [PATCH 6/6] push: honor branch.*.push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T16:08:10Z","receivedAt":"2013-06-24T16:08:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> I haven't yet dug deep enough to figure out an obvious failure mode\n> (and I probably should not have sent this email until I'd found one),\n> but I wonder if we'd be better off forcing the $remote and $remote_ref\n> configuration for a given branch to appear as more of a single unit.\n\nThat sounds sensible. I could see perhaps we would want to require,\nfor branch.*.push to be effective, branch.*.pushremote must be set\n(honestly speaking, branch.*.push is not my itch and I'd probably be\nhappier if we didn't add it in the first place, though ;-).\n\n> What if, when setting up tracking for a given branch, we immediately\n> specified its complete pull and push targets?\n>\n> For example, when in a centralized workflow (e.g. push.default =\n> upstream) and we're checking out local branch foo from origin's foo,\n> we could set up the following configuration [1]:\n>\n> [branch \"foo\"]\n> pull = origin/foo\n> push = origin/foo\n\nThey should both be refs/heads/foo, as these are meant to name the\nname in _their_ repository. I see what you are saying, but the\nbehaviour you want happens without branch.foo.push, and the addition\nmay be redundant. I do not immediately see what it is buying us.\n\nOther than that when the user stops being centralized and starts to\npush to his own publishing repository, branch.*.push needs to be\nremoved in addition to flipping push.default from upstream to\ncurrent, that is.\n\n> In a triangular workflow (assuming we had configuration to specify\n> such, and also a default push remote), we could then instead set up\n> the following config:\n>\n> [branch \"foo\"]\n> pull = origin/foo\n> push = my_public/foo\n\nAgain, these are both refs/heads/foo.\n\n> This leaves no ambiguity for even the most novice user as to the pull\n> and push targets for a given branch, and it's also easy to change it,\n> either by editing the config file directly, or by using hypothetical\n> commands:\n>\n> git branch foo --pulls-from=origin/bar\n> git branch foo --pushes-to=other_repo/foo\n\nBut you need to do that for _all_ branches when you acquire your own\npublishing point; isn't that a rather cumbersome usability glitch?\n\n> Obviously, specifying the remote and/or refspec on the command-line\n> would still override, as it does today, but for the argument-less\n> forms of \"git pull\" and \"git push\", the hierarchy of options and\n> defaults being consulted to figure out $remote and $remote_ref would\n> be small and easily understandable.\n\nNot really.\n\nIn addition to \"you need to run around and change configuration for\nall branches\" issue, you can never do push.default=matching, if you\nalways set branch.foo.push and made it stronger than push.default,\nno?\n"},{"id":"221824","messageId":"CALkWK0=a40hACGYApN7odrTytDmoRG_00_Q=EuaHuQ_=h8LBkg@mail.gmail.com","threadId":"34260","inReplyTo":"7vy59zbjns.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/6] push: honor branch.*.push","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T16:09:46Z","receivedAt":"2013-06-24T16:09:46Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> If the user said \"git push\" without an explicit request to push to\n> \"ram\", and if branch.master.pushremote was not set to \"ram\", and\n> still the command \"git push\" pushed the branch to \"ram\", then I\n> would understand what you are worried about, but otherwise I do not\n> see how what you are saying makes sense.\n\nWe currently have no system to differentiate between those two cases.\n\n> A safety valve is different from a straight-jacket. If you are\n> working largely with a central repository and have push.default set\n> to upstream, are you now disallowed to push out things to other\n> places to ask help from your colleague to check your wip? Why?\n\nI've been harping about how this safety valve is poorly done, and I'm\nthe first person interested in loosening it. But I do not think this\nis the way to do it: drop in branch..push magic, and make\nupstream suddenly work with triangular workflows?\n\n> Or are you saying that with push.default set to upstream, only these\n> two forms should be allowed?\n>\n> $ git push ;# no destination, no refspec\n> $ git push there ref:spec ;# both explicitly specified\n\nNo, no. What I meant is:\n\n From the documentation of push.default, I _expect_ upstream to kick\n in only in the first case. In the second case, I _know_ that my\n push.default is inconsequential.\n\nWith branch..push magic, you've sneaked in a push refspec under\nthe carpet, and the first form suddenly doesn't care about\npush.default anymore. remote..push is also an under-the-carpet\nimplementation that I don't like: it's done in push_with_options()\nwhich can completely bypass setup_default_push_refspecs(), shooting\nthe user in the face.\n\nI want properly defined precedence and well-defined overall behavior.\nNot sneaky stuff. I don't have an answer, but will start a discussion\nwith some code soon. In the meantime, since we've already figured out\nthis series, merge it without 6/6.\n\nThanks.\n"},{"id":"221850","messageId":"7vbo6vbgbs.fsf@alter.siamese.dyndns.org","threadId":"34260","inReplyTo":"CALkWK0=a40hACGYApN7odrTytDmoRG_00_Q=EuaHuQ_=h8LBkg@mail.gmail.com","subject":"Re: [PATCH 6/6] push: honor branch.*.push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T16:53:43Z","receivedAt":"2013-06-24T16:53:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Junio C Hamano wrote:\n>> If the user said \"git push\" without an explicit request to push to\n>> \"ram\", and if branch.master.pushremote was not set to \"ram\", and\n>> still the command \"git push\" pushed the branch to \"ram\", then I\n>> would understand what you are worried about, but otherwise I do not\n>> see how what you are saying makes sense.\n>\n> We currently have no system to differentiate between those two cases.\n\nI am not sure what two cases you are talking about?\n\nIf you do not set anywhere (like branch.master.pushremote or\nremote.pushdefault) to push to \"ram\", and if you did not say \"git\npush ram\" but just said \"git push\", we will not push to \"ram\"\n(otherwise it is broken). So if the push is going to \"ram\", the\nuser must have asked us to push there, either via the command line,\nor these explicit configuration variables. And why do you need to\ndifferenciate between the command line \"ram\" and configured \"ram\"?\nAfter all, isn't the configuration merely a typesaver? If you know\noften push to \"ram\", you configure to push there. If you don't, you\ndon't.\n\n>> Or are you saying that with push.default set to upstream, only these\n>> two forms should be allowed?\n>>\n>> $ git push ;# no destination, no refspec\n>> $ git push there ref:spec ;# both explicitly specified\n>\n> No, no. What I meant is:\n>\n> From the documentation of push.default, I _expect_ upstream to kick\n> in only in the first case. In the second case, I _know_ that my\n> push.default is inconsequential.\n\nThe push.default is meant to be in effect only when there is no\nother stronger clue from the user for what to update (e.g. command\nline refspec, remote.*.push). Because branch.*.push weatherbaloon\npatch did not have any documentation update, it did not say it is a\nyet another way to explicitly configure the push destination branch.\nPerhaps that led to your expectation for upstream to kick in.\n\nOf course, that requires, as you earlier pointed out, that the logic\nto read from branch.*.push need to be moved out of the push.default\nlogic (in the weatherbaloon patch) and hosted to do_push() where it\nchecks if there is remote->pushrefspec[]. If branch.*.push wants to\ndefeat remote.*.push, then it should be checked before deciding to\nuse that configured remote.*.push.\n\n> I want properly defined precedence and well-defined overall behavior.\n\nOf course. There is no sneakiness.\n\nAs I already said, I personally do not know what branch.*.push buys\nus, and will happy to drop 6/6. It is merely a weatherbaloon patch.\n"},{"id":"221857","messageId":"CALkWK0nv7On34g-zE5uHAH69TT-KcEt8z-LxhveNp1b4b+NHsg@mail.gmail.com","threadId":"34260","inReplyTo":"7vbo6vbgbs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/6] push: honor branch.*.push","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T17:13:54Z","receivedAt":"2013-06-24T17:13:54Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> If you do not set anywhere (like branch.master.pushremote or\n> remote.pushdefault) to push to \"ram\", and if you did not say \"git\n> push ram\" but just said \"git push\", we will not push to \"ram\"\n> (otherwise it is broken). So if the push is going to \"ram\", the\n> user must have asked us to push there, either via the command line,\n> or these explicit configuration variables. And why do you need to\n> differenciate between the command line \"ram\" and configured \"ram\"?\n> After all, isn't the configuration merely a typesaver? If you know\n> often push to \"ram\", you configure to push there. If you don't, you\n> don't.\n\nYes, you're talking about configuration variables in general. If the user says\n\n $ git status\n\nexpecting long-form output, but gets short-form output, the world\nhasn't ended. But if the user does\n\n $ git push -f\n\nexpecting the push to go to one place, when it actually goes to\nanother place, she would have killed a few co-workers.\n\nI'm not saying that we need to differentiate between configuration\nvariables and CLI options; what I _am_ saying is that we need to think\ntwice about moving a CLI option to a configuration variable, precisely\nbecause we do not differentiate between the two cases.\n\nAs I have said earlier, my philosophy to prevent disasters does not\ninvolve erroring-out or denying for the most part; rather it focuses\non giving the user immediate feedback like:\n\n $ git push -f\n # pushing master:next to ram (press ^C to abort)\n\nAnother form of feedback is the refspec @{p[ush]}, which I'm\nstruggling to complete.\n\n> The push.default is meant to be in effect only when there is no\n> other stronger clue from the user for what to update (e.g. command\n> line refspec, remote.*.push).\n\nYes, I know what it does currently. With your patch, the list becomes\n\"push.default will be in effect only when there is no CLI refspec,\nremote..push or branch..push in effect\". I'm wondering if\nthere's a more elegant way to do it.\n\n> Of course, that requires, as you earlier pointed out, that the logic\n> to read from branch.*.push need to be moved out of the push.default\n> logic (in the weatherbaloon patch) and hosted to do_push() where it\n> checks if there is remote->pushrefspec[]. If branch.*.push wants to\n> defeat remote.*.push, then it should be checked before deciding to\n> use that configured remote.*.push.\n\nI suspect there's a better way to do this.\n\n> As I already said, I personally do not know what branch.*.push buys\n> us, and will happy to drop 6/6. It is merely a weatherbaloon patch.\n\nI hope we come out with a better version soon.\n"},{"id":"221859","messageId":"7vwqpja0g9.fsf@alter.siamese.dyndns.org","threadId":"34260","inReplyTo":"7v7ghkdl2p.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/6] t/t5528-push-default: generalize test_push_*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T17:21:58Z","receivedAt":"2013-06-24T17:21:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johan Herland writes:\n>\n>>> + git --git-dir=\"${3:-repo1}\" log -1 --format='%h %s' \"$2\" >actual &&\n>>\n>> Isn't ${3:-repo1} a bashism?\n>\n> I do not think so. But now I looked at it again, I think I would\n> use ${3-repo1} form in this case myself. No caller passes an empty\n> string to the third place.\n\nActually, because the caller blindly does this:\n\n # $3 = [optional] repo to check for actual output (repo1 by default)\n test_push_success () {\n git -c push.default=\"$1\" push &&\n check_pushed_commit HEAD \"$2\" \"$3\"\n }\n\nwhere it should be doing something like this:\n\n check_pushed_commit HEAD \"$2\" ${3+\"$3\"}\n\nif it wants $3 to be \"optional\", ${3:-repo1} is needed here to work\nit around.\n\nSo I'll leave it as-is for now.\n"},{"id":"221868","messageId":"7vfvw79xsq.fsf@alter.siamese.dyndns.org","threadId":"34260","inReplyTo":"CALkWK0nv7On34g-zE5uHAH69TT-KcEt8z-LxhveNp1b4b+NHsg@mail.gmail.com","subject":"Re: [PATCH 6/6] push: honor branch.*.push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T18:19:17Z","receivedAt":"2013-06-24T18:19:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> I'm not saying that we need to differentiate between configuration\n> variables and CLI options; what I _am_ saying is that we need to think\n> twice about moving a CLI option to a configuration variable, precisely\n> because we do not differentiate between the two cases.\n\nWith remote.pushdefault, I think the ship has long sailed.\n"},{"id":"221870","messageId":"CALkWK0=FmOn1dcCvVpN5nh6ysMTt=swSquU1yi_mzq9QhUnXrQ@mail.gmail.com","threadId":"34260","inReplyTo":"7vfvw79xsq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/6] push: honor branch.*.push","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T18:23:45Z","receivedAt":"2013-06-24T18:23:45Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> With remote.pushdefault, I think the ship has long sailed.\n\nWhat's wrong with the \"early feedback\" approach I suggested?\n"}]}