{"thread":{"id":"18271","subject":"[PATCH v2] New config push.default to decide default behavior for push","startedAt":"2009-03-11T22:01:45Z","lastAt":"2009-03-16T21:13:05Z","messageCount":7,"participants":["Finn Arne Gangstad","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"107742","messageId":"20090311220144.GA6782@pvv.org","threadId":"18271","inReplyTo":null,"subject":"[PATCH v2] New config push.default to decide default behavior for push","fromName":"Finn Arne Gangstad","fromEmail":"finnag@pvv.org","sentAt":"2009-03-11T22:01:45Z","receivedAt":"2009-03-11T22:01:45Z","isPatch":true,"sender":{"key":"finnag@pvv.org","avatar":"https://gravatar.com/avatar/b421ddd58c3f0f93aa473e17b98bb8d53c221fef741746bc8cb59fae4ec6d95e?d=mp&s=160"},"body":"This is intended to replace fg/push-default, I'll repost some of the missing\nparts as other patches.\n\n--8<--\nNew config push.default to decide default behavior for push\n\nCurrently git push will push all matching branches to the current\nremote.  For some workflows this default is suboptimal, for new users\nit is often surprising, and some have found the default behaviour too\neasy to trig by accident with unwanted consequences.\n\nIntroduce a new configuration variable \"push.default\" that decides what\naction git push should take if no refspecs are given or implied by the\ncommand line arguments or the current remote configuration. If this\nvariable is unconfigured, display a prominent warning when default\nbehavior is trigged.\n\nPossible values are:\n'nothing'  : Push nothing\n'matching' : Current default behaviour, push all branches that already exist\n   in the current remote\n'tracking' : Push the current branch to whatever it is tracking\n'current'  : Push the current branch to a branch of the same name\n\nSigned-off-by: Finn Arne Gangstad <finnag@pvv.org>\n---\n Documentation/RelNotes-1.6.3.txt |    7 +++\n Documentation/config.txt         |   19 +++++++++\n builtin-push.c                   |   80 +++++++++++++++++++++++++++++++++++--\n cache.h                          |    9 ++++\n config.c                         |   25 ++++++++++++\n environment.c                    |    1 +\n 6 files changed, 136 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/RelNotes-1.6.3.txt b/Documentation/RelNotes-1.6.3.txt\nindex ee1fddb..3eb6bf0 100644\n--- a/Documentation/RelNotes-1.6.3.txt\n+++ b/Documentation/RelNotes-1.6.3.txt\n@@ -22,6 +22,13 @@ branch pointed at by its HEAD, gets a large warning.  You can choose what\n should happen upon such a push by setting the configuration variable\n receive.denyDeleteCurrent in the receiving repository.\n \n+In a future release, the default of \"git push\" without further\n+arguments may be changed. Currently, it will push all matching\n+refspecs to the current remote.  A configuration variable push.default\n+has been introduced to select the default behaviour.  To ease the\n+transition, a big warning is issued if this is not configured and a\n+git push without arguments is attempted.\n+\n \n Updates since v1.6.2\n --------------------\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex f5152c5..6fdf829 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1160,6 +1160,25 @@ pull.octopus::\n 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.\n+\n+\tThe term `current remote` means the remote configured for the current\n+\tbranch, or `origin` if no remote is configured. `origin` is also used\n+\tif you are not on any branch.\n+\n+\tPossible values are:\n++\n+* `nothing` do not push anything.\n+* `matching` push all matching branches to the current remote.\n+  All branches having the same name in both ends are considered to be\n+  matching. This is the current default value.\n+* `tracking` push the current branch to whatever it is tracking.\n+* `current` push the current branch to a branch of the same name.\n+\n receive.fsckObjects::\n \tIf it is set to true, git-receive-pack will check all received\n \tobjects. It will abort in the case of a malformed object or a\ndiff --git a/builtin-push.c b/builtin-push.c\nindex 122fdcf..fa5eabb 100644\n--- a/builtin-push.c\n+++ b/builtin-push.c\n@@ -48,6 +48,75 @@ static void set_refspecs(const char **refs, int nr)\n \t}\n }\n \n+static void setup_push_tracking(struct remote *remote)\n+{\n+\tint n;\n+\tstruct branch *branch = branch_get(NULL);\n+\tif (!branch)\n+\t\tdie(\"You are not currently on a branch.\");\n+\tif (!branch->merge_nr)\n+\t\tdie(\"The current branch %s is not tracking anything.\",\n+\t\t    branch->name);\n+\tif (branch->remote != remote)\n+\t\tdie(\"The current branch is tracking \\\"%s\\\", not \\\"%s\\\"!\",\n+\t\t    branch->remote->name, remote->name);\n+\tfor (n = 0; n < branch->merge_nr; n++) {\n+\t\tstruct strbuf rs = STRBUF_INIT;\n+\t\tstrbuf_addf(&rs, \"%s:%s\", branch->name, branch->merge[n]->src);\n+\t\tadd_refspec(rs.buf);\n+\t}\n+}\n+\n+static const char *warn_unconfigured_push_msg[] = {\n+\t\"You did not specify any refspecs to push, and the current remote\",\n+\t\"has not configured any push refspecs. The default action in this\",\n+\t\"case has been to push all matching refspecs, that is, all branches\",\n+\t\"that exist both locally and remotely will be updated.\",\n+\t\"This default may change in the future.\",\n+\t\"\",\n+\t\"You can specify what action you want to take in this case, and\",\n+\t\"avoid seeing this message again, by configuring 'push.default' to:\",\n+\t\"  'nothing'  : Do not push anythig\",\n+\t\"  'matching' : Push all matching branches (the current default)\",\n+\t\"  'tracking' : Push the current branch to whatever it is tracking\",\n+\t\"  'current'  : Push the current branch\",\n+\t\"\"\n+};\n+\n+static void warn_unconfigured_push()\n+{\n+\tint i;\n+\tfor (i = 0; i < ARRAY_SIZE(warn_unconfigured_push_msg); i++)\n+\t\twarning(\"%s\", warn_unconfigured_push_msg[i]);\n+}\n+\n+static void do_default_push(struct remote *remote)\n+{\n+\tgit_config(git_default_config, NULL);\n+\tswitch (push_default) {\n+\tcase PUSH_DEFAULT_UNSPECIFIED:\n+\t\twarn_unconfigured_push();\n+\t\t/* fallthrough */\n+\n+\tcase PUSH_DEFAULT_MATCHING:\n+\t\tadd_refspec(\":\");\n+\t\tbreak;\n+\n+\tcase PUSH_DEFAULT_TRACKING:\n+\t\tsetup_push_tracking(remote);\n+\t\tbreak;\n+\n+\tcase PUSH_DEFAULT_CURRENT:\n+\t\tadd_refspec(\"HEAD\");\n+\t\tbreak;\n+\n+\tcase PUSH_DEFAULT_NOTHING:\n+\t\tdie(\"Nothing to push, and push is configured to push nothing \"\n+\t\t    \"by default.\");\n+\t\tbreak;\n+\t}\n+}\n+\n static int do_push(const char *repo, int flags)\n {\n \tint i, errs;\n@@ -76,11 +145,12 @@ static int do_push(const char *repo, int flags)\n \t\treturn error(\"--all and --mirror are incompatible\");\n \t}\n \n-\tif (!refspec\n-\t\t&& !(flags & TRANSPORT_PUSH_ALL)\n-\t\t&& remote->push_refspec_nr) {\n-\t\trefspec = remote->push_refspec;\n-\t\trefspec_nr = remote->push_refspec_nr;\n+\tif (!refspec && !(flags & TRANSPORT_PUSH_ALL)) {\n+\t\tif (remote->push_refspec_nr) {\n+\t\t\trefspec = remote->push_refspec;\n+\t\t\trefspec_nr = remote->push_refspec_nr;\n+\t\t} else if (!(flags & TRANSPORT_PUSH_MIRROR))\n+\t\t\tdo_default_push(remote);\n \t}\n \terrs = 0;\n \tfor (i = 0; i < remote->url_nr; i++) {\ndiff --git a/cache.h b/cache.h\nindex 189151d..df4f117 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -541,8 +541,17 @@ enum rebase_setup_type {\n \tAUTOREBASE_ALWAYS,\n };\n \n+enum push_default_type {\n+\tPUSH_DEFAULT_UNSPECIFIED = -1,\n+\tPUSH_DEFAULT_NOTHING = 0,\n+\tPUSH_DEFAULT_MATCHING,\n+\tPUSH_DEFAULT_TRACKING,\n+\tPUSH_DEFAULT_CURRENT,\n+};\n+\n extern enum branch_track git_branch_track;\n extern enum rebase_setup_type autorebase;\n+extern enum push_default_type push_default;\n \n #define GIT_REPO_VERSION 0\n extern int repository_format_version;\ndiff --git a/config.c b/config.c\nindex 0c8c76f..e36a2b0 100644\n--- a/config.c\n+++ b/config.c\n@@ -565,6 +565,28 @@ static int git_default_branch_config(const char *var, const char *value)\n \treturn 0;\n }\n \n+static int git_default_push_config(const char *var, const char *value)\n+{\n+\tif (!strcmp(var, \"push.default\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\telse if (!strcmp(value, \"nothing\"))\n+\t\t\tpush_default = PUSH_DEFAULT_NOTHING;\n+\t\telse if (!strcmp(value, \"matching\"))\n+\t\t\tpush_default = PUSH_DEFAULT_MATCHING;\n+\t\telse if (!strcmp(value, \"tracking\"))\n+\t\t\tpush_default = PUSH_DEFAULT_TRACKING;\n+\t\telse if (!strcmp(value, \"current\"))\n+\t\t\tpush_default = PUSH_DEFAULT_CURRENT;\n+\t\telse\n+\t\t\treturn error(\"Malformed value for %s\", var);\n+\t\treturn 0;\n+\t}\n+\n+\t/* Add other config variables here and to Documentation/config.txt. */\n+\treturn 0;\n+}\n+\n static int git_default_mailmap_config(const char *var, const char *value)\n {\n \tif (!strcmp(var, \"mailmap.file\"))\n@@ -588,6 +610,9 @@ int git_default_config(const char *var, const char *value, void *dummy)\n \tif (!prefixcmp(var, \"branch.\"))\n \t\treturn git_default_branch_config(var, value);\n \n+\tif (!prefixcmp(var, \"push.\"))\n+\t\treturn git_default_push_config(var, value);\n+\n \tif (!prefixcmp(var, \"mailmap.\"))\n \t\treturn git_default_mailmap_config(var, value);\n \ndiff --git a/environment.c b/environment.c\nindex e278bce..4696885 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -42,6 +42,7 @@ enum safe_crlf safe_crlf = SAFE_CRLF_WARN;\n unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;\n enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;\n enum rebase_setup_type autorebase = AUTOREBASE_NEVER;\n+enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;\n \n /* Parallel index stat data preload? */\n int core_preload_index = 0;\n-- \n1.6.2.107.ge47ee.dirty\n"},{"id":"107751","messageId":"7vwsavlg6q.fsf@gitster.siamese.dyndns.org","threadId":"18271","inReplyTo":"20090311220144.GA6782@pvv.org","subject":"Re: [PATCH v2] New config push.default to decide default behavior for push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-12T00:48:13Z","receivedAt":"2009-03-12T00:48:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Finn Arne Gangstad <finnag@pvv.org> writes:\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index f5152c5..6fdf829 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1160,6 +1160,25 @@ pull.octopus::\n>  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.\n> +\n> +\tThe term `current remote` means the remote configured for the current\n> +\tbranch, or `origin` if no remote is configured. `origin` is also used\n> +\tif you are not on any branch.\n> +\n> +\tPossible values are:\n> ++\n> +* `nothing` do not push anything.\n> +* `matching` push all matching branches to the current remote.\n> +  All branches having the same name in both ends are considered to be\n> +  matching. This is the current default value.\n> +* `tracking` push the current branch to whatever it is tracking.\n> +* `current` push the current branch to a branch of the same name.\n> +\n\nI thought I fixed asciidoc formatting around this part in the version I\nqueued in 'pu'; in any case, the second and subsequent paragraphs need\ndedenting.\n\n> +static void warn_unconfigured_push()\n> +{\n\nI think I also fixed this \"old style declaration\".\n\n> +static void do_default_push(struct remote *remote)\n> +{\n> +\tgit_config(git_default_config, NULL);\n> +\tswitch (push_default) {\n> +\tcase PUSH_DEFAULT_UNSPECIFIED:\n> +\t\twarn_unconfigured_push();\n> +\t\t/* fallthrough */\n> +\n> +\tcase PUSH_DEFAULT_MATCHING:\n> +\t\tadd_refspec(\":\");\n> +\t\tbreak;\n> +\n> +\tcase PUSH_DEFAULT_TRACKING:\n> +\t\tsetup_push_tracking(remote);\n> +\t\tbreak;\n> +\n> +\tcase PUSH_DEFAULT_CURRENT:\n> +\t\tadd_refspec(\"HEAD\");\n> +\t\tbreak;\n> +\n> +\tcase PUSH_DEFAULT_NOTHING:\n> +\t\tdie(\"Nothing to push, and push is configured to push nothing \"\n> +\t\t    \"by default.\");\n> +\t\tbreak;\n> +\t}\n> +}\n\nThis part looks good, provided if we were to go ahead and plan to change\nthe default in the future.\n\n> +static void setup_push_tracking(struct remote *remote)\n> +{\n> +\tint n;\n> +\tstruct branch *branch = branch_get(NULL);\n> +\tif (!branch)\n> +\t\tdie(\"You are not currently on a branch.\");\n> +\tif (!branch->merge_nr)\n> +\t\tdie(\"The current branch %s is not tracking anything.\",\n> +\t\t    branch->name);\n> +\tif (branch->remote != remote)\n> +\t\tdie(\"The current branch is tracking \\\"%s\\\", not \\\"%s\\\"!\",\n> +\t\t    branch->remote->name, remote->name);\n> +\tfor (n = 0; n < branch->merge_nr; n++) {\n> +\t\tstruct strbuf rs = STRBUF_INIT;\n> +\t\tstrbuf_addf(&rs, \"%s:%s\", branch->name, branch->merge[n]->src);\n> +\t\tadd_refspec(rs.buf);\n> +\t}\n> +}\n\nIf a branch is configured to merge from multiple places (e.g. testing\nbranch similar to the linux-next tree to integrate from multiple staging\ntrees), a sane default would be not to push it out to any of the branches\nit pulls from---it is a consumer to the other branches, and it is meant to\nbe sent to somewhere else, not back to any of the originators.  Instead,\nthis code will push to all of them, which I would not see any sane use\ncase for.  It might make a bit sense if you refused unless merge_nr is\nexactly one.\n\nAlso I am not sure if the check between the name of the remote makes much\npractical sense.  Many people use two remotes to name the same one for\npushing over ssh and fetching over git.  Which name comes in which?  I\nthink with this logic you are trying to catch a mistake like:\n\n    $ git push --tracking $there\n\nwhen the current branch tracks a remote branch that did not come from the\nremote repository $there but somewhere else, but if that is the motivation\nbehind it, does it help to forbid \"push --tracking\" to take any\ndestination repository to reduce such a confusion?\n"},{"id":"107809","messageId":"20090312115433.GA2848@pvv.org","threadId":"18271","inReplyTo":"7vwsavlg6q.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] New config push.default to decide default behavior for push","fromName":"Finn Arne Gangstad","fromEmail":"finnag@pvv.org","sentAt":"2009-03-12T11:54:33Z","receivedAt":"2009-03-12T11:54:33Z","isPatch":true,"sender":{"key":"finnag@pvv.org","avatar":"https://gravatar.com/avatar/b421ddd58c3f0f93aa473e17b98bb8d53c221fef741746bc8cb59fae4ec6d95e?d=mp&s=160"},"body":"On Wed, Mar 11, 2009 at 05:48:13PM -0700, Junio C Hamano wrote:\n\n> I thought I fixed asciidoc formatting around this part in the version I\n> queued in 'pu'; in any case, the second and subsequent paragraphs need\n> dedenting.\n\nI am sorry, it wasn't immediately obvious to me that you had changed\nthe version i sent. Fixed up this (and other fixes) in my working\nbranch I think.\n\n> > +static void setup_push_tracking(struct remote *remote)\n> > +{\n> > +\tint n;\n> > +\tstruct branch *branch = branch_get(NULL);\n> > +\tif (!branch)\n> > +\t\tdie(\"You are not currently on a branch.\");\n> > +\tif (!branch->merge_nr)\n> > +\t\tdie(\"The current branch %s is not tracking anything.\",\n> > +\t\t    branch->name);\n> > +\tif (branch->remote != remote)\n> > +\t\tdie(\"The current branch is tracking \\\"%s\\\", not \\\"%s\\\"!\",\n> > +\t\t    branch->remote->name, remote->name);\n> > +\tfor (n = 0; n < branch->merge_nr; n++) {\n> > +\t\tstruct strbuf rs = STRBUF_INIT;\n> > +\t\tstrbuf_addf(&rs, \"%s:%s\", branch->name, branch->merge[n]->src);\n> > +\t\tadd_refspec(rs.buf);\n> > +\t}\n> > +}\n> \n> If a branch is configured to merge from multiple places (e.g. testing\n> branch similar to the linux-next tree to integrate from multiple staging\n> trees), a sane default would be not to push it out to any of the branches\n> it pulls from---it is a consumer to the other branches, and it is meant to\n> be sent to somewhere else, not back to any of the originators.  Instead,\n> this code will push to all of them, which I would not see any sane use\n> case for.  It might make a bit sense if you refused unless merge_nr is\n> exactly one.\n\nYes I agree, fixed this up.\n\n> Also I am not sure if the check between the name of the remote makes much\n> practical sense.  Many people use two remotes to name the same one for\n> pushing over ssh and fetching over git.  Which name comes in which?  I\n> think with this logic you are trying to catch a mistake like:\n> \n>     $ git push --tracking $there\n\nYes that was the idea. I was not familiar with the \"multiple remotes\nto the same thing\" common usage, but have no problems supporting that\ninstead.\n\nSomething like this amended into the last commit? I can amend it on top\nof the last one and resend if that is better.\n\n--8<--\ngit push tracking mode fixes\n\nIf a branch is tracking multiple branches, refuse to push it.\nSome asciidoc format fixes.\n\nSigned-off-by: Finn Arne Gangstad <finnag@pvv.org>\n---\nTo be amended into the previous commit\n\n Documentation/config.txt |   16 ++++++++--------\n builtin-push.c           |   25 +++++++++++--------------\n 2 files changed, 19 insertions(+), 22 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 6fdf829..986becc 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1165,19 +1165,19 @@ push.default::\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.\n-\n-\tThe term `current remote` means the remote configured for the current\n-\tbranch, or `origin` if no remote is configured. `origin` is also used\n-\tif you are not on any branch.\n-\n-\tPossible values are:\n++\n+The term `current remote` means the remote configured for the current\n+branch, or `origin` if no remote is configured. `origin` is also used\n+if you are not on any branch. Possible values are:\n +\n * `nothing` do not push anything.\n * `matching` push all matching branches to the current remote.\n   All branches having the same name in both ends are considered to be\n   matching. This is the current default value.\n-* `tracking` push the current branch to whatever it is tracking.\n-* `current` push the current branch to a branch of the same name.\n+* `tracking` push the current branch to the branch it is tracking.\n+* `current` push the current branch to a branch of the same name on the\n+  current remote.\n+\n \n receive.fsckObjects::\n \tIf it is set to true, git-receive-pack will check all received\ndiff --git a/builtin-push.c b/builtin-push.c\nindex fa5eabb..51f4c4a 100644\n--- a/builtin-push.c\n+++ b/builtin-push.c\n@@ -48,23 +48,20 @@ static void set_refspecs(const char **refs, int nr)\n \t}\n }\n \n-static void setup_push_tracking(struct remote *remote)\n+static void setup_push_tracking(void)\n {\n-\tint n;\n+\tstruct strbuf refspec = STRBUF_INIT;\n \tstruct branch *branch = branch_get(NULL);\n \tif (!branch)\n \t\tdie(\"You are not currently on a branch.\");\n \tif (!branch->merge_nr)\n \t\tdie(\"The current branch %s is not tracking anything.\",\n \t\t    branch->name);\n-\tif (branch->remote != remote)\n-\t\tdie(\"The current branch is tracking \\\"%s\\\", not \\\"%s\\\"!\",\n-\t\t    branch->remote->name, remote->name);\n-\tfor (n = 0; n < branch->merge_nr; n++) {\n-\t\tstruct strbuf rs = STRBUF_INIT;\n-\t\tstrbuf_addf(&rs, \"%s:%s\", branch->name, branch->merge[n]->src);\n-\t\tadd_refspec(rs.buf);\n-\t}\n+\tif (branch->merge_nr != 1)\n+\t\tdie(\"The current branch %s is tracking multiple branches, \"\n+\t\t    \"refusing to push.\", branch->name);\n+\tstrbuf_addf(&refspec, \"%s:%s\", branch->name, branch->merge[0]->src);\n+\tadd_refspec(refspec.buf);\n }\n \n static const char *warn_unconfigured_push_msg[] = {\n@@ -83,14 +80,14 @@ static const char *warn_unconfigured_push_msg[] = {\n \t\"\"\n };\n \n-static void warn_unconfigured_push()\n+static void warn_unconfigured_push(void)\n {\n \tint i;\n \tfor (i = 0; i < ARRAY_SIZE(warn_unconfigured_push_msg); i++)\n \t\twarning(\"%s\", warn_unconfigured_push_msg[i]);\n }\n \n-static void do_default_push(struct remote *remote)\n+static void do_default_push(void)\n {\n \tgit_config(git_default_config, NULL);\n \tswitch (push_default) {\n@@ -103,7 +100,7 @@ static void do_default_push(struct remote *remote)\n \t\tbreak;\n \n \tcase PUSH_DEFAULT_TRACKING:\n-\t\tsetup_push_tracking(remote);\n+\t\tsetup_push_tracking();\n \t\tbreak;\n \n \tcase PUSH_DEFAULT_CURRENT:\n@@ -150,7 +147,7 @@ static int do_push(const char *repo, int flags)\n \t\t\trefspec = remote->push_refspec;\n \t\t\trefspec_nr = remote->push_refspec_nr;\n \t\t} else if (!(flags & TRANSPORT_PUSH_MIRROR))\n-\t\t\tdo_default_push(remote);\n+\t\t\tdo_default_push();\n \t}\n \terrs = 0;\n \tfor (i = 0; i < remote->url_nr; i++) {\n-- \n1.6.2.81.gc6c21.dirty\n"},{"id":"108024","messageId":"7viqmbakmt.fsf@gitster.siamese.dyndns.org","threadId":"18271","inReplyTo":"20090312115433.GA2848@pvv.org","subject":"Re: [PATCH v2] New config push.default to decide default behavior for push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-14T20:56:42Z","receivedAt":"2009-03-14T20:56:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Finn Arne Gangstad <finnag@pvv.org> writes:\n\n> Something like this amended into the last commit? I can amend it on top\n> of the last one and resend if that is better.\n\nThanks.\n\nI looked at these two patches after squashing them into one, and I think\nit makes sense as the final shape of a two patch series.\n\nAlthough the purist in me tells me that addition of the \"tracking\" and the\n\"current\" should be in a separate commit as they are purely new features,\nI think it is Ok.  In that sense, \"nothing\" is a new feature anyway, and\n\"current\" is something we already have, so the true addition here is only\nthe \"tracking\" one.\n\nI also reworded the commit log message a bit, like this:\n\n    Author: Finn Arne Gangstad <finnag@pvv.org>\n    Date:   Wed Mar 11 23:01:45 2009 +0100\n\n    New config push.default to decide default behavior for push\n    \n    When \"git push\" is not told what refspecs to push, it pushes all matching\n    branches to the current remote.  For some workflows this default is not\n    useful, and surprises new users.  Some have even found that this default\n    behaviour too easy to trigger by accident with unwanted consequences.\n    \n    Introduce a new configuration variable \"push.default\" that decides what\n    action git push should take if no refspecs are given or implied by the\n    command line arguments or the current remote configuration. If this\n    variable is unconfigured, display a prominent warning when default\n    behavior is triggered.\n    \n    Possible values are:\n    \n      'nothing'  : Push nothing;\n      'matching' : Current default behaviour, push all branches that already\n                   exist in the current remote;\n      'tracking' : Push the current branch to whatever it is tracking;\n      'current'  : Push the current branch to a branch of the same name,\n      \t           i.e. HEAD.\n\n    Signed-off-by: Finn Arne Gangstad <finnag@pvv.org>\n    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nI however had to scratch my head for 20 seconds wondering where the push\nhappens when do_default_push() codepath is taken, and it turns out that\nthe function is there merely to set up the push refspecs for the default\ncase; the function is misnamed.  I'd further squash the following patch\nin.\n\ndiff --git a/builtin-push.c b/builtin-push.c\nindex 51f4c4a..e4988da 100644\n--- a/builtin-push.c\n+++ b/builtin-push.c\n@@ -76,8 +76,7 @@ static const char *warn_unconfigured_push_msg[] = {\n \t\"  'nothing'  : Do not push anythig\",\n \t\"  'matching' : Push all matching branches (the current default)\",\n \t\"  'tracking' : Push the current branch to whatever it is tracking\",\n-\t\"  'current'  : Push the current branch\",\n-\t\"\"\n+\t\"  'current'  : Push the current branch\"\n };\n \n static void warn_unconfigured_push(void)\n@@ -87,7 +86,7 @@ static void warn_unconfigured_push(void)\n \t\twarning(\"%s\", warn_unconfigured_push_msg[i]);\n }\n \n-static void do_default_push(void)\n+static void setup_default_push_refspecs(void)\n {\n \tgit_config(git_default_config, NULL);\n \tswitch (push_default) {\n@@ -147,7 +146,7 @@ static int do_push(const char *repo, int flags)\n \t\t\trefspec = remote->push_refspec;\n \t\t\trefspec_nr = remote->push_refspec_nr;\n \t\t} else if (!(flags & TRANSPORT_PUSH_MIRROR))\n-\t\t\tdo_default_push();\n+\t\t\tsetup_default_push_refspecs();\n \t}\n \terrs = 0;\n \tfor (i = 0; i < remote->url_nr; i++) {\n"},{"id":"108068","messageId":"7viqmayslg.fsf@gitster.siamese.dyndns.org","threadId":"18271","inReplyTo":"7viqmbakmt.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] New config push.default to decide default behavior for push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-16T04:55:23Z","receivedAt":"2009-03-16T04:55:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Finn Arne Gangstad <finnag@pvv.org> writes:\n>\n>> Something like this amended into the last commit? I can amend it on top\n>> of the last one and resend if that is better.\n>\n> Thanks.\n>\n> I looked at these two patches after squashing them into one, and I think\n> it makes sense as the final shape of a two patch series.\n\nI seem to have forgotten to say what the two pieces in the series should\nlook like here.  What I meant was a split along these lines:\n\n (1) introduce the configuration variable, so that people who do not like\n     the current default can get a different default, but do not warn;\n\n (2) start warning to notify the users of possible different settings and\n     forcing them to choose now, before we switch the default.\n\nAs some people still seem to object to the change of default (and that\nincludes 20-30% of myself), we may end up deciding not to switch the\ndefault after all, but even in that case, applying the first half would\nbenefit people who would want different behaviour.\n\nIf we were to decide not to switch the default, applying (2) would force\npeople who want the current default to set the configuration only to\nsquelch the message, which is an annoying inconvenience.\n\nBut since then I thought about this a bit more, and I am inclined to\nchange my mind.  Having (2) will allow people who wish there were a way to\nhave a different default to discover that there is already an easy way to\ndo so.  The tradeoff between \"a slight inconvenience to traditionalist\" vs\n\"helping people to discover the feature designed for them, without which\nthey would be badly burned\" does not look too bad to me.\n"},{"id":"108095","messageId":"20090316155629.GA17526@pvv.org","threadId":"18271","inReplyTo":"7viqmayslg.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] New config push.default to decide default behavior for push","fromName":"Finn Arne Gangstad","fromEmail":"finnag@pvv.org","sentAt":"2009-03-16T15:56:29Z","receivedAt":"2009-03-16T15:56:29Z","isPatch":true,"sender":{"key":"finnag@pvv.org","avatar":"https://gravatar.com/avatar/b421ddd58c3f0f93aa473e17b98bb8d53c221fef741746bc8cb59fae4ec6d95e?d=mp&s=160"},"body":"On Sun, Mar 15, 2009 at 09:55:23PM -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> As some people still seem to object to the change of default (and that\n> includes 20-30% of myself), we may end up deciding not to switch the\n> default after all, but even in that case, applying the first half would\n> benefit people who would want different behaviour.\n\nI think the suggested new default is a lot safer then the current\none. A default of \"nothing\" will print a nice message if you end up\npushing nothing, which you will fix in a heartbeat with a single git\nconfig command.\n\nIf you erroneously push one or more branches however, cleanup might\nend up being very complicated. Many pushable repos are set up with\ndisallowing non-fast-forward pushes, so it may require intervention by\nsomeone else to clean up, and by then someone else have already\nfetched the bad push.\n\n- Finn Arne\n"},{"id":"108140","messageId":"7vljr5uq72.fsf@gitster.siamese.dyndns.org","threadId":"18271","inReplyTo":"20090316155629.GA17526@pvv.org","subject":"Re: [PATCH v2] New config push.default to decide default behavior for push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-16T21:13:05Z","receivedAt":"2009-03-16T21:13:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Finn Arne Gangstad <finnag@pvv.org> writes:\n\n> On Sun, Mar 15, 2009 at 09:55:23PM -0700, Junio C Hamano wrote:\n>> Junio C Hamano <gitster@pobox.com> writes:\n>> \n>> As some people still seem to object to the change of default (and that\n>> includes 20-30% of myself), we may end up deciding not to switch the\n>> default after all, but even in that case, applying the first half would\n>> benefit people who would want different behaviour.\n>\n> I think the suggested new default is a lot safer then the current\n> one. A default of \"nothing\" will print a nice message if you end up\n> pushing nothing, which you will fix in a heartbeat with a single git\n> config command.\n>\n> If you erroneously push one or more branches however, cleanup might\n> end up being very complicated. Many pushable repos are set up with\n> disallowing non-fast-forward pushes, so it may require intervention by\n> someone else to clean up, and by then someone else have already\n> fetched the bad push.\n\nI think traditionalists who do not like changing the default already know\nthese, though.  I would not object to the push.default as a _choice_.\n\nIn fact, sourceforge.jp (they added git support late last year, and I keep\na secondary public repository just like my alt-git.git at repo.or.cz) is\none of such places.  It seems to forbid non fast-forward pushes, and that\nis why I have been pushing only maint and master there.  It does allow\ndeletion, and I could push deletion followed by creation in two stages,\ni.e. \"git push sfjp :pu && git push sfjp pu\", but I did not bother.\n\nIn a later part of the message you are responding to (but did not quote),\nI was agreeing with all of what you wrote here, and even more ;-) Notice\nthe \"tradeoff does not look too bad to me\" part.\n\nYour new [1/2] gives the choice without advertisement, and _if_ you remove\nor tone down \"The default may change in the future\" from [2/2], it becomes\npurely an advertisement of the feature to help people from burning\nthemselves.  I do not see anything a sane traditionist would object to at\nthat point, and I thought we could even squash the two into one commit\n(which was what I meant by \"I am inclined to change my mind\" in the\nmessage you are responding to).\n"}]}