[PATCH 2/2] push: --ignore-stale option

[PATCH 2/2] push: --ignore-stale option

[Junio C Hamano]

If you forked many local branches that are not active from a shared
'origin' repository, it is sometimes convenient to say "I know many of my
branches are stale, so do not push them, but push ones that have my own
work". Using the usual 'matching refs' semantics that is designed for the
workflow to push into your own publishing repository is not suitable for
this purpose as-is, because many of your local branches are likely to have
been made stale by other people pushing into the same shared repository,
triggering 'no fast-forward' errors.

Teach a new "--ignore-stale" option to "git push" which tells it not to
push stale refs (i.e. the commit that would have been pushed without the
option is an ancestor of the commit that is at the destination). With this,
a lazy workflow could be like this:

    $ git clone <<origin>>
    $ git checkout -b topic1 origin/topic1
    $ work work work
    $ git push origin :
    $ git checkout -b topic2 origin/topic2
    $ work work work
    $ git push --ignore-stale origin :

and the second push does not have to worry about other people working on
topic1 and updating it in the central repository while you haven't touched
the corresponding local branch at all.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * You may end up having 47 local branches, 7 of them worked on recently
   and none of them pushed yet, which requires you to remember which 7 of
   them you need to push among 47. The reason you left these 7 unpushed
   even though you checked out other branches at least 6 times after
   making the last commit on these branches is probably because you wanted
   to make sure everything is in good order, delaying the pushout as much
   as possible, which by itself is a good discipline.

   You however should be testing these 7 before pushing them out anyway,
   and the sane way to do so is to check one out, test it, push it, and
   iterate that sequence 7 times. If you do so, a workable alternative is
   to use the configuration to push the current branch and you do not need
   this patch series at all.

   Perhaps this series encourages a wrong workflow in that sense. I dunno.

 Documentation/git-push.txt |   10 +++++++++-
 builtin/push.c             |    1 +
 builtin/send-pack.c        |    7 +++++++
 cache.h                    |    1 +
 remote.c                   |   20 ++++++++++++++++++++
 t/t5516-fetch-push.sh      |   31 +++++++++++++++++++++++++++++++
 transport.c                |    6 ++++++
 transport.h                |    1 +
 8 files changed, 76 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index aede488..f2b5ee2 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git push' [--all | --mirror | --tags] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
 	   [--repo=<repository>] [-f | --force] [-v | --verbose] [-u | --set-upstream]
-	   [<repository> [<refspec>...]]
+	   [--ignore-stale] [<repository> [<refspec>...]]
 
 DESCRIPTION
 -----------
@@ -114,6 +114,14 @@ nor in any Push line of the corresponding remotes file---see below).
 	This flag disables the check.  This can cause the
 	remote repository to lose commits; use it with care.
 
+--ignore-stale::
+	When the commit that is pushed is known to be an ancestor of the
+	commit that is at the remote ref, exclude it from the push
+	request. This can be used with the "matching" semantics to push
+	out only the branches that have your own work, when you know many
+	of your branches since they were forked from their upstream are
+	untouched and stale.
+
 --repo=<repository>::
 	This option is only relevant if no <repository> argument is
 	passed in the invocation. In this case, 'git push' derives the
diff --git a/builtin/push.c b/builtin/push.c
index 35cce53..165d2be 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -261,6 +261,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT('u', "set-upstream", &flags, "set upstream for git pull/status",
 			TRANSPORT_PUSH_SET_UPSTREAM),
 		OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"),
+		OPT_BIT(0, "ignore-stale", &flags, "ignore stale refs", TRANSPORT_PUSH_IGNORE_STALE),
 		OPT_END()
 	};
 
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index ec107ed..0cfc69a 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -196,6 +196,11 @@ static void print_helper_status(struct ref *ref)
 			msg = "up to date";
 			break;
 
