[PATCH v3] push: Enhance unspecified push default warning

[PATCH v3] push: Enhance unspecified push default warning

[Greg Jacobson]

When the unset push.default warning message is displayed
this may be the first time many users encounter push.default.
Modified the warning message to explain in a compact
manner what push.default is and why it is being changed in
Git 2.0.  Also provided additional information to help users
decide if this change will affect their workflow.

Signed-off-by: Greg Jacobson <coder5000@gmail.com>
---
 builtin/push.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/builtin/push.c b/builtin/push.c
index 7b1b66c..5393e28 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -174,6 +174,15 @@ N_("push.default is unset; its implicit value is
changing in\n"
    "\n"
    "  git config --global push.default simple\n"
    "\n"
+   "When push.default is set to 'matching', git will push all local branches\n"
+   "to the remote branches with the same (matching) name.  This will no\n"
+   "longer be the default in Git 2.0 because a branch could be\n"
+   "unintentionally pushed to a remote.\n"
+   "\n"
+   "In Git 2.0 the new push.default of 'simple' will push only the current\n"
+   "branch to the same remote branch used by git pull.   A push will\n"
+   "only succeed if the remote and local branches have the same name.\n"
+   "\n"
    "See 'git help config' and search for 'push.default' for further
information.\n"
    "(the 'simple' mode was introduced in Git 1.7.11. Use the similar mode\n"
    "'current' instead of 'simple' if you sometimes use older versions
of Git)");
-- 
1.8.4.474.g128a96c.dirty

Re: [PATCH v3] push: Enhance unspecified push default warning

[Greg Jacobson]

Is there anything I could do to improve this patch?  Thank you.

On Fri, Oct 4, 2013 at 10:20 AM, Greg Jacobson <coder5000@gmail.com> wrote:
> When the unset push.default warning message is displayed
> this may be the first time many users encounter push.default.
> Modified the warning message to explain in a compact
> manner what push.default is and why it is being changed in
> Git 2.0.  Also provided additional information to help users
> decide if this change will affect their workflow.
>
> Signed-off-by: Greg Jacobson <coder5000@gmail.com>
> ---
>  builtin/push.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index 7b1b66c..5393e28 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -174,6 +174,15 @@ N_("push.default is unset; its implicit value is
> changing in\n"
>     "\n"
>     "  git config --global push.default simple\n"
>     "\n"
> +   "When push.default is set to 'matching', git will push all local branches\n"
> +   "to the remote branches with the same (matching) name.  This will no\n"
> +   "longer be the default in Git 2.0 because a branch could be\n"
> +   "unintentionally pushed to a remote.\n"
> +   "\n"
> +   "In Git 2.0 the new push.default of 'simple' will push only the current\n"
> +   "branch to the same remote branch used by git pull.   A push will\n"
> +   "only succeed if the remote and local branches have the same name.\n"
> +   "\n"
>     "See 'git help config' and search for 'push.default' for further
> information.\n"
>     "(the 'simple' mode was introduced in Git 1.7.11. Use the similar mode\n"
>     "'current' instead of 'simple' if you sometimes use older versions
> of Git)");
> --
> 1.8.4.474.g128a96c.dirty

Re: [PATCH v3] push: Enhance unspecified push default warning

[Junio C Hamano]

Greg Jacobson <coder5000@gmail.com> writes:

> Is there anything I could do to improve this patch?  Thank you.

My vague recollection is that we started from an excerpt from the
documentation page, not unlike this patch attempts to, but because
such an excerpt has to be less complete than the documentation for
brevity's sake, it is bound to be an incorrect and/or misleading
one, and decided that we are better off referring the users, who do
want to choose something other than the default we chose, to the
documentation.

Somebody cares to dig up the old discussion threads and post a few
pointers?

> On Fri, Oct 4, 2013 at 10:20 AM, Greg Jacobson <coder5000@gmail.com> wrote:
>> When the unset push.default warning message is displayed
>> this may be the first time many users encounter push.default.
>> Modified the warning message to explain in a compact
>> manner what push.default is and why it is being changed in
>> Git 2.0.  Also provided additional information to help users
>> decide if this change will affect their workflow.
>>
>> Signed-off-by: Greg Jacobson <coder5000@gmail.com>
>> ---
>>  builtin/push.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/builtin/push.c b/builtin/push.c
>> index 7b1b66c..5393e28 100644
>> --- a/builtin/push.c
>> +++ b/builtin/push.c
>> @@ -174,6 +174,15 @@ N_("push.default is unset; its implicit value is
>> changing in\n"
>>     "\n"
>>     "  git config --global push.default simple\n"
>>     "\n"
>> +   "When push.default is set to 'matching', git will push all local branches\n"
>> +   "to the remote branches with the same (matching) name.  This will no\n"
>> +   "longer be the default in Git 2.0 because a branch could be\n"
>> +   "unintentionally pushed to a remote.\n"
>> +   "\n"
>> +   "In Git 2.0 the new push.default of 'simple' will push only the current\n"
>> +   "branch to the same remote branch used by git pull.   A push will\n"
>> +   "only succeed if the remote and local branches have the same name.\n"
>> +   "\n"
>>     "See 'git help config' and search for 'push.default' for further
>> information.\n"
>>     "(the 'simple' mode was introduced in Git 1.7.11. Use the similar mode\n"
>>     "'current' instead of 'simple' if you sometimes use older versions
>> of Git)");
>> --
>> 1.8.4.474.g128a96c.dirty

