{"thread":{"id":"56106","subject":"[PATCH] push: make default consistent","startedAt":"2021-07-14T16:49:40Z","lastAt":"2021-07-19T03:48:06Z","messageCount":4,"participants":["Felipe Contreras"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"430076","messageId":"20210714164900.606330-1-felipe.contreras@gmail.com","threadId":"56106","inReplyTo":null,"subject":"[PATCH] push: make default consistent","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-14T16:49:00Z","receivedAt":"2021-07-14T16:49:40Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"The default action \"simple\" doesn't make sense. It's supposed to be the\nsame as \"current\", except with an extra safety in case the name of the\nupstream branch doesn't match the name of the current branch (and we are\npushing to the same remote). But if there's no upstream configured\nthere's no need for any check.\n\nIf this works:\n\n  git clone $central .\n  ...\n  git push\n\nThen this should too:\n\n  git clone $central .\n  git checkout -b fix-1\n  ...\n  git push\n\nCloning automatically sets up an upstream branch for \"master\", and\ntherefore it passes the safety check, but that is much more dangerous\nthan pushing any other branch.\n\nWhy do we barf with \"fix-1\", but not \"master\"?\n\nInstead of behaving like \"current\" if the current branch doesn't have an\nupstream configured, `git push` fails like \"upstream\", so it's a\nFrankensteinian action.\n\nIf the upstream branch isn't configured, \"simple\" should not abort, just\nlike \"current\".\n\nReported-by: Mathias Kunter <mathiaskunter@gmail.com>\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n\nNow that my push reorganization patches have landed on master, it's\nclear why the current behavior doesn't really match the behavior of\n\"current\".\n\nAs was discussed previously:\nhttps://lore.kernel.org/git/60b15cd2c4136_2183bc20893@natae.notmuch/\n\n Documentation/config/push.txt |  5 +++--\n Documentation/git-push.txt    |  6 +++---\n builtin/push.c                | 17 ++++++++++++-----\n t/t5528-push-default.sh       |  2 +-\n 4 files changed, 19 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/config/push.txt b/Documentation/config/push.txt\nindex 632033638c..d267d05e7a 100644\n--- a/Documentation/config/push.txt\n+++ b/Documentation/config/push.txt\n@@ -27,8 +27,9 @@ push.default::\n * `simple` - pushes the current branch with the same name on the remote.\n +\n If you are working on a centralized workflow (pushing to the same repository you\n-pull from, which is typically `origin`), then you need to configure an upstream\n-branch with the same name.\n+pull from, which is typically `origin`), and you have configured an upstream\n+branch, then the name must be the same as the current branch, otherwise this\n+action will fail as a precaution.\n +\n This mode is the default since Git 2.0, and is the safest option suited for\n beginners.\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex a953c7c387..58352bbf88 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -39,9 +39,9 @@ what to push (See linkgit:git-config[1] for the meaning of `push.default`).\n \n When neither the command-line nor the configuration specify what to\n push, the default behavior is used, which corresponds to the `simple`\n-value for `push.default`: the current branch is pushed to the\n-corresponding upstream branch, but as a safety measure, the push is\n-aborted if the upstream branch does not have the same name as the\n+value for `push.default`: the current branch is pushed to a remote\n+branch with the same name, but as a safety measure the push is aborted\n+if there's a configured upstream branch with a different name than the\n local one.\n \n \ndiff --git a/builtin/push.c b/builtin/push.c\nindex e8b10a9b7e..6062edb6f6 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -185,9 +185,11 @@ static const char message_detached_head_die[] =\n \t   \"\\n\"\n \t   \"    git push %s HEAD:<name-of-remote-branch>\\n\");\n \n-static const char *get_upstream_ref(struct branch *branch, const char *remote_name)\n+static const char *get_upstream_ref(struct branch *branch, const char *remote_name, int fatal)\n {\n-\tif (!branch->merge_nr || !branch->merge || !branch->remote_name)\n+\tif (!branch->merge_nr || !branch->merge || !branch->remote_name) {\n+\t\tif (!fatal)\n+\t\t\treturn NULL;\n \t\tdie(_(\"The current branch %s has no upstream branch.\\n\"\n \t\t    \"To push the current branch and set the remote as upstream, use\\n\"\n \t\t    \"\\n\"\n@@ -195,6 +197,7 @@ static const char *get_upstream_ref(struct branch *branch, const char *remote_na\n \t\t    branch->name,\n \t\t    remote_name,\n \t\t    branch->name);\n+\t}\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@@ -231,12 +234,16 @@ static void setup_default_push_refspecs(struct remote *remote)\n \tswitch (push_default) {\n \tdefault:\n \tcase PUSH_DEFAULT_UNSPECIFIED:\n-\tcase PUSH_DEFAULT_SIMPLE:\n+\tcase PUSH_DEFAULT_SIMPLE: {\n+\t\tconst char *upstream;\n+\n \t\tif (!same_remote)\n \t\t\tbreak;\n-\t\tif (strcmp(branch->refname, get_upstream_ref(branch, remote->name)))\n+\t\tupstream = get_upstream_ref(branch, remote->name, 0);\n+\t\tif (upstream && strcmp(branch->refname, upstream))\n \t\t\tdie_push_simple(branch, remote);\n \t\tbreak;\n+\t}\n \n \tcase PUSH_DEFAULT_UPSTREAM:\n \t\tif (!same_remote)\n@@ -244,7 +251,7 @@ static void setup_default_push_refspecs(struct remote *remote)\n \t\t\t      \"your current branch '%s', without telling me what to push\\n\"\n \t\t\t      \"to update which remote branch.\"),\n \t\t\t    remote->name, branch->name);\n-\t\tdst = get_upstream_ref(branch, remote->name);\n+\t\tdst = get_upstream_ref(branch, remote->name, 1);\n \t\tbreak;\n \n \tcase PUSH_DEFAULT_CURRENT:\ndiff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh\nindex f280e00eb7..ba38256ef4 100755\n--- a/t/t5528-push-default.sh\n+++ b/t/t5528-push-default.sh\n@@ -126,7 +126,7 @@ test_expect_success 'push from/to new branch with current creates remote branch'\n test_expect_success 'push to existing branch, with no upstream configured' '\n \ttest_config branch.main.remote repo1 &&\n \tgit checkout main &&\n-\ttest_push_failure simple &&\n+\ttest_push_success simple main &&\n \ttest_push_failure upstream\n '\n \n-- \n2.32.0.38.g1d70fa854e\n\n"},{"id":"430471","messageId":"20210719034758.1288766-1-felipe.contreras@gmail.com","threadId":"56106","inReplyTo":"20210714164900.606330-1-felipe.contreras@gmail.com","subject":"[PATCH v2 0/2] push: fix default action","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-19T03:47:56Z","receivedAt":"2021-07-19T03:48:02Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"The default action \"simple\" doesn't make sense. It's supposed to be the\nsame as \"current\", except with an extra safety in case the name of the\nupstream branch doesn't match the name of the current branch (and we are\npushing to the same remote). But if there's no upstream configured\nthere's no need for any check.\n\nIf this works:\n\n  git clone $central .\n  ...\n  git push\n\nThen this should too:\n\n  git clone $central .\n  git checkout -b topic\n  ...\n  git push\n\nCloning automatically sets up an upstream branch for \"master\", and\ntherefore it passes the safety check, but that is much more dangerous\nthan pushing any other branch.\n\nWhy do we barf with \"topic\", but not \"master\"?\n\nInstead of behaving like \"current\" if the current branch doesn't have an\nupstream configured, `git push` fails like \"upstream\", so it's a\nFrankensteinian action.\n\nIf the upstream branch isn't configured, \"simple\" should not abort, just\nlike \"current\".\n\nSince v1 I changed get_upstream_ref to always be non-fatal, and die on\nthe only caller instead.\n\nFelipe Contreras (2):\n  push: reorganize get_upstream_ref\n  push: make default consistent\n\n Documentation/config/push.txt |  5 +++--\n Documentation/git-push.txt    |  6 +++---\n builtin/push.c                | 25 +++++++++++++++----------\n t/t5528-push-default.sh       |  2 +-\n 4 files changed, 22 insertions(+), 16 deletions(-)\n\nRange-diff against v1:\n-:  ---------- > 1:  7031b26f28 push: reorganize get_upstream_ref\n1:  20bc9059f9 ! 2:  964b3c4289 push: make default consistent\n    @@ Commit message\n         pushing to the same remote). But if there's no upstream configured\n         there's no need for any check.\n     \n    -    If this works:\n    -\n    -      git clone $central .\n    -      ...\n    -      git push\n    -\n    -    Then this should too:\n    -\n    -      git clone $central .\n    -      git checkout -b fix-1\n    -      ...\n    -      git push\n    -\n    -    Cloning automatically sets up an upstream branch for \"master\", and\n    -    therefore it passes the safety check, but that is much more dangerous\n    -    than pushing any other branch.\n    -\n    -    Why do we barf with \"fix-1\", but not \"master\"?\n    -\n         Instead of behaving like \"current\" if the current branch doesn't have an\n         upstream configured, `git push` fails like \"upstream\", so it's a\n         Frankensteinian action.\n    @@ Documentation/git-push.txt: what to push (See linkgit:git-config[1] for the mean\n      \n     \n      ## builtin/push.c ##\n    -@@ builtin/push.c: static const char message_detached_head_die[] =\n    - \t   \"\\n\"\n    - \t   \"    git push %s HEAD:<name-of-remote-branch>\\n\");\n    - \n    --static const char *get_upstream_ref(struct branch *branch, const char *remote_name)\n    -+static const char *get_upstream_ref(struct branch *branch, const char *remote_name, int fatal)\n    - {\n    --\tif (!branch->merge_nr || !branch->merge || !branch->remote_name)\n    -+\tif (!branch->merge_nr || !branch->merge || !branch->remote_name) {\n    -+\t\tif (!fatal)\n    -+\t\t\treturn NULL;\n    - \t\tdie(_(\"The current branch %s has no upstream branch.\\n\"\n    - \t\t    \"To push the current branch and set the remote as upstream, use\\n\"\n    - \t\t    \"\\n\"\n    -@@ builtin/push.c: static const char *get_upstream_ref(struct branch *branch, const char *remote_na\n    - \t\t    branch->name,\n    - \t\t    remote_name,\n    - \t\t    branch->name);\n    -+\t}\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     @@ builtin/push.c: static void setup_default_push_refspecs(struct remote *remote)\n    - \tswitch (push_default) {\n    - \tdefault:\n    - \tcase PUSH_DEFAULT_UNSPECIFIED:\n    --\tcase PUSH_DEFAULT_SIMPLE:\n    -+\tcase PUSH_DEFAULT_SIMPLE: {\n    -+\t\tconst char *upstream;\n    -+\n      \t\tif (!same_remote)\n      \t\t\tbreak;\n    --\t\tif (strcmp(branch->refname, get_upstream_ref(branch, remote->name)))\n    -+\t\tupstream = get_upstream_ref(branch, remote->name, 0);\n    -+\t\tif (upstream && strcmp(branch->refname, upstream))\n    + \t\tdst = get_upstream_ref(branch);\n    +-\t\tif (!dst)\n    +-\t\t\tdie(_(\"The current branch %s has no upstream branch.\\n\"\n    +-\t\t\t    \"To push the current branch and set the remote as upstream, use\\n\"\n    +-\t\t\t    \"\\n\"\n    +-\t\t\t    \"    git push --set-upstream %s %s\\n\"),\n    +-\t\t\t    branch->name,\n    +-\t\t\t    remote->name,\n    +-\t\t\t    branch->name);\n    +-\t\tif (strcmp(branch->refname, dst))\n    ++\t\tif (dst && strcmp(branch->refname, dst))\n      \t\t\tdie_push_simple(branch, remote);\n    ++\t\tif (!dst)\n    ++\t\t\tdst = branch->refname;\n      \t\tbreak;\n    -+\t}\n      \n      \tcase PUSH_DEFAULT_UPSTREAM:\n    - \t\tif (!same_remote)\n    -@@ builtin/push.c: static void setup_default_push_refspecs(struct remote *remote)\n    - \t\t\t      \"your current branch '%s', without telling me what to push\\n\"\n    - \t\t\t      \"to update which remote branch.\"),\n    - \t\t\t    remote->name, branch->name);\n    --\t\tdst = get_upstream_ref(branch, remote->name);\n    -+\t\tdst = get_upstream_ref(branch, remote->name, 1);\n    - \t\tbreak;\n    - \n    - \tcase PUSH_DEFAULT_CURRENT:\n     \n      ## t/t5528-push-default.sh ##\n     @@ t/t5528-push-default.sh: test_expect_success 'push from/to new branch with current creates remote branch'\n-- \n2.32.0.40.gb9b36f9b52\n\n"},{"id":"430472","messageId":"20210719034758.1288766-2-felipe.contreras@gmail.com","threadId":"56106","inReplyTo":"20210719034758.1288766-1-felipe.contreras@gmail.com","subject":"[PATCH v2 1/2] push: reorganize get_upstream_ref","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-19T03:47:57Z","receivedAt":"2021-07-19T03:48:04Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Instead of dying inside the function if there's no upstream branch\nconfigured, return NULL and let the callers do that themselves.\n\nIn the next patch we won't always want to die.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n builtin/push.c | 31 +++++++++++++++++++++----------\n 1 file changed, 21 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex e8b10a9b7e..3ee7d6ada1 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -185,16 +185,10 @@ static const char message_detached_head_die[] =\n \t   \"\\n\"\n \t   \"    git push %s HEAD:<name-of-remote-branch>\\n\");\n \n-static const char *get_upstream_ref(struct branch *branch, const char *remote_name)\n+static const char *get_upstream_ref(struct branch *branch)\n {\n \tif (!branch->merge_nr || !branch->merge || !branch->remote_name)\n-\t\tdie(_(\"The current branch %s has no upstream branch.\\n\"\n-\t\t    \"To push the current branch and set the remote as upstream, use\\n\"\n-\t\t    \"\\n\"\n-\t\t    \"    git push --set-upstream %s %s\\n\"),\n-\t\t    branch->name,\n-\t\t    remote_name,\n-\t\t    branch->name);\n+\t\treturn NULL;\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@@ -234,7 +228,16 @@ static void setup_default_push_refspecs(struct remote *remote)\n \tcase PUSH_DEFAULT_SIMPLE:\n \t\tif (!same_remote)\n \t\t\tbreak;\n-\t\tif (strcmp(branch->refname, get_upstream_ref(branch, remote->name)))\n+\t\tdst = get_upstream_ref(branch);\n+\t\tif (!dst)\n+\t\t\tdie(_(\"The current branch %s has no upstream branch.\\n\"\n+\t\t\t    \"To push the current branch and set the remote as upstream, use\\n\"\n+\t\t\t    \"\\n\"\n+\t\t\t    \"    git push --set-upstream %s %s\\n\"),\n+\t\t\t    branch->name,\n+\t\t\t    remote->name,\n+\t\t\t    branch->name);\n+\t\tif (strcmp(branch->refname, dst))\n \t\t\tdie_push_simple(branch, remote);\n \t\tbreak;\n \n@@ -244,7 +247,15 @@ static void setup_default_push_refspecs(struct remote *remote)\n \t\t\t      \"your current branch '%s', without telling me what to push\\n\"\n \t\t\t      \"to update which remote branch.\"),\n \t\t\t    remote->name, branch->name);\n-\t\tdst = get_upstream_ref(branch, remote->name);\n+\t\tdst = get_upstream_ref(branch);\n+\t\tif (!dst)\n+\t\t\tdie(_(\"The current branch %s has no upstream branch.\\n\"\n+\t\t\t    \"To push the current branch and set the remote as upstream, use\\n\"\n+\t\t\t    \"\\n\"\n+\t\t\t    \"    git push --set-upstream %s %s\\n\"),\n+\t\t\t    branch->name,\n+\t\t\t    remote->name,\n+\t\t\t    branch->name);\n \t\tbreak;\n \n \tcase PUSH_DEFAULT_CURRENT:\n-- \n2.32.0.40.gb9b36f9b52\n\n"},{"id":"430473","messageId":"20210719034758.1288766-3-felipe.contreras@gmail.com","threadId":"56106","inReplyTo":"20210719034758.1288766-1-felipe.contreras@gmail.com","subject":"[PATCH v2 2/2] push: make default consistent","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-19T03:47:58Z","receivedAt":"2021-07-19T03:48:06Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"The default action \"simple\" doesn't make sense. It's supposed to be the\nsame as \"current\", except with an extra safety in case the name of the\nupstream branch doesn't match the name of the current branch (and we are\npushing to the same remote). But if there's no upstream configured\nthere's no need for any check.\n\nInstead of behaving like \"current\" if the current branch doesn't have an\nupstream configured, `git push` fails like \"upstream\", so it's a\nFrankensteinian action.\n\nIf the upstream branch isn't configured, \"simple\" should not abort, just\nlike \"current\".\n\nReported-by: Mathias Kunter <mathiaskunter@gmail.com>\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n Documentation/config/push.txt |  5 +++--\n Documentation/git-push.txt    |  6 +++---\n builtin/push.c                | 12 +++---------\n t/t5528-push-default.sh       |  2 +-\n 4 files changed, 10 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/config/push.txt b/Documentation/config/push.txt\nindex 632033638c..d267d05e7a 100644\n--- a/Documentation/config/push.txt\n+++ b/Documentation/config/push.txt\n@@ -27,8 +27,9 @@ push.default::\n * `simple` - pushes the current branch with the same name on the remote.\n +\n If you are working on a centralized workflow (pushing to the same repository you\n-pull from, which is typically `origin`), then you need to configure an upstream\n-branch with the same name.\n+pull from, which is typically `origin`), and you have configured an upstream\n+branch, then the name must be the same as the current branch, otherwise this\n+action will fail as a precaution.\n +\n This mode is the default since Git 2.0, and is the safest option suited for\n beginners.\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex a953c7c387..58352bbf88 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -39,9 +39,9 @@ what to push (See linkgit:git-config[1] for the meaning of `push.default`).\n \n When neither the command-line nor the configuration specify what to\n push, the default behavior is used, which corresponds to the `simple`\n-value for `push.default`: the current branch is pushed to the\n-corresponding upstream branch, but as a safety measure, the push is\n-aborted if the upstream branch does not have the same name as the\n+value for `push.default`: the current branch is pushed to a remote\n+branch with the same name, but as a safety measure the push is aborted\n+if there's a configured upstream branch with a different name than the\n local one.\n \n \ndiff --git a/builtin/push.c b/builtin/push.c\nindex 3ee7d6ada1..64134531de 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -229,16 +229,10 @@ static void setup_default_push_refspecs(struct remote *remote)\n \t\tif (!same_remote)\n \t\t\tbreak;\n \t\tdst = get_upstream_ref(branch);\n-\t\tif (!dst)\n-\t\t\tdie(_(\"The current branch %s has no upstream branch.\\n\"\n-\t\t\t    \"To push the current branch and set the remote as upstream, use\\n\"\n-\t\t\t    \"\\n\"\n-\t\t\t    \"    git push --set-upstream %s %s\\n\"),\n-\t\t\t    branch->name,\n-\t\t\t    remote->name,\n-\t\t\t    branch->name);\n-\t\tif (strcmp(branch->refname, dst))\n+\t\tif (dst && strcmp(branch->refname, dst))\n \t\t\tdie_push_simple(branch, remote);\n+\t\tif (!dst)\n+\t\t\tdst = branch->refname;\n \t\tbreak;\n \n \tcase PUSH_DEFAULT_UPSTREAM:\ndiff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh\nindex f280e00eb7..ba38256ef4 100755\n--- a/t/t5528-push-default.sh\n+++ b/t/t5528-push-default.sh\n@@ -126,7 +126,7 @@ test_expect_success 'push from/to new branch with current creates remote branch'\n test_expect_success 'push to existing branch, with no upstream configured' '\n \ttest_config branch.main.remote repo1 &&\n \tgit checkout main &&\n-\ttest_push_failure simple &&\n+\ttest_push_success simple main &&\n \ttest_push_failure upstream\n '\n \n-- \n2.32.0.40.gb9b36f9b52\n\n"}]}