+		case REF_STATUS_STALE:
+			res = "ok";
+			msg = "ignored stale";
+			break;
+
 		case REF_STATUS_REJECT_NONFASTFORWARD:
 			res = "error";
 			msg = "non-fast forward";
@@ -282,6 +287,7 @@ int send_pack(struct send_pack_args *args,
 		switch (ref->status) {
 		case REF_STATUS_REJECT_NONFASTFORWARD:
 		case REF_STATUS_UPTODATE:
+		case REF_STATUS_STALE:
 			continue;
 		default:
 			; /* do nothing */
@@ -379,6 +385,7 @@ int send_pack(struct send_pack_args *args,
 		switch (ref->status) {
 		case REF_STATUS_NONE:
 		case REF_STATUS_UPTODATE:
+		case REF_STATUS_STALE:
 		case REF_STATUS_OK:
 			break;
 		default:
diff --git a/cache.h b/cache.h
index 8c98d05..696805d 100644
--- a/cache.h
+++ b/cache.h
@@ -1009,6 +1009,7 @@ struct ref {
 		REF_STATUS_OK,
 		REF_STATUS_REJECT_NONFASTFORWARD,
 		REF_STATUS_REJECT_NODELETE,
+		REF_STATUS_STALE,
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
 		REF_STATUS_EXPECTING_REPORT
diff --git a/remote.c b/remote.c
index 95d7f37..9c63426 100644
--- a/remote.c
+++ b/remote.c
@@ -1224,11 +1224,25 @@ int match_push_refs(struct ref *src, struct ref **dst,
 	return 0;
 }
 
+/*
+ * Do we know if the other side has newer commit than what we are
+ * trying to push (i.e. old_sha1 is descendant of new_sha1)? If so
+ * just ignore the request to push this particular bref under the
+ * "--ignore-stale" option.
+ */
+static int is_stale_push(unsigned char *old_sha1, unsigned char *new_sha1)
+{
+	if (!has_sha1_file(old_sha1) || !has_sha1_file(new_sha1))
+		return 0;
+	return ref_newer(old_sha1, new_sha1);
+}
+
 void set_ref_status_for_push(struct ref *remote_refs, unsigned flags)
 {
 	struct ref *ref;
 	int send_mirror = flags & TRANSPORT_PUSH_MIRROR;
 	int force_update = flags & TRANSPORT_PUSH_FORCE;
+	int ignore_stale = flags & TRANSPORT_PUSH_IGNORE_STALE;
 
 	for (ref = remote_refs; ref; ref = ref->next) {
 		if (ref->peer_ref)
@@ -1243,6 +1257,12 @@ void set_ref_status_for_push(struct ref *remote_refs, unsigned flags)
 			continue;
 		}
 
+		if (ignore_stale && !ref->deletion &&
+		    is_stale_push(ref->old_sha1, ref->new_sha1)) {
+			ref->status = REF_STATUS_STALE;
+			continue;
+		}
+
 		/* This part determines what can overwrite what.
 		 * The rules are:
 		 *
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b69cf57..c925640 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -979,4 +979,35 @@ test_expect_success 'push --porcelain --dry-run rejected' '
 	test_cmp .git/foo .git/bar
 '
 
+test_expect_success 'push --ignore-stale' '
+	mk_empty &&
+	(
+		cd testrepo &&
+		git fetch --update-head-ok .. "refs/heads/*:refs/heads/*" &&
+		git checkout -b next master &&
+		git commit --allow-empty -m "updated next" &&
+		git push . next:master &&
+		git for-each-ref >../snapshot.before
+	) &&
+	git checkout branch1 &&
+	git commit --allow-empty -m "updated branch1" &&
+	test_must_fail git push testrepo : &&
+	git fetch testrepo "+refs/heads/*:refs/remotes/origin/*" &&
+	git push --ignore-stale testrepo : &&
+	(
+		cd testrepo &&
+		git for-each-ref >../snapshot.after
+	) &&
+
+	# branch1 must be updated and master must stay the same
+	git for-each-ref refs/heads/branch1 >expect &&
+	grep refs/heads/branch1 snapshot.after >actual &&
+	test_cmp expect actual &&
+
+	grep refs/heads/master snapshot.before >expect &&
+	grep refs/heads/master snapshot.after >actual &&
+	test_cmp expect actual
+
+'
+
 test_done
diff --git a/transport.c b/transport.c
index 95556da..d124d70 100644
--- a/transport.c
+++ b/transport.c
@@ -566,6 +566,7 @@ static int push_had_errors(struct ref *ref)
 	for (; ref; ref = ref->next) {
 		switch (ref->status) {
 		case REF_STATUS_NONE:
+		case REF_STATUS_STALE:
 		case REF_STATUS_UPTODATE:
 		case REF_STATUS_OK:
 			break;
@@ -581,6 +582,7 @@ int transport_refs_pushed(struct ref *ref)
 	for (; ref; ref = ref->next) {
 		switch(ref->status) {
 		case REF_STATUS_NONE:
+		case REF_STATUS_STALE:
 		case REF_STATUS_UPTODATE:
 			break;
 		default:
@@ -690,6 +692,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 		print_ref_status('=', "[up to date]", ref,
 						 ref->peer_ref, NULL, porcelain);
 		break;
+	case REF_STATUS_STALE:
+		print_ref_status('=', "[stale ignored]", ref,
+						 ref->peer_ref, NULL, porcelain);
+		break;
 	case REF_STATUS_REJECT_NONFASTFORWARD:
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
 						 "non-fast-forward", porcelain);
diff --git a/transport.h b/transport.h
index 059b330..5db8d23 100644
--- a/transport.h
+++ b/transport.h
@@ -102,6 +102,7 @@ struct transport {
 #define TRANSPORT_PUSH_PORCELAIN 16
 #define TRANSPORT_PUSH_SET_UPSTREAM 32
 #define TRANSPORT_RECURSE_SUBMODULES_CHECK 64
+#define TRANSPORT_PUSH_IGNORE_STALE 128
 
 #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
 
-- 
1.7.8.249.gb1b73

Re: [PATCH 2/2] push: --ignore-stale option

[Andreas Ericsson]

On 12/14/2011 12:35 AM, Junio C Hamano wrote:
> 
>     You however should be testing these 7 before pushing them out anyway,
>     and the sane way to do so is to check one out, test it, push it, and
>     iterate that sequence 7 times. If you do so, a workable alternative is
>     to use the configuration to push the current branch and you do not need
>     this patch series at all.
> 
>     Perhaps this series encourages a wrong workflow in that sense. I dunno.
> 

I for one really love it and will probably make an alias to use for most of
my repos. With 'maint' branches (with topics) being worked on by one team and
'master' (with topics) being worked on by a different group, but all branches
generally checked out, tracked and touched by everyone sooner or later, I've
come to just ignore the "not fast-forward" error. I've never stopped loathing
it though, since git should just *know* that it doesn't fast-forward because
I haven't done anything and not because I've done something (and maybe
forgotten to push) and someone else has done something different, which would
actually be interesting for me to know.

So big thumbs up and three loud "hurrah"'s for this.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

Re: [PATCH 2/2] push: --ignore-stale option

[Jeff King]

On Tue, Dec 13, 2011 at 03:35:13PM -0800, Junio C Hamano wrote:

> Teach a new "--ignore-stale" option to "git push" which tells it not to
> push stale refs (i.e. the commit that would have been pushed without the
> option is an ancestor of the commit that is at the destination). With this,
> a lazy workflow could be like this:
> 
>     $ git clone <<origin>>
>     $ git checkout -b topic1 origin/topic1
>     $ work work work
>     $ git push origin :
>     $ git checkout -b topic2 origin/topic2
>     $ work work work
>     $ git push --ignore-stale origin :
> 
> and the second push does not have to worry about other people working on
> topic1 and updating it in the central repository while you haven't touched
> the corresponding local branch at all.

Unless the action of the people in the central repo was to rewind
history, in which case you are overwriting their work. Probably
unlikely, though.

But I still see this as solving the wrong problem, and encouraging a
dangerous workflow. Why are you using ":" to push matching branches if
you don't want to push topic1? I think a much more likely scenario is:

  $ git clone <<origin>>
  $ git checkout -b topic1 origin/topic1
  $ work work work
  $ : hmm, it's not ready yet
  $ git checkout -b topic2 origin/topic2
  $ work work work
  $ : looking good, let's ship it!
  $ git push  ;# we default to matching!

I.e., the user is not actually intending to push all matching branches,
but does so accidentally because "matching" is the default. And they
have accidentally just pushed non-ready work to topic1, which would
happen even with --ignore-stale.

IOW, I am not convinced that people are actually consciously choosing
the workflow you outlined above, and are instead interested in
--ignore-stale as a convenient way of guessing which branches should be
pushed during a matching push. But it's only a guess, and as I showed
above, it can still cause problems. I think the right solution is to
switch the default away from matching, which is the root cause of the
confusion.

-Peff

Re: [PATCH 2/2] push: --ignore-stale option

[Marc Branchaud]

On 11-12-15 02:38 AM, Jeff King wrote:
> 
> But I still see this as solving the wrong problem, and encouraging a
> dangerous workflow. Why are you using ":" to push matching branches if
> you don't want to push topic1? I think a much more likely scenario is:
> 
>   $ git clone <<origin>>
>   $ git checkout -b topic1 origin/topic1
>   $ work work work
>   $ : hmm, it's not ready yet
>   $ git checkout -b topic2 origin/topic2
>   $ work work work
>   $ : looking good, let's ship it!
>   $ git push  ;# we default to matching!
> 
> I.e., the user is not actually intending to push all matching branches,
> but does so accidentally because "matching" is the default. And they
> have accidentally just pushed non-ready work to topic1, which would
> happen even with --ignore-stale.
> 
> IOW, I am not convinced that people are actually consciously choosing
> the workflow you outlined above, and are instead interested in
> --ignore-stale as a convenient way of guessing which branches should be
> pushed during a matching push. But it's only a guess, and as I showed
> above, it can still cause problems. I think the right solution is to
> switch the default away from matching, which is the root cause of the
> confusion.

100% agree.

In my experience, people are confused by any magic that happens when a local
ref name matches a remote's ref name.  I find folks have a much easier time
with git once they embrace the notion that a remote's branches are distinct
from their local clone's, and that explicitly saying
	push origin <local>:<remote>
is the clearest way of making sure git does what you want.

It's convenient for advanced users to take advantage of the shortcuts any
ref-matching magic provides, but I find novice users tend to make mistakes as
often as not.

		M.

Re: [PATCH 2/2] push: --ignore-stale option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But I still see this as solving the wrong problem, and encouraging a
> dangerous workflow.

Yes, that is what I meant, so we are in total agreement that these people
should be encouraged not to use the matching semantics.

How that encouragement is made we may differ, though.

Re: [PATCH 2/2] push: --ignore-stale option

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>> But I still see this as solving the wrong problem, and encouraging a
>> dangerous workflow.
>
> Yes, that is what I meant, so we are in total agreement that these people
> should be encouraged not to use the matching semantics.
>
> How that encouragement is made we may differ, though.

So here is my idea of how best to implement such an encouragement.

The first patch is a documentation update/clean-up to prepare for the main
one which is the second.

[PATCH 1/2] advice: Document that they all default to true

[Junio C Hamano]

By definition, the default value of "advice.*" variables must be true and
they all control various additional help messages that are designed to aid
new users. Setting one to false is to tell Git that the user understands
the nature of the error and does not need the additional verbose help
message.

Also fix the asciidoc markup for linkgit:git-checkout[1] in the
description of the detachedHead advice by removing an excess colon.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8a7d2d4..9be7106 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -115,35 +115,32 @@ in the appropriate manual page. You will find a description of non-core
 porcelain configuration variables in the respective porcelain documentation.
 
 advice.*::
-	When set to 'true', display the given optional help message.
-	When set to 'false', do not display. The configuration variables
-	are:
+	These variables control various optional help messages designed to
+	aid new users. All 'advice.*' variables default to 'true', and you
+	can tell Git that you do not need help by setting these to 'false':
 +
 --
 	pushNonFastForward::
 		Advice shown when linkgit:git-push[1] refuses
-		non-fast-forward refs. Default: true.
+		non-fast-forward refs.
 	statusHints::
 		Directions on how to stage/unstage/add shown in the
 		output of linkgit:git-status[1] and the template shown
-		when writing commit messages. Default: true.
+		when writing commit messages.
 	commitBeforeMerge::
 		Advice shown when linkgit:git-merge[1] refuses to
 		merge to avoid overwriting local changes.
-		Default: true.
 	resolveConflict::
 		Advices shown by various commands when conflicts
 		prevent the operation from being performed.
-		Default: true.
 	implicitIdentity::
 		Advice on how to set your identity configuration when
 		your information is guessed from the system username and
-		domain name. Default: true.
-
+		domain name.
 	detachedHead::
-		Advice shown when you used linkgit::git-checkout[1] to
+		Advice shown when you used linkgit:git-checkout[1] to
 		move to the detach HEAD state, to instruct how to create
-		a local branch after the fact.  Default: true.
+		a local branch after the fact.
 --
 
 core.fileMode::
-- 
1.7.8.370.gb3269

[PATCH 2/2] push: hint to use push.default=upstream when appropriate

[Junio C Hamano]

If you push into a shared repository that others push to, and you have
local branches in your repository that matches the remote side but do not
keep them up-to-date, then 'matching refs' is not an appropriate default
for you.

Detect when push failed due to non-fast-forward _and_ we did matching refs
by default (i.e. if the user explicitly said ':' from the command line, or
had push.default set to 'matching', then we do not want to advise), and
give a hint to tell the user that the user may want to set 'push.default'
configuration variable to 'upstream', if the remote repository receives
pushes from other places.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt |    6 ++++++
 advice.c                 |    2 ++
 advice.h                 |    1 +
 builtin/push.c           |   25 +++++++++++++++++++++++++
 cache.h                  |    3 ++-
 environment.c            |    2 +-
 6 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9be7106..8da7063 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -141,6 +141,12 @@ advice.*::
 		Advice shown when you used linkgit:git-checkout[1] to
 		move to the detach HEAD state, to instruct how to create
 		a local branch after the fact.
+	pushUseUpstream::
+		Advice to set 'push.default' to 'upstream' when you ran
+		linkgit:git-push[1] and pushed 'matching refs' by default
+		(i.e. you did not have any explicit refspec on the command
+		line, and no 'push.default' configuration was set) and it
+		resulted in a non-fast-forward error.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index e02e632..9b56709 100644
--- a/advice.c
+++ b/advice.c
@@ -6,6 +6,7 @@ int advice_commit_before_merge = 1;
 int advice_resolve_conflict = 1;
 int advice_implicit_identity = 1;
 int advice_detached_head = 1;
+int advice_push_use_upstream = 1;
 
 static struct {
 	const char *name;
@@ -17,6 +18,7 @@ static struct {
 	{ "resolveconflict", &advice_resolve_conflict },
 	{ "implicitidentity", &advice_implicit_identity },
 	{ "detachedhead", &advice_detached_head },
+	{ "pushuseupstream", &advice_push_use_upstream },
 };
 
 void advise(const char *advice, ...)
diff --git a/advice.h b/advice.h
index e5d0af7..6bb89cd 100644
--- a/advice.h
+++ b/advice.h
@@ -9,6 +9,7 @@ extern int advice_commit_before_merge;
 extern int advice_resolve_conflict;
 extern int advice_implicit_identity;
 extern int advice_detached_head;
+extern int advice_push_use_upstream;
 
 int git_default_advice_config(const char *var, const char *value);
 void advise(const char *advice, ...);
diff --git a/builtin/push.c b/builtin/push.c
index 35cce53..5f8c7f4 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -9,6 +9,7 @@
 #include "transport.h"
 #include "parse-options.h"
 #include "submodule.h"
+#include "advice.h"
 
 static const char * const push_usage[] = {
 	"git push [<options>] [<repository> [<refspec>...]]",
@@ -24,6 +25,7 @@ static int progress;
 static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;
+static int default_matching_used;
 
 static void add_refspec(const char *ref)
 {
@@ -95,6 +97,9 @@ static void setup_default_push_refspecs(struct remote *remote)
 {
 	switch (push_default) {
 	default:
+	case PUSH_DEFAULT_UNSPECIFIED:
+		default_matching_used = 1;
+		/* fallthru */
 	case PUSH_DEFAULT_MATCHING:
 		add_refspec(":");
 		break;
@@ -114,6 +119,23 @@ static void setup_default_push_refspecs(struct remote *remote)
 	}
 }
 
+static const char *message_advice_use_upstream[] = {
+	"If you are pushing into a repository that receives pushes from",
+	"repositories other than the current repository, you may want to",
+	"set 'push.default' configuration variable to 'upstream' to avoid",
+	"pushing branches you haven't worked on that others have updated.",
+};
+
+static void advise_use_upstream(void)
+{
+	int i;
+
+	if (!advice_push_use_upstream)
+		return;
+	for (i = 0; i < ARRAY_SIZE(message_advice_use_upstream); i++)
+		advise(message_advice_use_upstream[i]);
+}
+
 static int push_with_options(struct transport *transport, int flags)
 {
 	int err;
@@ -136,6 +158,9 @@ static int push_with_options(struct transport *transport, int flags)
 
 	err |= transport_disconnect(transport);
 
+	if (nonfastforward && default_matching_used)
+		advise_use_upstream();
+
 	if (!err)
 		return 0;
 
diff --git a/cache.h b/cache.h
index 627fb6a..6c86725 100644
--- a/cache.h
+++ b/cache.h
@@ -624,7 +624,8 @@ enum push_default_type {
 	PUSH_DEFAULT_NOTHING = 0,
 	PUSH_DEFAULT_MATCHING,
 	PUSH_DEFAULT_UPSTREAM,
-	PUSH_DEFAULT_CURRENT
+	PUSH_DEFAULT_CURRENT,
+	PUSH_DEFAULT_UNSPECIFIED
 };
 
 extern enum branch_track git_branch_track;
diff --git a/environment.c b/environment.c
index c93b8f4..d7e6c65 100644
--- a/environment.c
+++ b/environment.c
@@ -52,7 +52,7 @@ enum safe_crlf safe_crlf = SAFE_CRLF_WARN;
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
 enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
-enum push_default_type push_default = PUSH_DEFAULT_MATCHING;
+enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
 #ifndef OBJECT_CREATION_MODE
 #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
 #endif
-- 
1.7.8.370.gb3269

Re: [PATCH 1/2] advice: Document that they all default to true

[Jeff King]

On Sun, Dec 18, 2011 at 09:35:01PM -0800, Junio C Hamano wrote:

> By definition, the default value of "advice.*" variables must be true and
> they all control various additional help messages that are designed to aid
> new users. Setting one to false is to tell Git that the user understands
> the nature of the error and does not need the additional verbose help
> message.
> 
> Also fix the asciidoc markup for linkgit:git-checkout[1] in the
> description of the detachedHead advice by removing an excess colon.

Makes sense.

Acked-by: Jeff King <peff@peff.net>

-Peff

Re: [PATCH 2/2] push: hint to use push.default=upstream when appropriate

[Jeff King]

On Sun, Dec 18, 2011 at 09:38:29PM -0800, Junio C Hamano wrote:

> If you push into a shared repository that others push to, and you have
> local branches in your repository that matches the remote side but do not
> keep them up-to-date, then 'matching refs' is not an appropriate default
> for you.
> 
> Detect when push failed due to non-fast-forward _and_ we did matching refs
> by default (i.e. if the user explicitly said ':' from the command line, or
> had push.default set to 'matching', then we do not want to advise), and
> give a hint to tell the user that the user may want to set 'push.default'
> configuration variable to 'upstream', if the remote repository receives
> pushes from other places.

I like the general idea. I initially had the thought "wait, don't we
already complain loudly when push.default is not set?". But it seems we
ripped that out long ago (I think I set push.default to "matching" to
shut it up, and then never noticed that doing so is no longer
necessary).

Focusing instead on when an actual (suspected) misconfiguration has
occurred is a much better approach. It won't catch the case of "oops,
I'm on branch 'foo' and just pushed not-ready-to-publish work to 'bar'".
But the point is not necessarily to catch every mistake, but rather
catch some easy ones and hopefully educate the user to update their
settings or modify their workflow as appropriate.

> +static const char *message_advice_use_upstream[] = {
> +	"If you are pushing into a repository that receives pushes from",
> +	"repositories other than the current repository, you may want to",
> +	"set 'push.default' configuration variable to 'upstream' to avoid",
> +	"pushing branches you haven't worked on that others have updated.",
> +};

Minor English nit: I would say "...you want to set _the_ 'push.default'
configuration variable...".

The first few lines feel a little overwhelming, as you are setting up a
hypothetical "if you are in situation X" in as few words as possible,
but it involves the word "repository" three different times (to refer to
three different repositories). I don't think there is anything
inaccurate in the text, or even that the same meaning could be conveyed
more succinctly. But I wonder if it would make sense to take a step back
and stop worrying about the potential repository setup, and try to
describe the failure more specifically.

It seems like the real problem is that they are on branch "foo", but the
matching behavior tried to push "bar", which didn't work. And we are
worried that they may be surprised that we even attempted to push "bar"
at all.

And that probably happened because of the situation you describe, but we
(and the user) don't have to think about that. We can just think about
the more immediate mistake of "oops, I didn't want to push 'bar'".

Which leads me to two ideas:

  1. This is not good advice to give when pushing the _current_ branch
     failed due to non-ff. Setting push.default to "upstream" would
     probably yield the same error. We should probably tighten the
     condition for showing this message to when a non-HEAD branch failed
     to fast-forward.

  2. The text should focus on the (possible) local misconfiguration, not
     the repo setup.

So maybe something like:

  By default, git pushes all branches that have a matching counterpart
  on the remote. In this case, some of your local branches were stale
  with respect to their remote counterparts. If you did not intend to
  push these branches, you may want to set the 'push.default'
  configuration variable to 'upstream' to push only the current branch.

I'm not 100% happy with that text, but I think you can see the direction
I am talking about.

>  static int push_with_options(struct transport *transport, int flags)
>  {
>  	int err;
> @@ -136,6 +158,9 @@ static int push_with_options(struct transport *transport, int flags)
>  
>  	err |= transport_disconnect(transport);
>  
> +	if (nonfastforward && default_matching_used)
> +		advise_use_upstream();
> +

How does the location of this message interact with the existing
messages?  It seems to come before the usual non-fast-forward advice,
but after the status table and "failed to push some refs to..." error
message.

E.g.:

  To origin
     1065894..c6935ca  master -> master
   ! [rejected]        other -> other (non-fast-forward)
  error: failed to push some refs to 'origin'
  hint: If you are pushing into a repository that receives pushes from
  hint: repositories other than the current repository, you may want to
  hint: set 'push.default' configuration variable to 'upstream' to avoid
  hint: pushing branches you haven't worked on that others have updated.
  To prevent you from losing history, non-fast-forward updates were rejected
  Merge the remote changes (e.g. 'git pull') before pushing again.  See the
  'Note about fast-forwards' section of 'git push --help' for details.

which is quite a chunk of text. If we follow my suggestion above and
only print this message for non-HEAD refs, then these two messages
become an either/or situation, I think. If the HEAD failed to
fast-forward, then the right advice is probably "git pull && git push".
If a non-HEAD failed, then the right advice is either "git checkout $X
&& git pull && git push" or "here's how to avoid accidentally pushing
$X".

So the logic would be something like:

  if (nonfastforward == NONFASTFORWARD_HEAD)
          advise_pull_before_push();
  else if (nonfastforward == NONFASTFORWAD_OTHER) {
          if (default_matching_used)
                  advise_use_upstream();
          else
                  advise_checkout_pull_push();
  }

I'm not sure that the checkout-pull-push advice is really all that good,
but we don't have much else to offer.

-Peff

Re: [PATCH 2/2] push: hint to use push.default=upstream when appropriate

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It seems like the real problem is that they are on branch "foo", but the
> matching behavior tried to push "bar", which didn't work. And we are
> worried that they may be surprised that we even attempted to push "bar"
> at all.

Of we may be seeing a non-fast-forward on 'foo' itself, which is your
1. below.

> And that probably happened because of the situation you describe, but we
> (and the user) don't have to think about that. We can just think about
> the more immediate mistake of "oops, I didn't want to push 'bar'".
>
> Which leads me to two ideas:
>
>   1. This is not good advice to give when pushing the _current_ branch
>      failed due to non-ff. Setting push.default to "upstream" would
>      probably yield the same error. We should probably tighten the
>      condition for showing this message to when a non-HEAD branch failed
>      to fast-forward.
>
>   2. The text should focus on the (possible) local misconfiguration, not
>      the repo setup.

OK, I think we are in agreement.

> So maybe something like:
>
>   By default, git pushes all branches that have a matching counterpart
>   on the remote. In this case, some of your local branches were stale
>   with respect to their remote counterparts. If you did not intend to
>   push these branches, you may want to set the 'push.default'
>   configuration variable to 'upstream' to push only the current branch.
>
> I'm not 100% happy with that text, but I think you can see the direction
> I am talking about.

As long as we can tighten the condition further to ensure that the advice
above is triggered only when appropriate, I actually am 100% happy with
that text. Seeing others do the real work for me always makes me happy ;-)

In addition to "did we use default-matching?", we should use "did we get
non-fast-forward error on a branch that is _not_ the current one?" instead
of the "did we get any non-fast-forward error?" in my patch, and the text
should match the situation more-or-less exactly.

> ... If we follow my suggestion above and
> only print this message for non-HEAD refs, then these two messages
> become an either/or situation, I think. If the HEAD failed to
> fast-forward, then the right advice is probably "git pull && git push".
> If a non-HEAD failed, then the right advice is either "git checkout $X
> && git pull && git push" or "here's how to avoid accidentally pushing
> $X".
>
> So the logic would be something like:
>
>   if (nonfastforward == NONFASTFORWARD_HEAD)
>           advise_pull_before_push();
>   else if (nonfastforward == NONFASTFORWAD_OTHER) {
>           if (default_matching_used)
>                   advise_use_upstream();
>           else
>                   advise_checkout_pull_push();
>   }

Sounds good. Spelling things out at this detail would let others who may
be interested in getting their hands dirty try to come up with an updated
patch ;-).

Thanks.