Re: [PATCH v3] push: Enhance unspecified push default warning

[Matthieu Moy]

----- Original Message -----
> Greg Jacobson <coder5000@gmail.com> writes:
> 
> > Is there anything I could do to improve this patch?  Thank you.
> 
> My vague recollection is that we started from an excerpt from the
> documentation page, not unlike this patch attempts to, but because
> such an excerpt has to be less complete than the documentation for
> brevity's sake, it is bound to be an incorrect and/or misleading
> one, and decided that we are better off referring the users, who do
> want to choose something other than the default we chose, to the
> documentation.
> 
> Somebody cares to dig up the old discussion threads and post a few
> pointers?

The previous versions of this patch received only minor comments,
which were taken into account:

http://thread.gmane.org/gmane.comp.version-control.git/235675
http://thread.gmane.org/gmane.comp.version-control.git/235694

I don't remember all the discussions on the patch which introduced
the warning, but I don't think it's relevant to digg them before applying the patch:

* The assumption was that users would read the docs, but as I already mentioned:
  "Judging by the question asked on stackoverflow
  ( http://stackoverflow.com/questions/13148066/warning-push-default-is-unset-its-implicit-value-is-changing-in-git-2-0 )
  and its popularity, telling the users to read the docs did not work very
  well."

* The warning has been there for a while now. Advanced users have already set push.default.
  We shouldn't be worried about eating a bit of screen real estate for users who didn't yet.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v3] push: Enhance unspecified push default warning

[Matthieu Moy]

----- Original Message -----
> Greg Jacobson <coder5000@gmail.com> writes:
> 
> > Is there anything I could do to improve this patch?  Thank you.
> 
> My vague recollection is that we started from an excerpt from the
> documentation page, not unlike this patch attempts to, but because
> such an excerpt has to be less complete than the documentation for
> brevity's sake, it is bound to be an incorrect and/or misleading
> one, and decided that we are better off referring the users, who do
> want to choose something other than the default we chose, to the
> documentation.
> 
> Somebody cares to dig up the old discussion threads and post a few
> pointers?

The previous versions of this patch received only minor comments,
which were taken into account:

http://thread.gmane.org/gmane.comp.version-control.git/235675
http://thread.gmane.org/gmane.comp.version-control.git/235694

I don't remember all the discussions on the patch which introduced
the warning, but I don't think it's relevant to digg them before applying the patch:

* The assumption was that users would read the docs, but as I already mentioned:
  "Judging by the question asked on stackoverflow
  ( http://stackoverflow.com/questions/13148066/warning-push-default-is-unset-its-implicit-value-is-changing-in-git-2-0 )
  and its popularity, telling the users to read the docs did not work very
  well."

* The warning has been there for a while now. Advanced users have already set push.default.
  We shouldn't be worried about eating a bit of screen real estate for users who didn't yet.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v3] push: Enhance unspecified push default warning

[Junio C Hamano]

Matthieu Moy <matthieu.moy@grenoble-inp.fr> writes:

> I don't remember all the discussions on the patch which introduced
> the warning, but I don't think it's relevant to digg them before applying the patch:

If we apply the patch then it is too late to dig them ;-)

> * The assumption was that users would read the docs, but as I already mentioned:
>   "Judging by the question asked on stackoverflow
>   ( http://stackoverflow.com/questions/13148066/warning-push-default-is-unset-its-implicit-value-is-changing-in-git-2-0 )
>   and its popularity, telling the users to read the docs did not work very
>   well."

That is true, but does it justify giving a misleading information in
the advice message?

Specifically:

>> +   "When push.default is set to 'matching', git will push all local branches\n"
>> +   "to the remote branches with the same (matching) name.

invites those who do not read documentation to mistake it with using
an explicit "refs/heads/*:refs/heads/*" refspec.

And this one

>> +   "In Git 2.0 the new push.default of 'simple' will push only the current\n"
>> +   "branch to the same remote branch used by git pull.   A push will\n"
>> +   "only succeed if the remote and local branches have the same name.\n"

while you can see that it is not telling a lie if you read it twice,
"will only succeed if" feels somewhat roundabout.

	... push only the current branch back to the branch of the
	same name, but only if 'git pull' is set to pull from that
	branch. Otherwise the push will fail.

might be an improvement, but I dunno.

> * The warning has been there for a while now. Advanced users have
> already set push.default.  We shouldn't be worried about eating a
> bit of screen real estate for users who didn't yet.

This part I can agree with.

Re: [PATCH v3] push: Enhance unspecified push default warning

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <matthieu.moy@grenoble-inp.fr> writes:
>
>> I don't remember all the discussions on the patch which introduced
>> the warning, but I don't think it's relevant to digg them before applying the patch:
>
> If we apply the patch then it is too late to dig them ;-)
>
>> * The assumption was that users would read the docs, but as I already mentioned:
>>   "Judging by the question asked on stackoverflow
>>   ( http://stackoverflow.com/questions/13148066/warning-push-default-is-unset-its-implicit-value-is-changing-in-git-2-0 )
>>   and its popularity, telling the users to read the docs did not work very
>>   well."
>
> That is true, but does it justify giving a misleading information in
> the advice message?

Also applying this will have an unpleasant fallout to merging the
endgame patch b2ed944a (push: switch default from "matching" to
"simple", 2013-01-04).  The added text needs to be corrected with an
evil merge.

I'd prefer to having worry about such a fallout only once.  Which
arguably we already did when we came up with the current message, so
I am fairly annoyed by this patch coming loooooong after we
concluded the original discussion.

Sigh X-<.  I'll worry about this later, as b2ed944a is in 'next'
during the feature freeze, and I'd prefer not having to rebase it on
top of the final version of this patch.

Re: [PATCH v3] push: Enhance unspecified push default warning

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Also applying this will have an unpleasant fallout to merging the
> endgame patch b2ed944a (push: switch default from "matching" to
> "simple", 2013-01-04).  The added text needs to be corrected with an
> evil merge.
>
> I'd prefer to having worry about such a fallout only once.  Which
> arguably we already did when we came up with the current message, so
> I am fairly annoyed by this patch coming loooooong after we
> concluded the original discussion.
>
> Sigh X-<.  I'll worry about this later, as b2ed944a is in 'next'
> during the feature freeze, and I'd prefer not having to rebase it on
> top of the final version of this patch.

Here is a rebase of the endgame patch, on top of the result of
applying Greg's patch (you have to fix the line-wrapping in the
message, though). The only change from the version we have been
cooking since January is the message in builtin/push.c.  I haven't
checked if the result merges cleanly to other topics in flight
though.

It will be quite messy to merge this and Greg's patch to anything
past 3153a9e8 (Merge branch 'jc/push-2.0-default-to-simple' into
next, 2013-10-28), which already has the original endgame patch, so
I'll postpone it until later (I still need to tag 1.8.5-rc1 today).

Thanks.

-- >8 --
Subject: [PATCH] push: switch default from "matching" to "simple"

We promised to change the behaviour of lazy "git push [there]" that
does not say what to push on the command line from "matching" to
"simple" in Git 2.0.

This finally flips that bit.

Helped-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt   | 14 ++++----------
 Documentation/git-push.txt | 10 ++++++----
 advice.c                   |  2 --
 advice.h                   |  1 -
 builtin/push.c             | 37 ++++++++++---------------------------
 5 files changed, 20 insertions(+), 44 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab26963..bb45969 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -142,19 +142,13 @@ advice.*::
 --
 	pushUpdateRejected::
 		Set this variable to 'false' if you want to disable
-		'pushNonFFCurrent', 'pushNonFFDefault',
+		'pushNonFFCurrent',
 		'pushNonFFMatching', 'pushAlreadyExists',
 		'pushFetchFirst', and 'pushNeedsForce'
 		simultaneously.
 	pushNonFFCurrent::
 		Advice shown when linkgit:git-push[1] fails due to a
 		non-fast-forward update to the current branch.
-	pushNonFFDefault::
-		Advice to set 'push.default' to 'upstream' or 'current'
-		when you ran linkgit:git-push[1] and pushed 'matching
-		refs' by default (i.e. you did not provide an explicit
-		refspec, and no 'push.default' configuration was set)
-		and it resulted in a non-fast-forward error.
 	pushNonFFMatching::
 		Advice shown when you ran linkgit:git-push[1] and pushed
 		'matching refs' explicitly (i.e. you used ':', or
@@ -1929,7 +1923,7 @@ When pushing to a remote that is different from the remote you normally
 pull from, work as `current`.  This is the safest option and is suited
 for beginners.
 +
-This mode will become the default in Git 2.0.
+This mode has become the default in Git 2.0.
 
 * `matching` - push all branches having the same name on both ends.
   This makes the repository you are pushing to remember the set of
@@ -1948,8 +1942,8 @@ suitable for pushing into a shared central repository, as other
 people may add new branches there, or update the tip of existing
 branches outside your control.
 +
-This is currently the default, but Git 2.0 will change the default
-to `simple`.
+This used to be the default, but not since Git 2.0 (`simple` is the
+new default).
 
 --
 
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 9eec740..5553f99 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -78,8 +78,8 @@ the local side, the remote side is updated if a branch of the same name
 already exists on the remote side.
 
 --all::
-	Instead of naming each ref to push, specifies that all
-	refs under `refs/heads/` be pushed.
+	Push all branches (i.e. refs under `refs/heads/`); cannot be
+	used with other <refspec>.
 
 --prune::
 	Remove remote branches that don't have a local counterpart. For example
@@ -437,8 +437,10 @@ Examples
 	configured for the current branch).
 
 `git push origin`::
-	Without additional configuration, works like
-	`git push origin :`.
+	Without additional configuration, pushes the current branch to
+	the configured upstream (`remote.origin.merge` configuration
+	variable) if it has the same name as the current branch, and
+	errors out without pushing otherwise.
 +
 The default behavior of this command when no <refspec> is given can be
 configured by setting the `push` option of the remote, or the `push.default`
diff --git a/advice.c b/advice.c
index 3eca9f5..486f823 100644
--- a/advice.c
+++ b/advice.c
@@ -2,7 +2,6 @@
 
 int advice_push_update_rejected = 1;
 int advice_push_non_ff_current = 1;
-int advice_push_non_ff_default = 1;
 int advice_push_non_ff_matching = 1;
 int advice_push_already_exists = 1;
 int advice_push_fetch_first = 1;
@@ -23,7 +22,6 @@ static struct {
 } advice_config[] = {
 	{ "pushupdaterejected", &advice_push_update_rejected },
 	{ "pushnonffcurrent", &advice_push_non_ff_current },
-	{ "pushnonffdefault", &advice_push_non_ff_default },
 	{ "pushnonffmatching", &advice_push_non_ff_matching },
 	{ "pushalreadyexists", &advice_push_already_exists },
 	{ "pushfetchfirst", &advice_push_fetch_first },
diff --git a/advice.h b/advice.h
index 08fbc8e..5ecc6c1 100644
--- a/advice.h
+++ b/advice.h
@@ -5,7 +5,6 @@
 
 extern int advice_push_update_rejected;
 extern int advice_push_non_ff_current;
-extern int advice_push_non_ff_default;
 extern int advice_push_non_ff_matching;
 extern int advice_push_already_exists;
 extern int advice_push_fetch_first;
diff --git a/builtin/push.c b/builtin/push.c
index 5393e28..a79354d 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -26,7 +26,6 @@ static struct push_cas_option cas;
 static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;
-static int default_matching_used;
 
 static void add_refspec(const char *ref)
 {
@@ -164,9 +163,9 @@ static void setup_push_current(struct remote *remote, struct branch *branch)
 }
 
 static char warn_unspecified_push_default_msg[] =
-N_("push.default is unset; its implicit value is changing in\n"
+N_("push.default is unset; its implicit value has changed in\n"
    "Git 2.0 from 'matching' to 'simple'. To squelch this message\n"
-   "and maintain the current behavior after the default changes, use:\n"
+   "and maintain the traditional behavior, use:\n"
    "\n"
    "  git config --global push.default matching\n"
    "\n"
@@ -175,11 +174,11 @@ N_("push.default is unset; its implicit value is changing in\n"
    "  git config --global push.default simple\n"
    "\n"
    "When push.default is set to 'matching', git will push all local branches\n"
-   "to the remote branches with the same (matching) name.  This will no\n"
-   "longer be the default in Git 2.0 because a branch could be\n"
+   "to the remote branches with the same (matching) name.  This is no\n"
+   "longer the default since Git 2.0 because a branch could be\n"
    "unintentionally pushed to a remote.\n"
    "\n"
-   "In Git 2.0 the new push.default of 'simple' will push only the current\n"
+   "In Git 2.0 the new push.default of 'simple' pushes only the current\n"
    "branch to the same remote branch used by git pull.   A push will\n"
    "only succeed if the remote and local branches have the same name.\n"
    "\n"
@@ -209,14 +208,14 @@ static void setup_default_push_refspecs(struct remote *remote)
 
 	switch (push_default) {
 	default:
-	case PUSH_DEFAULT_UNSPECIFIED:
-		default_matching_used = 1;
-		warn_unspecified_push_default_configuration();
-		/* fallthru */
 	case PUSH_DEFAULT_MATCHING:
 		add_refspec(":");
 		break;
 
+	case PUSH_DEFAULT_UNSPECIFIED:
+		warn_unspecified_push_default_configuration();
+		/* fallthru */
+
 	case PUSH_DEFAULT_SIMPLE:
 		if (triangular)
 			setup_push_current(remote, branch);
@@ -245,12 +244,6 @@ static const char message_advice_pull_before_push[] =
 	   "'git pull ...') before pushing again.\n"
 	   "See the 'Note about fast-forwards' in 'git push --help' for details.");
 
-static const char message_advice_use_upstream[] =
-	N_("Updates were rejected because a pushed branch tip is behind its remote\n"
-	   "counterpart. If you did not intend to push that branch, you may want to\n"
-	   "specify branches to push or set the 'push.default' configuration variable\n"
-	   "to 'simple', 'current' or 'upstream' to push only the current branch.");
-
 static const char message_advice_checkout_pull_push[] =
 	N_("Updates were rejected because a pushed branch tip is behind its remote\n"
 	   "counterpart. Check out this branch and integrate the remote changes\n"
@@ -279,13 +272,6 @@ static void advise_pull_before_push(void)
 	advise(_(message_advice_pull_before_push));
 }
 
-static void advise_use_upstream(void)
-{
-	if (!advice_push_non_ff_default || !advice_push_update_rejected)
-		return;
-	advise(_(message_advice_use_upstream));
-}
-
 static void advise_checkout_pull_push(void)
 {
 	if (!advice_push_non_ff_matching || !advice_push_update_rejected)
@@ -347,10 +333,7 @@ static int push_with_options(struct transport *transport, int flags)
 	if (reject_reasons & REJECT_NON_FF_HEAD) {
 		advise_pull_before_push();
 	} else if (reject_reasons & REJECT_NON_FF_OTHER) {
-		if (default_matching_used)
-			advise_use_upstream();
-		else
-			advise_checkout_pull_push();
+		advise_checkout_pull_push();
 	} else if (reject_reasons & REJECT_ALREADY_EXISTS) {
 		advise_ref_already_exists();
 	} else if (reject_reasons & REJECT_FETCH_FIRST) {
-- 
1.8.5-rc0-343-g6932893

Re: [PATCH v3] push: Enhance unspecified push default warning

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> That is true, but does it justify giving a misleading information in
> the advice message?

Clearly, yes. Trying to be exhaustive here is not a good idea, we'd end
up rewritting the man page, and then users won't read the message
because it's too long.

> Specifically:
>
>>> +   "When push.default is set to 'matching', git will push all local branches\n"
>>> +   "to the remote branches with the same (matching) name.
>
> invites those who do not read documentation to mistake it with using
> an explicit "refs/heads/*:refs/heads/*" refspec.

Yes, but those who want to know the exact behavior should read the doc.
That's life.

>>> +   "In Git 2.0 the new push.default of 'simple' will push only the current\n"
>>> +   "branch to the same remote branch used by git pull.   A push will\n"
>>> +   "only succeed if the remote and local branches have the same name.\n"
>
> while you can see that it is not telling a lie if you read it twice,
> "will only succeed if" feels somewhat roundabout.
>
> 	... push only the current branch back to the branch of the
> 	same name, but only if 'git pull' is set to pull from that
> 	branch. Otherwise the push will fail.
>
> might be an improvement, but I dunno.

I do not see much difference actually. I tend to prefer the original
version: to me the expected behavior is to make push and pull
essentially symetrical, and the fact that it fails if the branch is
named differently is a safety feature comming on top of that.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v3] push: Enhance unspecified push default warning

[Jonathan Nieder]

Matthieu Moy wrote:
> Junio C Hamano <gitster@pobox.com> writes:

>> Specifically:
>>
>>>> +   "When push.default is set to 'matching', git will push all local branches\n"
>>>> +   "to the remote branches with the same (matching) name.
>>
>> invites those who do not read documentation to mistake it with using
>> an explicit "refs/heads/*:refs/heads/*" refspec.
>
> Yes, but those who want to know the exact behavior should read the doc.
> That's life.

Surely we can do better?

For example:

	When push.default is set to 'matching', git will push local branches
	to remote branches that already exist with the same (matching) name.

>>>> +   "In Git 2.0 the new push.default of 'simple' will push only the current\n"
>>>> +   "branch to the same remote branch used by git pull.   A push will\n"
>>>> +   "only succeed if the remote and local branches have the same name.\n"
>>
>> while you can see that it is not telling a lie if you read it twice,
>> "will only succeed if" feels somewhat roundabout.
>>
>> 	... push only the current branch back to the branch of the
>> 	same name, but only if 'git pull' is set to pull from that
>> 	branch. Otherwise the push will fail.
>>
>> might be an improvement, but I dunno.
>
> I do not see much difference actually. I tend to prefer the original
> version: to me the expected behavior is to make push and pull
> essentially symetrical, and the fact that it fails if the branch is
> named differently is a safety feature comming on top of that.

Perhaps:

	In Git 2.0 (or now, if push.default is set to 'simple'), git will behave
	more conservatively by pushing only the current branch to the corresponding
	remote branch used by "git pull", and only if the remote and local branches
	have the same name.

Except that forgets the exception having to do with triangular
workflows.  So maybe:

	In Git 2.0, git will default to a more conservative 'simple' behavior
	that only pushes the current branch.

Hope that helps,
Jonathan

Re: [PATCH v3] push: Enhance unspecified push default warning

[Matthieu Moy]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 	When push.default is set to 'matching', git will push local branches
> 	to remote branches that already exist with the same (matching) name.

Yes, that's better than the original patch (and remains two lines).

>>>>> +   "In Git 2.0 the new push.default of 'simple' will push only the current\n"
>>>>> +   "branch to the same remote branch used by git pull.   A push will\n"
>>>>> +   "only succeed if the remote and local branches have the same name.\n"
>>>
>>> while you can see that it is not telling a lie if you read it twice,
>>> "will only succeed if" feels somewhat roundabout.
>>>
>>> 	... push only the current branch back to the branch of the
>>> 	same name, but only if 'git pull' is set to pull from that
>>> 	branch. Otherwise the push will fail.
>>>
>>> might be an improvement, but I dunno.
>>
>> I do not see much difference actually. I tend to prefer the original
>> version: to me the expected behavior is to make push and pull
>> essentially symetrical, and the fact that it fails if the branch is
>> named differently is a safety feature comming on top of that.
>
> Perhaps:
>
> 	In Git 2.0 (or now, if push.default is set to 'simple'), git will behave
> 	more conservatively by pushing only the current branch to the corresponding
> 	remote branch used by "git pull", and only if the remote and local branches
> 	have the same name.

I prefered the original, as it had two sentences. Reading only the first
one gave the important information.

> 	In Git 2.0, git will default to a more conservative 'simple' behavior
> 	that only pushes the current branch.

That's an option too, but I think mentionning "git pull" was a good
idea.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v3] push: Enhance unspecified push default warning

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> 	When push.default is set to 'matching', git will push local branches
>> 	to remote branches that already exist with the same (matching) name.
>
> Yes, that's better than the original patch (and remains two lines).
>
>>>>>> +   "In Git 2.0 the new push.default of 'simple' will push only the current\n"
>>>>>> +   "branch to the same remote branch used by git pull.   A push will\n"
>>>>>> +   "only succeed if the remote and local branches have the same name.\n"
>>>>
>>>> while you can see that it is not telling a lie if you read it twice,
>>>> "will only succeed if" feels somewhat roundabout.
>>>>
>>>> 	... push only the current branch back to the branch of the
>>>> 	same name, but only if 'git pull' is set to pull from that
>>>> 	branch. Otherwise the push will fail.
>>>>
>>>> might be an improvement, but I dunno.
>>>
>>> I do not see much difference actually. I tend to prefer the original
>>> version: to me the expected behavior is to make push and pull
>>> essentially symetrical, and the fact that it fails if the branch is
>>> named differently is a safety feature comming on top of that.
>>
>> Perhaps:
>>
>> 	In Git 2.0 (or now, if push.default is set to 'simple'), git will behave
>> 	more conservatively by pushing only the current branch to the corresponding
>> 	remote branch used by "git pull", and only if the remote and local branches
>> 	have the same name.
>
> I prefered the original, as it had two sentences. Reading only the first
> one gave the important information.

Actually, to me, I found the "two sentences" the worst part in the
original.  It made it sound as if the default will be switching to
'upstream', and all readers need to read the second sentence that
clarifies that it is not the case, in a somewhat round-about
way---"will only succeed if" invites "and otherwise...?"

>> 	In Git 2.0, git will default to a more conservative 'simple' behavior
>> 	that only pushes the current branch.
>
> That's an option too, but I think mentionning "git pull" was a good
> idea.

Yes, mentioning "git pull" is a good idea throughout these
proposals.

Re: [PATCH v3] push: Enhance unspecified push default warning

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Actually, to me, I found the "two sentences" the worst part in the
> original.  It made it sound as if the default will be switching to
> 'upstream', and all readers need to read the second sentence that
> clarifies that it is not the case, in a somewhat round-about
> way---"will only succeed if" invites "and otherwise...?"

To me, this is not an issue given the audience (people too lazy to read
the docs): for them, current == simple == upstream modulo details
they're not interested about. The safety measure of "simple" is actually
here to make sure nothing wrong happens if they make the confusion.

But that's just a preference, I'm OK with your other proposal too.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v3] push: Enhance unspecified push default warning

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> 	When push.default is set to 'matching', git will push local branches
>> 	to remote branches that already exist with the same (matching) name.
>
> Yes, that's better than the original patch (and remains two lines).
> ...
>
>> 	In Git 2.0, git will default to a more conservative 'simple' behavior
>> 	that only pushes the current branch.
>
> That's an option too, but I think mentionning "git pull" was a good
> idea.

OK, I'll tentatively update the draft to read like this, redo the
endgame patch on top and requeue.

 builtin/push.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 5393e28..27c5754 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -174,14 +174,12 @@ N_("push.default is unset; its implicit value is changing in\n"
    "\n"
    "  git config --global push.default simple\n"
    "\n"
-   "When push.default is set to 'matching', git will push all local branches\n"
-   "to the remote branches with the same (matching) name.  This will no\n"
-   "longer be the default in Git 2.0 because a branch could be\n"
-   "unintentionally pushed to a remote.\n"
+   "When push.default is set to 'matching', git will push local branches\n"
+   "to the remote branches that already exist with the same name.\n"
    "\n"
-   "In Git 2.0 the new push.default of 'simple' will push only the current\n"
-   "branch to the same remote branch used by git pull.   A push will\n"
-   "only succeed if the remote and local branches have the same name.\n"
+   "In Git 2.0, Git will default to the more conservative 'simple'\n"
+   "behavior that only pushes the current branch to the corresponding\n"
+   "remote branch used by 'git pull' to update the current branch from.\n"
    "\n"
    "See 'git help config' and search for 'push.default' for further information.\n"
    "(the 'simple' mode was introduced in Git 1.7.11. Use the similar mode\n"

Re: [PATCH v3] push: Enhance unspecified push default warning

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> OK, I'll tentatively update the draft to read like this, redo the
> endgame patch on top and requeue.

... and the corresponding part of the endgame patch now reads like
this.  I suspect that we may want a bigger change to unstress
'simple' at that phase of the transition, though, by perhaps
rewording the 'matching' part to make it more explicit that the
setting is primarily for those who want to keep the traditional
behaviour.

diff --git a/builtin/push.c b/builtin/push.c
index 27c5754..a0534d0 100644
--- a/builtin/push.c
+++ b/builtin/push.c
 ...
@@ -164,9 +163,9 @@ static void setup_push_current(struct remote *remote, struct branch *branch)
 }
 
 static char warn_unspecified_push_default_msg[] =
-N_("push.default is unset; its implicit value is changing in\n"
+N_("push.default is unset; its implicit value has changed in\n"
    "Git 2.0 from 'matching' to 'simple'. To squelch this message\n"
-   "and maintain the current behavior after the default changes, use:\n"
+   "and maintain the traditional behavior, use:\n"
    "\n"
    "  git config --global push.default matching\n"
    "\n"
@@ -177,7 +176,7 @@ N_("push.default is unset; its implicit value is changing in\n"
    "When push.default is set to 'matching', git will push local branches\n"
    "to the remote branches that already exist with the same name.\n"
    "\n"
-   "In Git 2.0, Git will default to the more conservative 'simple'\n"
+   "Since Git 2.0, Git defaults to the more conservative 'simple'\n"
    "behavior that only pushes the current branch to the corresponding\n"
    "remote branch used by 'git pull' to update the current branch from.\n"
    "\n"
@@ -207,14 +206,14 @@ static void setup_default_push_refspecs(struct remote *remote)
 ... 

Re: [PATCH v3] push: Enhance unspecified push default warning

[Marc Branchaud]

On 13-11-08 01:02 PM, Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> 
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>>> 	When push.default is set to 'matching', git will push local branches
>>> 	to remote branches that already exist with the same (matching) name.
>>
>> Yes, that's better than the original patch (and remains two lines).
>> ...
>>
>>> 	In Git 2.0, git will default to a more conservative 'simple' behavior
>>> 	that only pushes the current branch.
>>
>> That's an option too, but I think mentionning "git pull" was a good
>> idea.
> 
> OK, I'll tentatively update the draft to read like this, redo the
> endgame patch on top and requeue.
> 
>  builtin/push.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/builtin/push.c b/builtin/push.c
> index 5393e28..27c5754 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -174,14 +174,12 @@ N_("push.default is unset; its implicit value is changing in\n"
>     "\n"
>     "  git config --global push.default simple\n"
>     "\n"
> -   "When push.default is set to 'matching', git will push all local branches\n"
> -   "to the remote branches with the same (matching) name.  This will no\n"
> -   "longer be the default in Git 2.0 because a branch could be\n"
> -   "unintentionally pushed to a remote.\n"
> +   "When push.default is set to 'matching', git will push local branches\n"
> +   "to the remote branches that already exist with the same name.\n"
>     "\n"
> -   "In Git 2.0 the new push.default of 'simple' will push only the current\n"
> -   "branch to the same remote branch used by git pull.   A push will\n"
> -   "only succeed if the remote and local branches have the same name.\n"
> +   "In Git 2.0, Git will default to the more conservative 'simple'\n"
> +   "behavior that only pushes the current branch to the corresponding\n"
> +   "remote branch used by 'git pull' to update the current branch from.\n"

That reads a bit awkwardly.  How about:

	In Git 2.0, Git will default to the more conservative 'simple'
	behavior, which only pushes the current branch to the corresponding
	remote branch that 'git pull' uses to update the current branch.

		M.

Re: [PATCH v3] push: Enhance unspecified push default warning

[Junio C Hamano]

Marc Branchaud <marcnarc@xiplink.com> writes:

>> -   "In Git 2.0 the new push.default of 'simple' will push only the current\n"
>> -   "branch to the same remote branch used by git pull.   A push will\n"
>> -   "only succeed if the remote and local branches have the same name.\n"
>> +   "In Git 2.0, Git will default to the more conservative 'simple'\n"
>> +   "behavior that only pushes the current branch to the corresponding\n"
>> +   "remote branch used by 'git pull' to update the current branch from.\n"
>
> That reads a bit awkwardly.  How about:
>
> 	In Git 2.0, Git will default to the more conservative 'simple'
> 	behavior, which only pushes the current branch to the corresponding
> 	remote branch that 'git pull' uses to update the current branch.

OK, here is the version of Greg's patch (i.e. for versions before
2.0) with the above.  The endgame patch for 2.0 would change the
line that begins with "In Git 2.0," to:

	Since Git 2.0, Git defaults to the more conservative ...

Is everybody happy with this version?

-- >8 --
From: Greg Jacobson <coder5000@gmail.com>
Date: Fri, 4 Oct 2013 10:20:07 -0400
Subject: [PATCH] push: Enhance unspecified push default warning

When the unset push.default warning message is displayed this may be
the first time many users encounter push.default.

Explain in the warning message in a compact manner what push.default
is and what the change means to the end-user to help the users decide.

Signed-off-by: Greg Jacobson <coder5000@gmail.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Helped-by: Marc Branchaud <marcnarc@xiplink.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/push.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/builtin/push.c b/builtin/push.c
index 7b1b66c..a73982a 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -174,6 +174,13 @@ N_("push.default is unset; its implicit value is changing in\n"
    "\n"
    "  git config --global push.default simple\n"
    "\n"
+   "When push.default is set to 'matching', git will push local branches\n"
+   "to the remote branches that already exist with the same name.\n"
+   "\n"
+   "In Git 2.0, Git will default to the more conservative 'simple'\n"
+   "behavior, which only pushes the current branch to the corresponding\n"
+   "remote branch that 'git pull' uses to update the current branch.\n"
+   "\n"
    "See 'git help config' and search for 'push.default' for further information.\n"
    "(the 'simple' mode was introduced in Git 1.7.11. Use the similar mode\n"
    "'current' instead of 'simple' if you sometimes use older versions of Git)");
-- 
1.8.5-rc1-310-g1febc12

Re: [PATCH v3] push: Enhance unspecified push default warning

[Jonathan Nieder]

Junio C Hamano wrote:

> Is everybody happy with this version?

Looks good to me.

Thanks,
Jonathan

Re: [PATCH v3] push: Enhance unspecified push default warning

[Marc Branchaud]

On 13-11-11 12:02 PM, Junio C Hamano wrote:
> 
> Is everybody happy with this version?

Looks good.

		M.

Re: [PATCH v3] push: Enhance unspecified push default warning

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Is everybody happy with this version?

I am.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/