[PATCH] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[PATCH] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Matthieu Moy]

'git push' failing because of non-fast forward is a very common situation,
and a beginner does not necessarily understand "fast forward" immediately.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
That may be a bit verbose, but I think it's worth it.

Ideally, there should be a core.expertUser config variable to disable
these kind of messages, but that's another story.

 builtin-push.c |    9 ++++++++-
 transport.c    |   10 +++++++---
 transport.h    |    3 ++-
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 1d92e22..214ca77 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -140,6 +140,7 @@ static int do_push(const char *repo, int flags)
 		struct transport *transport =
 			transport_get(remote, url[i]);
 		int err;
+		int nonfastforward;
 		if (receivepack)
 			transport_set_option(transport,
 					     TRANS_OPT_RECEIVEPACK, receivepack);
@@ -148,13 +149,19 @@ static int do_push(const char *repo, int flags)
 
 		if (flags & TRANSPORT_PUSH_VERBOSE)
 			fprintf(stderr, "Pushing to %s\n", url[i]);
-		err = transport_push(transport, refspec_nr, refspec, flags);
+		err = transport_push(transport, refspec_nr, refspec, flags,
+				     &nonfastforward);
 		err |= transport_disconnect(transport);
 
 		if (!err)
 			continue;
 
 		error("failed to push some refs to '%s'", url[i]);
+		if (nonfastforward) {
+			printf("Some branch push were rejected due to non-fast forward:\n");
+			printf("Merge the remote changes (git pull) before pushing your's\n");
+			printf("or use git push --force to discard the remote changes.\n");
+		}
 		errs++;
 	}
 	return !!errs;
diff --git a/transport.c b/transport.c
index de0d587..f231b35 100644
--- a/transport.c
+++ b/transport.c
@@ -820,7 +820,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 }
 
 static void print_push_status(const char *dest, struct ref *refs,
-							  int verbose, int porcelain)
+			      int verbose, int porcelain, int * nonfastforward)
 {
 	struct ref *ref;
 	int n = 0;
@@ -835,11 +835,14 @@ static void print_push_status(const char *dest, struct ref *refs,
 		if (ref->status == REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
 
+	*nonfastforward = 0;
 	for (ref = refs; ref; ref = ref->next) {
 		if (ref->status != REF_STATUS_NONE &&
 		    ref->status != REF_STATUS_UPTODATE &&
 		    ref->status != REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
+		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD)
+			*nonfastforward = 1;
 	}
 }
 
@@ -997,7 +1000,8 @@ int transport_set_option(struct transport *transport,
 }
 
 int transport_push(struct transport *transport,
-		   int refspec_nr, const char **refspec, int flags)
+		   int refspec_nr, const char **refspec, int flags,
+		   int * nonfastforward)
 {
 	verify_remote_names(refspec_nr, refspec);
 
@@ -1024,7 +1028,7 @@ int transport_push(struct transport *transport,
 
 		ret = transport->push_refs(transport, remote_refs, flags);
 
-		print_push_status(transport->url, remote_refs, verbose | porcelain, porcelain);
+		print_push_status(transport->url, remote_refs, verbose | porcelain, porcelain, nonfastforward);
 
 		if (!(flags & TRANSPORT_PUSH_DRY_RUN)) {
 			struct ref *ref;
diff --git a/transport.h b/transport.h
index 51b5397..639f13d 100644
--- a/transport.h
+++ b/transport.h
@@ -68,7 +68,8 @@ int transport_set_option(struct transport *transport, const char *name,
 			 const char *value);
 
 int transport_push(struct transport *connection,
-		   int refspec_nr, const char **refspec, int flags);
+		   int refspec_nr, const char **refspec, int flags,
+		   int * nonfastforward);
 
 const struct ref *transport_get_remote_refs(struct transport *transport);
 
-- 
1.6.4.57.g116738.dirty

Re: [PATCH] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Michael J Gruber]

Matthieu Moy venit, vidit, dixit 06.08.2009 19:32:
> 'git push' failing because of non-fast forward is a very common situation,
> and a beginner does not necessarily understand "fast forward" immediately.
> 
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> That may be a bit verbose, but I think it's worth it.
> 
> Ideally, there should be a core.expertUser config variable to disable
> these kind of messages, but that's another story.
> 
>  builtin-push.c |    9 ++++++++-
>  transport.c    |   10 +++++++---
>  transport.h    |    3 ++-
>  3 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin-push.c b/builtin-push.c
> index 1d92e22..214ca77 100644
> --- a/builtin-push.c
> +++ b/builtin-push.c
> @@ -140,6 +140,7 @@ static int do_push(const char *repo, int flags)
>  		struct transport *transport =
>  			transport_get(remote, url[i]);
>  		int err;
> +		int nonfastforward;
>  		if (receivepack)
>  			transport_set_option(transport,
>  					     TRANS_OPT_RECEIVEPACK, receivepack);
> @@ -148,13 +149,19 @@ static int do_push(const char *repo, int flags)
>  
>  		if (flags & TRANSPORT_PUSH_VERBOSE)
>  			fprintf(stderr, "Pushing to %s\n", url[i]);
> -		err = transport_push(transport, refspec_nr, refspec, flags);
> +		err = transport_push(transport, refspec_nr, refspec, flags,
> +				     &nonfastforward);
>  		err |= transport_disconnect(transport);
>  
>  		if (!err)
>  			continue;
>  
>  		error("failed to push some refs to '%s'", url[i]);
> +		if (nonfastforward) {
> +			printf("Some branch push were rejected due to non-fast forward:\n");
> +			printf("Merge the remote changes (git pull) before pushing your's\n");
> +			printf("or use git push --force to discard the remote changes.\n");
> +		}
>  		errs++;
>  	}
>  	return !!errs;

May I suggest "Some push was rejected because it would not result in a
fast forward:\n Merge in the remote changes (using git pull) before
pushing yours\n or use..."?

Cheers,
Michael

> diff --git a/transport.c b/transport.c
> index de0d587..f231b35 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -820,7 +820,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
>  }
>  
>  static void print_push_status(const char *dest, struct ref *refs,
> -							  int verbose, int porcelain)
> +			      int verbose, int porcelain, int * nonfastforward)
>  {
>  	struct ref *ref;
>  	int n = 0;
> @@ -835,11 +835,14 @@ static void print_push_status(const char *dest, struct ref *refs,
>  		if (ref->status == REF_STATUS_OK)
>  			n += print_one_push_status(ref, dest, n, porcelain);
>  
> +	*nonfastforward = 0;
>  	for (ref = refs; ref; ref = ref->next) {
>  		if (ref->status != REF_STATUS_NONE &&
>  		    ref->status != REF_STATUS_UPTODATE &&
>  		    ref->status != REF_STATUS_OK)
>  			n += print_one_push_status(ref, dest, n, porcelain);
> +		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD)
> +			*nonfastforward = 1;
>  	}
>  }
>  
> @@ -997,7 +1000,8 @@ int transport_set_option(struct transport *transport,
>  }
>  
>  int transport_push(struct transport *transport,
> -		   int refspec_nr, const char **refspec, int flags)
> +		   int refspec_nr, const char **refspec, int flags,
> +		   int * nonfastforward)
>  {
>  	verify_remote_names(refspec_nr, refspec);
>  
> @@ -1024,7 +1028,7 @@ int transport_push(struct transport *transport,
>  
>  		ret = transport->push_refs(transport, remote_refs, flags);
>  
> -		print_push_status(transport->url, remote_refs, verbose | porcelain, porcelain);
> +		print_push_status(transport->url, remote_refs, verbose | porcelain, porcelain, nonfastforward);
>  
>  		if (!(flags & TRANSPORT_PUSH_DRY_RUN)) {
>  			struct ref *ref;
> diff --git a/transport.h b/transport.h
> index 51b5397..639f13d 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -68,7 +68,8 @@ int transport_set_option(struct transport *transport, const char *name,
>  			 const char *value);
>  
>  int transport_push(struct transport *connection,
> -		   int refspec_nr, const char **refspec, int flags);
> +		   int refspec_nr, const char **refspec, int flags,
> +		   int * nonfastforward);
>  
>  const struct ref *transport_get_remote_refs(struct transport *transport);
>  

Re: [PATCH] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Matthieu Moy]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> May I suggest "Some push was rejected because it would not result in a
> fast forward:\n Merge in the remote changes (using git pull) before
> pushing yours\n or use..."?

Are you sure this is "Some push _was_ ..."? In the general case,
several branches are rejected, so that would be "were", no?

-- 
Matthieu

Re: [PATCH] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Michael J Gruber]

Matthieu Moy venit, vidit, dixit 07.08.2009 21:21:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> May I suggest "Some push was rejected because it would not result in a
>> fast forward:\n Merge in the remote changes (using git pull) before
>> pushing yours\n or use..."?
> 
> Are you sure this is "Some push _was_ ..."? In the general case,
> several branches are rejected, so that would be "were", no?
> 

Well, I'm certainly sure about the "yours" vs "your's" and about the
rewording of "due to".

If you want plural then please use "Some pushes were". I suggested the
singular, "A push was" or "Some push was". "Some" can denote an amount
but it is also used as a determiner in sentences like: "Some guy here
pretends to know English although he's not a native speaker." That would
be me :)

We don't know how many pushes failed, only that at least one did. Being
a mathematician I have to use the singular here, but feel free to use
the plural (also for the noun).

Cheers,
Michael

Re: [PATCH] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> 'git push' failing because of non-fast forward is a very common situation,
> and a beginner does not necessarily understand "fast forward" immediately.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> That may be a bit verbose, but I think it's worth it.
> ...
> +		if (nonfastforward) {
> +			printf("Some branch push were rejected due to non-fast forward:\n");
> +			printf("Merge the remote changes (git pull) before pushing your's\n");
> +			printf("or use git push --force to discard the remote changes.\n");
> +		}

Although I think the patch identified the right place to make changes, I
am not sure about what the help message should say.

If the user lacks understanding of what a fast-forward is, I do not think
giving two choices that would produce vastly different results (because
they are applicable in vastly different situations) would help such a user
very much, as the understanding of the concept of fast-forward is a must
to correctly decide which one to use.

Unfortunately, I do not think we have a good description of fast-forward
in our documentation set.  The glossary defines what it is, git-push
manual page says by default only fast-forwards are accepted, and
user-manual says that we do not create a merge commit in a fast-forward
situation.  But nobody talks about _why_ a non-fast-forward update (either
push or fetch) is a bad idea clearly.

I wrote that in my upcoming book so we could refer to it in this error
message, but I suspect it may not help most people since it is in Japanese
;-)

Jokes aside, perhaps we could add "see git-push documentation for details"
to the above message of yours, and add something like this to the
documentation.

 Documentation/git-push.txt |   75 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 75 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 2653388..c1ae82d 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -195,6 +195,81 @@ reason::
 	refs, no explanation is needed. For a failed ref, the reason for
 	failure is described.
 
+Note about fast-forwards
+------------------------
+
+When an update changes a branch (or more in general, a ref) that used to
+point at commit A to point at another commit B, it is called a
+fast-forward update if and only if B is a descendant of A.
+
+In a fast-forward update from A to B, the set of commits that the original
+commit A built on top of is a subset of the commits the new commit B
+builds on top of.  Hence, it does not lose any history.
+
+In contrast, a non-fast-forward update will lose history.  For example,
+suppose you and somebody else started at the same commit X, and you built
+a history leading to commit B while the other person built a history
+leading to commit A.  The history looks like this:
+
+----------------
+
+      B
+     /
+ ---X---A
+
+----------------
+
+Further suppose that the other person already pushed changes leading to A
+back to the original repository you two obtained the original commit X.
+
+The push done by the other person updated the branch that used to point at
+commit X to point at commit A.  It is a fast-forward.
+
+But if you try to push, you will attempt to update the branch (that
+now points at A) with commit B.  This does _not_ fast-forward.  If you did
+so, the changes introduced by commit A will be lost, because everybody
+will now start building on top of B.
+
+The command by default does not allow an update that is not a fast-forward
+to prevent such loss of history.
+
+If you do not want to lose your work (history from X to B) nor the work by
+the other person (history from X to A), you would need to first fetch the
+history from the repository, create a history that contains changes done
+by both parties, and push the result back.
+
+You can perform "git pull", resolve potential conflicts, and "git push"
+the result.  A "git pull" will create a merge commit C between commits A
+and B.
+
+----------------
+
+      B---C
+     /   /
+ ---X---A
+
+----------------
+
+Updating A with the resulting merge commit will fast-forward and your
+push will be accepted.
+
+Alternatively, you can rebase your change between X and B on top of A,
+with "git pull --rebase", and push the result back.  The rebase will
+create a new commit D that builds the change between X and B on top of
+A.
+
+----------------
+
+      B   D
+     /   /
+ ---X---A
+
+----------------
+
+Again, updating A with this commit will fast-forward and your push will be
+accepted.
+
+
 Examples
 --------
 

[PATCH] Re: push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Nicolas Sebrecht]

The 06/08/09, Junio C Hamano wrote:

>  Documentation/git-push.txt |   75 ++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 75 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index 2653388..c1ae82d 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -195,6 +195,81 @@ reason::
>  	refs, no explanation is needed. For a failed ref, the reason for
>  	failure is described.
>  
> +Note about fast-forwards
> +------------------------
> +
> +When an update changes a branch (or more in general, a ref) that used to
> +point at commit A to point at another commit B, it is called a
> +fast-forward update if and only if B is a descendant of A.
> +
> +In a fast-forward update from A to B, the set of commits that the original
> +commit A built on top of is a subset of the commits the new commit B
> +builds on top of.  Hence, it does not lose any history.
> +
> +In contrast, a non-fast-forward update will lose history.

I believe that this sentence a bit too much scaring for the beginner.
There are two kinds of update (push and pull). We loose history only
when pushing. I know this applies to Documentation/git-push.txt but out
of this context (and because we talk about pull near from here), I think
it would be clearer to say something like:

	In contrast, a non-fast-forward push will loose history.

>                                                             For example,
> +suppose you and somebody else started at the same commit X, and you built
> +a history leading to commit B while the other person built a history
> +leading to commit A.  The history looks like this:
> +
> +----------------
> +
> +      B
> +     /
> + ---X---A
> +
> +----------------

<...>

> +Alternatively, you can rebase your change between X and B on top of A,
> +with "git pull --rebase", and push the result back.  The rebase will
> +create a new commit D that builds the change between X and B on top of
> +A.
> +
> +----------------
> +
> +      B   D
> +     /   /
> + ---X---A
> +
> +----------------

Wouldn't "git pull --rebase" loose B? Shouldn't we have this

  ----------------
  
            D
           /
   ---X---A
  
  ----------------

instead?

-- 
Nicolas Sebrecht

Re: [PATCH] Re: push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Junio C Hamano]

Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:

>> +Note about fast-forwards
>> +------------------------
>> +
>> +When an update changes a branch (or more in general, a ref) that used to
>> +point at commit A to point at another commit B, it is called a
>> +fast-forward update if and only if B is a descendant of A.
>> +
>> +In a fast-forward update from A to B, the set of commits that the original
>> +commit A built on top of is a subset of the commits the new commit B
>> +builds on top of.  Hence, it does not lose any history.
>> +
>> +In contrast, a non-fast-forward update will lose history.
>
> I believe that this sentence a bit too much scaring for the beginner.
> There are two kinds of update (push and pull). We loose history only
> when pushing.

Three points that makes me think your suggested update is not appropriate
are:

 (1) This patch is about git-push documentation;

 (2) The opposite of git-push is git-fetch, not git-pull; and a non
     fast-forward fetch does lose history if you start building on a
     now-rewound tip of the remote tracking branch; and

 (3) We _do_ want this section to be scary.  We want the readers to be
     fully aware of the implications before tempting them with the --force
     option.

>> +Alternatively, you can rebase your change between X and B on top of A,
>> +with "git pull --rebase", and push the result back.  The rebase will
>> +create a new commit D that builds the change between X and B on top of
>> +A.
>> +
>> +----------------
>> +
>> +      B   D
>> +     /   /
>> + ---X---A
>> +
>> +----------------
>
> Wouldn't "git pull --rebase" loose B? Shouldn't we have this
>
>   ----------------
>   
>             D
>            /
>    ---X---A
>   
>   ----------------
>
> instead?

This makes B _loose_ (or, dangling), but does not _lose_ it.  It is still
reachable from the reflog.

We could choose to not draw it for simplicity, or we could annotate it
like this for completeness (and to give a warm-fuzzy feeling to the reader
that nothing is lost).

----------------

       branch@{1} (reachable from reflog)
             branch
      B      D
     /      /
 ---X------A

----------------

I don't know which is better.  If we were printing in colours in the
documentation, I would keep B but draw it in light gray.

Re: [PATCH] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> 'git push' failing because of non-fast forward is a very common situation,
>> and a beginner does not necessarily understand "fast forward" immediately.
>>
>> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>> ---
>> That may be a bit verbose, but I think it's worth it.
>> ...
>> +		if (nonfastforward) {
>> +			printf("Some branch push were rejected due to non-fast forward:\n");
>> +			printf("Merge the remote changes (git pull) before pushing your's\n");
>> +			printf("or use git push --force to discard the remote changes.\n");
>> +		}
>
> Although I think the patch identified the right place to make changes, I
> am not sure about what the help message should say.
>
> If the user lacks understanding of what a fast-forward is, I do not think
> giving two choices that would produce vastly different results

Well, there are different levels of mis-understanding of
"fast-forward", and one of them is just a mis-understanding of the
wording.

My experience is that many people understand "there are changes over
there that you don't have so an explicit merge is needed", but would
not necessarily use the wording "fast-forward" for this.

The second line of my message was not only here to point to "git
pull", but had also a subliminal message stating that there are
remote changes ;-).

> Jokes aside, perhaps we could add "see git-push documentation for details"
> to the above message of yours, and add something like this to the
> documentation.

100% convinced that this section in the doc is a good idea. I'm not
totally sure the error message should point to it, because that will
make the message 4 lines long instead of 3, so it may start disturbing
gurus for whom the whole message is useless.

Right now, I have this in my tree:

  if (nonfastforward) {
  	printf("Some push were rejected because it would not result in a fast forward:\n"
  	       "Merge in the remote changes (using git pull) before pushing yours, or\n"
  	       "use git push --force to discard the remote changes.\n"
  	       "See 'non-fast forward' section of 'git push --help' for details.\n");
  }

> +Alternatively, you can rebase your change between X and B on top of A,
> +with "git pull --rebase", and push the result back.  The rebase will
> +create a new commit D that builds the change between X and B on top of
> +A.
> +
> +----------------
> +
> +      B   D
> +     /   /
> + ---X---A
> +
> +----------------
> +
> +Again, updating A with this commit will fast-forward and your push will be
> +accepted.

Maybe add something about --force ? I don't like my wording very much,
but a first try is this:

Lastly, you can decide that the B shouldn't have existed, and delete
it. This is to do with a lot of care, not only because it will discard
the changes introduced in B, but also because if B has been pulled by
someone else, he will have a view of history inconsistant with the
original repository. This is done with the --force option.

-- 
Matthieu

Re: [PATCH] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> +Alternatively, you can rebase your change between X and B on top of A,
>> +with "git pull --rebase", and push the result back.  The rebase will
>> +create a new commit D that builds the change between X and B on top of
>> +A.
>> +
>> +----------------
>> +
>> +      B   D
>> +     /   /
>> + ---X---A
>> +
>> +----------------
>> +
>> +Again, updating A with this commit will fast-forward and your push will be
>> +accepted.
>
> Maybe add something about --force ? I don't like my wording very much,
> but a first try is this:
>
> Lastly, you can decide that the B shouldn't have existed, and delete
> it. This is to do with a lot of care, not only because it will discard
> the changes introduced in B, but also because if B has been pulled by
> someone else, he will have a view of history inconsistant with the
> original repository. This is done with the --force option.

To be consistent with the flow, I think you are discarding A in the
example, not B.  A is what somebody else pushed out before your failed
attempt of pushing B, and --force will discard A, replacing its history
with yours.

Of course, you also could decide that somebody else's change A is vastly
superior than your crappy B, and you may decide to do "git reset --hard A"
to get rid of your history locally; but you wouldn't be using "git push"
after that.  It is an equally valid outcome in the example situation and
until you fetch to see what A is, you cannot decide.

So, probably the order to teach would be:

 - You can pull to merge, or pull --rebase to rebase; either way, you are
   trying to preserve both histories.  [I've written on this in the
   previous message]

 - But you may realize that the commit by the other (i.e. A) was an
   incorrect solution to the same problem you solved with your B.  You
   _could_ force the push to replace it with B in such a case.  You need
   to tell the person who pushed A (and everybody else who might have
   fetched A and built on top) to discard their history (and rebuild their
   work that was done on top of A on top of B). [This is yours with A <=> B]

 - Alternatively you may realize that the commit by the other (i.e. A) was
   much better solution to the same problem you tried to solve with your
   B.  In such a case, you can simply discard B in your history with "git
   reset --hard A" after fetching.  You wouldn't be pushing anything back
   in this case.

I actually do not think it is appropriate to teach --force in an example
that involves more than one person (iow, in the context of the example in
my patch).  A lot better alternative in such a case is to "git merge -s
ours A" and push the result out, which keeps the fast-forwardness for the
person who did A, and others who pulled and built on top of A already.

So scratch your "lastly", replace it (and the second point in my list
above) with:

 - You may realize that the commit by the other (i.e. A) was an incorrect
   solution to the same problem you solved with your B.  In such a case,
   do _not_ use --force to remove A from the public history.  Instead,
   resolve the merge (in the previous instruction) favoring your solution,
   e.g. "git pull -s ours", and push the result out.

Re: [PATCH] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Matthieu Moy]

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

> I actually do not think it is appropriate to teach --force in an example
> that involves more than one person (iow, in the context of the example in
> my patch).

Right, that is the point. A more accurate example would be "oops, I
rewrote history after a push, shall I still push it?". But your
proposal is already long, let's not over-document it.

-- 
Matthieu

[PATCH v2] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Matthieu Moy]

'git push' failing because of non-fast forward is a very common situation,
and a beginner does not necessarily understand "fast forward" immediately.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
(hmm, I thought I had sent this v2 already, but seems I didn't)

With grammar fixes from Michael J Gruber.

Junio, this comes on top of your patch, since it refers to the
to-be-added section in 'git push --help'.

 builtin-push.c |   10 +++++++++-
 transport.c    |   10 +++++++---
 transport.h    |    3 ++-
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 1d92e22..5d4df7f 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -140,6 +140,7 @@ static int do_push(const char *repo, int flags)
 		struct transport *transport =
 			transport_get(remote, url[i]);
 		int err;
+		int nonfastforward;
 		if (receivepack)
 			transport_set_option(transport,
 					     TRANS_OPT_RECEIVEPACK, receivepack);
@@ -148,13 +149,20 @@ static int do_push(const char *repo, int flags)
 
 		if (flags & TRANSPORT_PUSH_VERBOSE)
 			fprintf(stderr, "Pushing to %s\n", url[i]);
-		err = transport_push(transport, refspec_nr, refspec, flags);
+		err = transport_push(transport, refspec_nr, refspec, flags,
+				     &nonfastforward);
 		err |= transport_disconnect(transport);
 
 		if (!err)
 			continue;
 
 		error("failed to push some refs to '%s'", url[i]);
+		if (nonfastforward) {
+			printf("Push was rejected because it would not result in a fast forward:\n"
+			       "Merge in the remote changes (using git pull) before pushing yours,\n"
+			       "or use git push --force to discard the remote changes.\n"
+			       "See 'non-fast forward' section of 'git push --help' for details.\n");
+		}
 		errs++;
 	}
 	return !!errs;
diff --git a/transport.c b/transport.c
index de0d587..f231b35 100644
--- a/transport.c
+++ b/transport.c
@@ -820,7 +820,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 }
 
 static void print_push_status(const char *dest, struct ref *refs,
-							  int verbose, int porcelain)
+			      int verbose, int porcelain, int * nonfastforward)
 {
 	struct ref *ref;
 	int n = 0;
@@ -835,11 +835,14 @@ static void print_push_status(const char *dest, struct ref *refs,
 		if (ref->status == REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
 
+	*nonfastforward = 0;
 	for (ref = refs; ref; ref = ref->next) {
 		if (ref->status != REF_STATUS_NONE &&
 		    ref->status != REF_STATUS_UPTODATE &&
 		    ref->status != REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
+		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD)
+			*nonfastforward = 1;
 	}
 }
 
@@ -997,7 +1000,8 @@ int transport_set_option(struct transport *transport,
 }
 
 int transport_push(struct transport *transport,
-		   int refspec_nr, const char **refspec, int flags)
+		   int refspec_nr, const char **refspec, int flags,
+		   int * nonfastforward)
 {
 	verify_remote_names(refspec_nr, refspec);
 
@@ -1024,7 +1028,7 @@ int transport_push(struct transport *transport,
 
 		ret = transport->push_refs(transport, remote_refs, flags);
 
-		print_push_status(transport->url, remote_refs, verbose | porcelain, porcelain);
+		print_push_status(transport->url, remote_refs, verbose | porcelain, porcelain, nonfastforward);
 
 		if (!(flags & TRANSPORT_PUSH_DRY_RUN)) {
 			struct ref *ref;
diff --git a/transport.h b/transport.h
index 51b5397..639f13d 100644
--- a/transport.h
+++ b/transport.h
@@ -68,7 +68,8 @@ int transport_set_option(struct transport *transport, const char *name,
 			 const char *value);
 
 int transport_push(struct transport *connection,
-		   int refspec_nr, const char **refspec, int flags);
+		   int refspec_nr, const char **refspec, int flags,
+		   int * nonfastforward);
 
 const struct ref *transport_get_remote_refs(struct transport *transport);
 
-- 
1.6.4.62.g39c83.dirty

Re: [PATCH v2] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Teemu Likonen]

On 2009-08-08 09:51 (+0200), Matthieu Moy wrote:
> 'git push' failing because of non-fast forward is a very common situation,
> and a beginner does not necessarily understand "fast forward" immediately.

> +		if (nonfastforward) {
> +			printf("Push was rejected because it would not result in a fast forward:\n"
> +			       "Merge in the remote changes (using git pull) before pushing yours,\n"
> +			       "or use git push --force to discard the remote changes.\n"
> +			       "See 'non-fast forward' section of 'git push --help' for details.\n");
> +		}

I'd like to add that some projects that use Git in (partially)
centralized manner prefer "git pull --rebase" before "git push".

Re: [PATCH v2] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Matthieu Moy]

Teemu Likonen <tlikonen@iki.fi> writes:

> On 2009-08-08 09:51 (+0200), Matthieu Moy wrote:
>> 'git push' failing because of non-fast forward is a very common situation,
>> and a beginner does not necessarily understand "fast forward" immediately.
>
>> +		if (nonfastforward) {
>> +			printf("Push was rejected because it would not result in a fast forward:\n"
>> +			       "Merge in the remote changes (using git pull) before pushing yours,\n"
>> +			       "or use git push --force to discard the remote changes.\n"
>> +			       "See 'non-fast forward' section of 'git push --help' for details.\n");
>> +		}
>
> I'd like to add that some projects that use Git in (partially)
> centralized manner prefer "git pull --rebase" before "git push".

Right, but I don't think this error message is the place to discuss
that. Anything involving rebasing should be taken with care, and
pointing the user to it in a short sentence sounds like "try shooting
yourself in the foot, and read the man page if it hurts" ;-).

-- 
Matthieu

Re: [PATCH v2] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> Teemu Likonen <tlikonen@iki.fi> writes:
>
>> On 2009-08-08 09:51 (+0200), Matthieu Moy wrote:
>>> 'git push' failing because of non-fast forward is a very common situation,
>>> and a beginner does not necessarily understand "fast forward" immediately.
>>
>>> +		if (nonfastforward) {
>>> +			printf("Push was rejected because it would not result in a fast forward:\n"
>>> +			       "Merge in the remote changes (using git pull) before pushing yours,\n"
>>> +			       "or use git push --force to discard the remote changes.\n"
>>> +			       "See 'non-fast forward' section of 'git push --help' for details.\n");
>>> +		}
>>
>> I'd like to add that some projects that use Git in (partially)
>> centralized manner prefer "git pull --rebase" before "git push".
>
> Right, but I don't think this error message is the place to discuss
> that. Anything involving rebasing should be taken with care, and
> pointing the user to it in a short sentence sounds like "try shooting
> yourself in the foot, and read the man page if it hurts" ;-).

Instead of saying "Merge in", we could say "Integrate" to cover both
practices.  I also happen to think that the mention of --force falls into
the same category as "try shooting and then study if it hurgs".

So how about phrasing it like this?

    Non-fast forward pushes were rejected because you would discard remote
    changes you have not seen.  Integrate them with your changes and then
    push again. See 'non-fast forward' section of 'git push --help'.

I think you can throw in a discussion on --force to the manual page, too.

[PATCH v2] Re: push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Nicolas Sebrecht]

The 08/08/09, Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> > Teemu Likonen <tlikonen@iki.fi> writes:
> >
> > Right, but I don't think this error message is the place to discuss
> > that. Anything involving rebasing should be taken with care, and
> > pointing the user to it in a short sentence sounds like "try shooting
> > yourself in the foot, and read the man page if it hurts" ;-).
> 
> Instead of saying "Merge in", we could say "Integrate" to cover both
> practices.  I also happen to think that the mention of --force falls into
> the same category as "try shooting and then study if it hurgs".

I'd say that everywhere we try to guess what the user should do (and
telling him to do so) falls into this category. Of course, some
operations are really destructive with no way to recover to a previous
state where some other operations aren't destructive at all. But Git can
be used in many various workflows and telling ― in an error/warning
message ― what the user should do may hurt some of the workflows and/or
finally confuse the beginners. Actually, I believe that a message with
only "See the /WONDERFULL/ section in the /LEARN_GIT/ man page" is the
best answer.

Also, I think that mentionning --force is the worst thing to do because
the beginner will immediatly run it without thinking of all the
consequences at all: "Oh, we could do that, let's try".

> So how about phrasing it like this?
> 
>     Non-fast forward pushes were rejected because you would discard remote
>     changes you have not seen.  Integrate them with your changes and then
>     push again. See 'non-fast forward' section of 'git push --help'.

Well, a beginner may rewrite a commit whithout realize what he did. If he is
the only to work on the projet, this message is somewhat wrong.

What about

     Non-fast forward pushes were rejected because you would discard remote
     changes. See 'non-fast forward' section of 'git push --help'.

?

-- 
Nicolas Sebrecht

Re: [PATCH v2] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Matthieu Moy]

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

> Instead of saying "Merge in", we could say "Integrate" to cover both
> practices.

I'm fine with both. I consider rebasing as a kind of merging, but ...

> I also happen to think that the mention of --force falls into the
> same category as "try shooting and then study if it hurgs".

Depending on the context. In the case of

git push
git commit --amend
git push

Pointing the user to 'git pull' is probably the thing which hurts the
most. And to me, the name --force already means "yes, I know what I'm
doing". My proposal was "[...] use git push --force to discard the
remote changes." which warns enough about the danger.

> So how about phrasing it like this?
>
>     Non-fast forward pushes were rejected because you would discard remote
>     changes you have not seen.  Integrate them with your changes and then
>     push again. See 'non-fast forward' section of 'git push --help'.

I thing not pointing to 'git pull' in the message really defeats the
purpose of the patch. I don't find an error message only telling me
"go read the doc as you should have done from the beginning" really
helps.

-- 
Matthieu

Re: [PATCH v2] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

>> So how about phrasing it like this?
>>
>>     Non-fast forward pushes were rejected because you would discard remote
>>     changes you have not seen.  Integrate them with your changes and then
>>     push again. See 'non-fast forward' section of 'git push --help'.
>
> I thing not pointing to 'git pull' in the message really defeats the
> purpose of the patch. I don't find an error message only telling me
> "go read the doc as you should have done from the beginning" really
> helps.

What the above three lines does much more than that.

If you read "would discard remote changes" and understand that it is what
you want, then you may know to try --force, without having to read the
doc, or if you do not remember --force, "git push --help" would remind
you.

If you read "Integrate them with your changes" and understand that it is
talking about "git pull" or "git pull --rebase", then you do not have to
read the doc.  It should "click".

If you lack such a basic understanding, you are better off go reading the
doc after all.

Re: [PATCH v2] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Matthieu Moy]

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

> If you read "Integrate them with your changes" and understand that it is
> talking about "git pull" or "git pull --rebase", then you do not have to
> read the doc.  It should "click".

But what's the point is being so vague, while you could just add "(see
git pull)"? See what you've just wrote: one should "understand that it
is about ...". So, why write Y thinking that the user should
understand that it is about X while you could write X directly?

-- 
Matthieu

Re: [PATCH v2] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> If you read "Integrate them with your changes" and understand that it is
>> talking about "git pull" or "git pull --rebase", then you do not have to
>> read the doc.  It should "click".
>
> But what's the point is being so vague, while you could just add "(see
> git pull)"? See what you've just wrote: one should "understand that it
> is about ...". So, why write Y thinking that the user should
> understand that it is about X while you could write X directly?

In order to cover both "pull" and "pull --rebase"?

Re: [PATCH v2] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> If you read "Integrate them with your changes" and understand that it is
>>> talking about "git pull" or "git pull --rebase", then you do not have to
>>> read the doc.  It should "click".
>>
>> But what's the point is being so vague, while you could just add "(see
>> git pull)"? See what you've just wrote: one should "understand that it
>> is about ...". So, why write Y thinking that the user should
>> understand that it is about X while you could write X directly?
>
> In order to cover both "pull" and "pull --rebase"?

"git pull" also covers both. "pull" is the command, "--rebase" is an
option of it.

-- 
Matthieu

Re: [PATCH v2] push: point to 'git pull' and 'git push --force' in case of non-fast forward

[Nanako Shiraishi]

From: Matthieu Moy <Matthieu.Moy@imag.fr>
Date: Sat,  8 Aug 2009 09:51:08 +0200
Subject: [PATCH] push: point to 'git pull' and 'git push --force' in case of non-fast forward

'git push' failing because of non-fast forward is a very common situation,
and a beginner does not necessarily understand "fast forward" immediately.

Add a new section to the git-push documentation and refer them to it.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com>
---

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

  > So how about phrasing it like this?
  >
  >     Non-fast forward pushes were rejected because you would discard remote
  >     changes you have not seen.  Integrate them with your changes and then
  >     push again. See 'non-fast forward' section of 'git push --help'.
  >
  > I think you can throw in a discussion on --force to the manual page, too.

  Here is my attempt to coagulate the improvements discussed on the thread so far. I added a paragraph that mentions --force in the new section of the manual, reworded the explanatory message on the UI, and forged two signatures.

 Documentation/git-push.txt |   86 ++++++++++++++++++++++++++++++++++++++++++++
 builtin-push.c             |    9 ++++-
 transport.c                |   10 ++++--
 transport.h                |    3 +-
 4 files changed, 103 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 2653388..58d2bd5 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -195,6 +195,92 @@ reason::
 	refs, no explanation is needed. For a failed ref, the reason for
 	failure is described.
 
+Note about fast-forwards
+------------------------
+
+When an update changes a branch (or more in general, a ref) that used to
+point at commit A to point at another commit B, it is called a
+fast-forward update if and only if B is a descendant of A.
+
+In a fast-forward update from A to B, the set of commits that the original
+commit A built on top of is a subset of the commits the new commit B
+builds on top of.  Hence, it does not lose any history.
+
+In contrast, a non-fast-forward update will lose history.  For example,
+suppose you and somebody else started at the same commit X, and you built
+a history leading to commit B while the other person built a history
+leading to commit A.  The history looks like this:
+
+----------------
+
+      B
+     /
+ ---X---A
+
+----------------
+
+Further suppose that the other person already pushed changes leading to A
+back to the original repository you two obtained the original commit X.
+
+The push done by the other person updated the branch that used to point at
+commit X to point at commit A.  It is a fast-forward.
+
+But if you try to push, you will attempt to update the branch (that
+now points at A) with commit B.  This does _not_ fast-forward.  If you did
+so, the changes introduced by commit A will be lost, because everybody
+will now start building on top of B.
+
+The command by default does not allow an update that is not a fast-forward
+to prevent such loss of history.
+
+If you do not want to lose your work (history from X to B) nor the work by
+the other person (history from X to A), you would need to first fetch the
+history from the repository, create a history that contains changes done
+by both parties, and push the result back.
+
+You can perform "git pull", resolve potential conflicts, and "git push"
+the result.  A "git pull" will create a merge commit C between commits A
+and B.
+
+----------------
+
+      B---C
+     /   /
+ ---X---A
+
+----------------
+
+Updating A with the resulting merge commit will fast-forward and your
+push will be accepted.
+
+Alternatively, you can rebase your change between X and B on top of A,
+with "git pull --rebase", and push the result back.  The rebase will
+create a new commit D that builds the change between X and B on top of
+A.
+
+----------------
+
+      B   D
+     /   /
+ ---X---A
+
+----------------
+
+Again, updating A with this commit will fast-forward and your push will be
+accepted.
+
+There is another common situation where you may encounter non-fast-forward
+rejection when you try to push, and it is possible even when you are
+pushing into a repository nobody else pushes into. After you push commit
+A yourself (in the first picture in this section), replace it with "git
+commit --amend" to produce commit B, and you try to push it out, because
+forgot that you have pushed A out already. In such a case, and only if
+you are certain that nobody in the meantime fetched your earlier commit A
+(and started building on top of it), you can run "git push --force" to
+overwrite it. In other words, "git push --force" is a method reserved for
+a case where you do mean to lose history.
+
+
 Examples
 --------
 
diff --git a/builtin-push.c b/builtin-push.c
index 1d92e22..50328f4 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -140,6 +140,7 @@ static int do_push(const char *repo, int flags)
 		struct transport *transport =
 			transport_get(remote, url[i]);
 		int err;
+		int nonfastforward;
 		if (receivepack)
 			transport_set_option(transport,
 					     TRANS_OPT_RECEIVEPACK, receivepack);
@@ -148,13 +149,19 @@ static int do_push(const char *repo, int flags)
 
 		if (flags & TRANSPORT_PUSH_VERBOSE)
 			fprintf(stderr, "Pushing to %s\n", url[i]);
-		err = transport_push(transport, refspec_nr, refspec, flags);
+		err = transport_push(transport, refspec_nr, refspec, flags,
+				     &nonfastforward);
 		err |= transport_disconnect(transport);
 
 		if (!err)
 			continue;
 
 		error("failed to push some refs to '%s'", url[i]);
+		if (nonfastforward) {
+			printf("To prevent you from losing history, non-fast-forward updates were rejected.\n"
+			       "Merge the remote changes before pushing again.\n"
+			       "See 'non-fast forward' section of 'git push --help' for details.\n");
+		}
 		errs++;
 	}
 	return !!errs;
diff --git a/transport.c b/transport.c
index de0d587..f231b35 100644
--- a/transport.c
+++ b/transport.c
@@ -820,7 +820,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 }
 
 static void print_push_status(const char *dest, struct ref *refs,
-							  int verbose, int porcelain)
+			      int verbose, int porcelain, int * nonfastforward)
 {
 	struct ref *ref;
 	int n = 0;
@@ -835,11 +835,14 @@ static void print_push_status(const char *dest, struct ref *refs,
 		if (ref->status == REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
 
+	*nonfastforward = 0;
 	for (ref = refs; ref; ref = ref->next) {
 		if (ref->status != REF_STATUS_NONE &&
 		    ref->status != REF_STATUS_UPTODATE &&
 		    ref->status != REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
+		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD)
+			*nonfastforward = 1;
 	}
 }
 
@@ -997,7 +1000,8 @@ int transport_set_option(struct transport *transport,
 }
 
 int transport_push(struct transport *transport,
-		   int refspec_nr, const char **refspec, int flags)
+		   int refspec_nr, const char **refspec, int flags,
+		   int * nonfastforward)
 {
 	verify_remote_names(refspec_nr, refspec);
 
@@ -1024,7 +1028,7 @@ int transport_push(struct transport *transport,
 
 		ret = transport->push_refs(transport, remote_refs, flags);
 
-		print_push_status(transport->url, remote_refs, verbose | porcelain, porcelain);
+		print_push_status(transport->url, remote_refs, verbose | porcelain, porcelain, nonfastforward);
 
 		if (!(flags & TRANSPORT_PUSH_DRY_RUN)) {
 			struct ref *ref;
diff --git a/transport.h b/transport.h
index 51b5397..639f13d 100644
--- a/transport.h
+++ b/transport.h
@@ -68,7 +68,8 @@ int transport_set_option(struct transport *transport, const char *name,
 			 const char *value);
 
 int transport_push(struct transport *connection,
-		   int refspec_nr, const char **refspec, int flags);
+		   int refspec_nr, const char **refspec, int flags,
+		   int * nonfastforward);
 
 const struct ref *transport_get_remote_refs(struct transport *transport);
 
-- 
1.6.2.GIT

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

[RFC/PATCH 0/4] make helpful messages optional

[Jeff King]

On Tue, Aug 11, 2009 at 12:03:13PM +0900, Nanako Shiraishi wrote:

>  		error("failed to push some refs to '%s'", url[i]);
> +		if (nonfastforward) {
> +			printf("To prevent you from losing history, non-fast-forward updates were rejected.\n"
> +			       "Merge the remote changes before pushing again.\n"
> +			       "See 'non-fast forward' section of 'git push --help' for details.\n");
> +		}

I saw this message in regular use for the first time today, and I
thought it was terribly ugly. The sheer number of lines, coupled with
the extremely ragged right edge made it much harder to pick out the
nicely formatted status table. And of course the information in the
message was totally worthless to me, as an experienced git user.

So rather than re-open the debate on whether this message should exist
or not, here is a patch series which tries to address the issue:

  [1/4]: push: fix english in non-fast-forward message

    Hopefully non-controversial.

  [2/4]: push: re-flow non-fast-forward message

    This makes it prettier, IMHO.  I suspect the message was flowed as
    it was originally on purpose so that each sentence began on its own
    line. I think making it easier on the eyes is more important,
    though.

  [3/4]: push: make non-fast-forward help message configurable

    I feel like we have had this exact sort of tension before: we want
    one thing to help new users and experienced users want another
    thing. We have resisted an "expert user" config variable in the past
    because proposals usually involved a change of behavior, which could
    lead to quite confusing results. However, I think the case of
    "helpful messages" is much simpler. The message is meant purely for
    human consumption and is redundant with information already given,
    so an expert sitting at a novice's terminal (or vice versa) will not
    encounter any surprises.

    This actually introduces infrastructure for other, similar messages,
    so that we can make existing ones optional, or build new ones as
    appropriate.

  [4/4]: status: make "how to stage" messages optional

    This uses the infrastructure in 3/4 to lose the "use git add to add
    untracked files" advice.

I think (1) and (2) are pretty straightforward. (3) and (4) are more
questionable, and I am undecided whether I am over-reacting to being
annoyed by the message. I do know this is not the first time I have had
the urge to write such a patch, so maybe others feel the same.

-Peff

[PATCH 1/4] push: fix english in non-fast-forward message

[Jeff King]

We must use an article when referring to the section
because it is a non-proper noun, and it must be the definite
article because we are referring to a specific section.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-push.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 67f6d96..8d72cc2 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -160,7 +160,7 @@ static int do_push(const char *repo, int flags)
 		if (nonfastforward) {
 			printf("To prevent you from losing history, non-fast-forward updates were rejected.\n"
 			       "Merge the remote changes before pushing again.\n"
-			       "See 'non-fast forward' section of 'git push --help' for details.\n");
+			       "See the 'non-fast forward' section of 'git push --help' for details.\n");
 		}
 		errs++;
 	}
-- 
1.6.4.2.266.g981efc.dirty

[PATCH 2/4] push: re-flow non-fast-forward message

[Jeff King]

The extreme raggedness of the right edge make this jarring
to read. Let's re-flow the text to fill the lines in a more
even way.

Signed-off-by: Jeff King <peff@peff.net>
---
I am actually not all that fond of the particular wording, either, but I
didn't want to re-open a wording bikeshed debate.

 builtin-push.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 8d72cc2..787011f 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -158,9 +158,9 @@ static int do_push(const char *repo, int flags)
 
 		error("failed to push some refs to '%s'", url[i]);
 		if (nonfastforward) {
-			printf("To prevent you from losing history, non-fast-forward updates were rejected.\n"
-			       "Merge the remote changes before pushing again.\n"
-			       "See the 'non-fast forward' section of 'git push --help' for details.\n");
+			printf("To prevent you from losing history, non-fast-forward updates were rejected\n"
+			       "Merge the remote changes before pushing again.  See the 'non-fast forward'\n"
+			       "section of 'git push --help' for details.\n");
 		}
 		errs++;
 	}
-- 
1.6.4.2.266.g981efc.dirty

[PATCH 3/4] push: make non-fast-forward help message configurable

[Jeff King]

This message is designed to help new users understand what
has happened when refs fail to push. However, it does not
help experienced users at all, and significantly clutters
the output, frequently dwarfing the regular status table and
making it harder to see.

This patch introduces a general configuration mechanism for
optional messages, with this push message as the first
example.

Signed-off-by: Jeff King <peff@peff.net>
---
Probably "messages" is too vague a term to use in the code and config.
Maybe "advice.*"?

 Documentation/config.txt |   16 ++++++++++++++++
 Makefile                 |    2 ++
 builtin-push.c           |    3 ++-
 cache.h                  |    1 +
 config.c                 |    3 +++
 messages.c               |   28 ++++++++++++++++++++++++++++
 messages.h               |   15 +++++++++++++++
 7 files changed, 67 insertions(+), 1 deletions(-)
 create mode 100644 messages.c
 create mode 100644 messages.h

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5256c7f..ec308a6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1201,6 +1201,22 @@ mergetool.keepTemporaries::
 mergetool.prompt::
 	Prompt before each invocation of the merge resolution program.
 
+message.all::
+	When set to 'true', display all optional help messages. When set
+	to 'false', do not display any. Defaults vary with individual
+	messages (see message.* below).
+
+message.*::
+	When set to 'true', display the given optional help message.
+	When set to 'false', do not display. The configuration variables
+	are:
++
+--
+	pushNonFastForward::
+		Help message shown when linkgit:git-push[1] refuses
+		non-fast-forward refs. Default: true.
+--
+
 pack.window::
 	The size of the window used by linkgit:git-pack-objects[1] when no
 	window size is given on the command line. Defaults to 10.
diff --git a/Makefile b/Makefile
index a614347..0785977 100644
--- a/Makefile
+++ b/Makefile
@@ -424,6 +424,7 @@ LIB_H += ll-merge.h
 LIB_H += log-tree.h
 LIB_H += mailmap.h
 LIB_H += merge-recursive.h
+LIB_H += messages.h
 LIB_H += object.h
 LIB_H += pack.h
 LIB_H += pack-refs.h
@@ -506,6 +507,7 @@ LIB_OBJS += mailmap.o
 LIB_OBJS += match-trees.o
 LIB_OBJS += merge-file.o
 LIB_OBJS += merge-recursive.o
+LIB_OBJS += messages.o
 LIB_OBJS += name-hash.o
 LIB_OBJS += object.o
 LIB_OBJS += pack-check.o
diff --git a/builtin-push.c b/builtin-push.c
index 787011f..dceac9f 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -157,7 +157,8 @@ static int do_push(const char *repo, int flags)
 			continue;
 
 		error("failed to push some refs to '%s'", url[i]);
-		if (nonfastforward) {
+		if (nonfastforward &&
+		    messages[MESSAGE_PUSH_NONFASTFORWARD].preference) {
 			printf("To prevent you from losing history, non-fast-forward updates were rejected\n"
 			       "Merge the remote changes before pushing again.  See the 'non-fast forward'\n"
 			       "section of 'git push --help' for details.\n");
diff --git a/cache.h b/cache.h
index 5fad24c..32d1a27 100644
--- a/cache.h
+++ b/cache.h
@@ -4,6 +4,7 @@
 #include "git-compat-util.h"
 #include "strbuf.h"
 #include "hash.h"
+#include "messages.h"
 
 #include SHA1_HEADER
 #ifndef git_SHA_CTX
diff --git a/config.c b/config.c
index e87edea..aed1547 100644
--- a/config.c
+++ b/config.c
@@ -627,6 +627,9 @@ int git_default_config(const char *var, const char *value, void *dummy)
 	if (!prefixcmp(var, "mailmap."))
 		return git_default_mailmap_config(var, value);
 
+	if (!prefixcmp(var, "message."))
+		return git_default_message_config(var, value);
+
 	if (!strcmp(var, "pager.color") || !strcmp(var, "color.pager")) {
 		pager_use_color = git_config_bool(var,value);
 		return 0;
diff --git a/messages.c b/messages.c
new file mode 100644
index 0000000..00fc196
--- /dev/null
+++ b/messages.c
@@ -0,0 +1,28 @@
+#include "cache.h"
+#include "messages.h"
+
+struct message_preference messages[] = {
+	{ "pushnonfastforward", 1 },
+};
+
+int git_default_message_config(const char *var, const char *value)
+{
+	const char *k = skip_prefix(var, "message.");
+	int i;
+
+	if (!strcmp(k, "all")) {
+		int v = git_config_bool(var, value);
+		for (i = 0; i < ARRAY_SIZE(messages); i++)
+			messages[i].preference = v;
+		return 0;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(messages); i++) {
+		if (strcmp(k, messages[i].name))
+			continue;
+		messages[i].preference = git_config_bool(var, value);
+		return 0;
+	}
+
+	return 0;
+}
diff --git a/messages.h b/messages.h
new file mode 100644
index 0000000..f175747
--- /dev/null
+++ b/messages.h
@@ -0,0 +1,15 @@
+#ifndef MESSAGE_H
+#define MESSAGE_H
+
+#define MESSAGE_PUSH_NONFASTFORWARD 0
+
+struct message_preference {
+	const char *name;
+	int preference;
+};
+
+extern struct message_preference messages[];
+
+int git_default_message_config(const char *var, const char *value);
+
+#endif /* MESSAGE_H */
-- 
1.6.4.2.266.g981efc.dirty

Re: [PATCH 3/4] push: make non-fast-forward help message configurable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Probably "messages" is too vague a term to use in the code and config.
> Maybe "advice.*"?

I hate to start off with a digression, but one expression from Japanese I
have long been frustrated with, because I cannot find a good counterpart
in English, is "大きなお世話".

J-E dictionaries often translate this phrase as "none of your business",
and it indeed is meant to be thrown at somebody who gives an unsolicited
and an unwanted advice to you, but at the same time it strongly implies
that the reason why such an advice is unwanted is because you very well
knows the issue and does not _need_ (not want) the help on the topic at
all.  It is not about _who_ gives the unwanted advice (which I think the
expression "none of YOUR business" talks about---if the same advice came
from somebody else, it might be appreciated), but it is all about on what
topic the advice is about, and you feel that you know about the topic very
well yourself and do not need any advice from anybody.

Enough digression.

I have always felt that many of the messages we have added since the
"newbie friendliness" drive around 1.3.0 deserve to be labeled with the
expression 大きなお世話.

Of course, "unsolicited-unneeded-advice.*" is too long as a variable name,
but I personally would very much welcome changes along this line and I
think "advice.*" is a good name for the category.

Re: [PATCH 3/4] push: make non-fast-forward help message configurable

[Jeff King]

On Sun, Sep 06, 2009 at 12:09:07AM -0700, Junio C Hamano wrote:

> I hate to start off with a digression, but one expression from Japanese I
> have long been frustrated with, because I cannot find a good counterpart
> in English, is "大きなお世話".
> 
> J-E dictionaries often translate this phrase as "none of your business",
> and it indeed is meant to be thrown at somebody who gives an unsolicited
> and an unwanted advice to you, but at the same time it strongly implies
> that the reason why such an advice is unwanted is because you very well
> knows the issue and does not _need_ (not want) the help on the topic at
> all.  It is not about _who_ gives the unwanted advice (which I think the
> expression "none of YOUR business" talks about---if the same advice came
> from somebody else, it might be appreciated), but it is all about on what
> topic the advice is about, and you feel that you know about the topic very
> well yourself and do not need any advice from anybody.

The best related English phrase I can think of is that the person is a
"back seat driver". The phrase "armchair $X" also comes to mind (e.g.,
"armchair critic"), though that is usually to indicate that the armchair
critic is giving _bad_ advice, not being well-versed in the field they
are advising about. And I think you are not so much saying the advice is
wrong as much as it is unwanted because you already know it.

I can't think of a noun that describes the advice itself, though
(nagging? ;) ).

> I have always felt that many of the messages we have added since the
> "newbie friendliness" drive around 1.3.0 deserve to be labeled with the
> expression 大きなお世話.
> 
> Of course, "unsolicited-unneeded-advice.*" is too long as a variable name,
> but I personally would very much welcome changes along this line and I
> think "advice.*" is a good name for the category.

Git config files are utf8, right? You can always internationalize this
feature. :)

Thinking on it more, I think "advice" is the right word. It is not about
arbitrary messages; it is about particular messages which try to advise.
You would never want this feature to cover messages that are
informational about a particular state or action that has occurred. Only
"maybe you should try this" messages.

I'll re-roll 3 and 4 based on that, but I will wait a bit to see any
more comments. Probably you should consider patches 1 and 2 as a
potential series for 'maint', and 3 and 4 should be spun off into their
own series (they really only rely textually on the first two).

-Peff

Re: [PATCH 3/4] push: make non-fast-forward help message configurable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'll re-roll 3 and 4 based on that, but I will wait a bit to see any
> more comments. Probably you should consider patches 1 and 2 as a
> potential series for 'maint', and 3 and 4 should be spun off into their
> own series (they really only rely textually on the first two).

Yeah, thanks.

And no need to hurry.  I've decided to queue 1 and 2 to 'master' (was
Nana's reroll of Matthieu's patch on 'maint'???) and 3 and 4 merged to
'pu', but I seem to be having a problem ssh'ing into k.org, and it appears
that no pushout will happen tonight.

Re: [PATCH 3/4] push: make non-fast-forward help message configurable

[Jeff King]

On Sun, Sep 06, 2009 at 12:30:14AM -0700, Junio C Hamano wrote:

> And no need to hurry.  I've decided to queue 1 and 2 to 'master' (was
> Nana's reroll of Matthieu's patch on 'maint'???) and 3 and 4 merged to

It was on 'maint', which surprised me, too. But v1.6.4.1 got it.

-Peff

Re: [PATCH 3/4] push: make non-fast-forward help message configurable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Thinking on it more, I think "advice" is the right word. It is not about
> arbitrary messages; it is about particular messages which try to advise.
> You would never want this feature to cover messages that are
> informational about a particular state or action that has occurred. Only
> "maybe you should try this" messages.

Speaking of which, has anybody felt annoyed by this message?

    $ git reset --hard HEAD^^
    HEAD is now at 3fb9d58 Do not scramble password read from .cvspass

This is not "maybe you should try this", but I would consider that it
falls into the same "I see you are trying to be helpful, but I know what I
am doing, and you are stealing screen real estate from me without helping
me at all, thank you very much" category.

Besides, if you want to use "where you are" to your next command, you can
just say HEAD without knowing exactly where you are.

It might be slightly more useful if the message said this instead:

    $ git reset --hard HEAD^^
    HEAD was at d1fe49f push: re-flow non-fast-forward message

Resetting is done because I want to build an alternate history starting
from an earlier point, and when I am done, I may want to feed this to "git
diff" or whatever to sanity check the result, without having to go through
the reflog.

Re: [PATCH 3/4] push: make non-fast-forward help message configurable

[Sverre Rabbelier]

Heya,

On Sun, Sep 6, 2009 at 09:52, Junio C Hamano<gitster@pobox.com> wrote:
> Resetting is done because I want to build an alternate history starting
> from an earlier point, and when I am done, I may want to feed this to "git
> diff" or whatever to sanity check the result, without having to go through
> the reflog.

I agree, the 'you are now at ...' is not very helpful, I just TOLD you
where I want to go to! However, 'you were at ....' is also not
optimal, since what I really want to know is where I was at "plus any
old data I had" (going back to the 'git reset --hard' should create a
snapshot of the current content before doing it's resetting'). I would
definitely prefer 'you were at' over 'you are now at' though.

WRT the 'none of your business', it sounds like 'I [already] know
[that]' might be approriate?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 3/4] push: make non-fast-forward help message configurable

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com>

> Speaking of which, has anybody felt annoyed by this message?
>
>     $ git reset --hard HEAD^^
>     HEAD is now at 3fb9d58 Do not scramble password read from .cvspass
>
> This is not "maybe you should try this", but I would consider that it
> falls into the same "I see you are trying to be helpful, but I know what I
> am doing, and you are stealing screen real estate from me without helping
> me at all, thank you very much" category.

You may be fixated at the sha1 part of the message when you find this message annoying, but I disagree strongly. I always appreciate the assurance this message gives me that I counted the number of commits correctly, whether I say HEAD^^^^ or HEAD~7.

Showing the subject of the commit you are now at is very useful and I will be equally irritated as you do if it starts showing the subject of the commit I was at.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH 3/4] push: make non-fast-forward help message configurable

[Johannes Sixt]

Nanako Shiraishi schrieb:
> Quoting Junio C Hamano <gitster@pobox.com>
> 
>> Speaking of which, has anybody felt annoyed by this message?
>>
>>     $ git reset --hard HEAD^^
>>     HEAD is now at 3fb9d58 Do not scramble password read from .cvspass
>>
>> This is not "maybe you should try this", but I would consider that it
>> falls into the same "I see you are trying to be helpful, but I know what I
>> am doing, and you are stealing screen real estate from me without helping
>> me at all, thank you very much" category.
> 
> You may be fixated at the sha1 part of the message when you find this
> message annoying, but I disagree strongly. I always appreciate the assurance
> this message gives me that I counted the number of commits correctly,
> whether I say HEAD^^^^ or HEAD~7.
> 
> Showing the subject of the commit you are now at is very useful and I will
> be equally irritated as you do if it starts showing the subject of the
> commit I was at.

I agree 100% with your reasoning.

-- Hannes

Re: [PATCH 3/4] push: make non-fast-forward help message configurable

[Mike Hommey]

On Mon, Sep 07, 2009 at 09:44:57AM +0900, Nanako Shiraishi wrote:
> Quoting Junio C Hamano <gitster@pobox.com>
> 
> > Speaking of which, has anybody felt annoyed by this message?
> >
> >     $ git reset --hard HEAD^^
> >     HEAD is now at 3fb9d58 Do not scramble password read from .cvspass
> >
> > This is not "maybe you should try this", but I would consider that it
> > falls into the same "I see you are trying to be helpful, but I know what I
> > am doing, and you are stealing screen real estate from me without helping
> > me at all, thank you very much" category.
> 
> You may be fixated at the sha1 part of the message when you find this message annoying, but I disagree strongly. I always appreciate the assurance this message gives me that I counted the number of commits correctly, whether I say HEAD^^^^ or HEAD~7.
> 
> Showing the subject of the commit you are now at is very useful and I will be equally irritated as you do if it starts showing the subject of the commit I was at.

I'd say both are equally interesting information.

Mike

Re: [PATCH 3/4] push: make non-fast-forward help message configurable

[Jeff King]

On Mon, Sep 07, 2009 at 09:44:57AM +0900, Nanako Shiraishi wrote:

> > Speaking of which, has anybody felt annoyed by this message?
> >
> >     $ git reset --hard HEAD^^
> >     HEAD is now at 3fb9d58 Do not scramble password read from .cvspass
> >
> > This is not "maybe you should try this", but I would consider that it
> > falls into the same "I see you are trying to be helpful, but I know what I
> > am doing, and you are stealing screen real estate from me without helping
> > me at all, thank you very much" category.
> 
> You may be fixated at the sha1 part of the message when you find this
> message annoying, but I disagree strongly. I always appreciate the
> assurance this message gives me that I counted the number of commits
> correctly, whether I say HEAD^^^^ or HEAD~7.

Let me add a "me too" to Nanako's comments. This assurance has actually
saved me in the past from accidentally going to the wrong commit (just
the other day I did a rebase followed by "git reset --hard HEAD@{1}",
when of course what I meant was "git reset --hard master@{1}".

I think this type of message is different from the other "advice"
messages.

In the case of the push non-fast-forward message and the status "here is
how you stage" comments, those messages are not specific to this exact
situation. They are general advice for "if you do not understand or need
a reminder of how git works, this is it." Experienced users know how git
works, so the messages are just clutter.

This message, on the other hand, tells you about this _specific_
instance. So even if you have mastered git, the information can reassure
you that you have gone to the intended commit (and yes, I have actually
gone to the wrong commit before, noticed it via this reset message, and
corrected the situation).

So really they are two different conceptual types of message. And while
I have no problem with an argument of "I _personally_ find this clutter
and would like to configure it off", I don't think such an option should
go under "advice.*". My patch had "message.all" (which will become
"advice.all") to turn off all advice messages, which can act as a sort
of "I am an expert" switch. But because this type of message is
conceptually different, it should not be lumped in with the others.

OTOH, I am open to arguments against "advice.all"; maybe it is a good
thing for users to manually say "this message is annoying me, and
therefore I am now an expert in this particular area". It's not like
there are more than two. ;)

-Peff

Re: [PATCH 3/4] push: make non-fast-forward help message configurable

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> On Mon, Sep 07, 2009 at 09:44:57AM +0900, Nanako Shiraishi wrote:
>
>> You may be fixated at the sha1 part of the message when you find this
>> message annoying, but I disagree strongly. I always appreciate the
>> assurance this message gives me that I counted the number of commits
>> correctly, whether I say HEAD^^^^ or HEAD~7.
>
> Let me add a "me too" to Nanako's comments.

I guess it depends on one's workflow. If you usually cut-and-paste
sha1's, then the message is superfluous, while if you usually use
magic revspec, it is useful.

So, it's probably a good idea to make this configurable.

> So really they are two different conceptual types of message. And while
> I have no problem with an argument of "I _personally_ find this clutter
> and would like to configure it off", I don't think such an option should
> go under "advice.*". My patch had "message.all" (which will become
> "advice.all")

To me, this is an argument in favor of keeping "message", to allow the
same mechanism for these different types of messages.

But I think the individual message.* should not be just true/false
switch, but could be always/auto/never :

- always: show the message, regardless of message.all
- auto (the default): rely on message.all to decide whether to show
  the message
- never: never show it.

So you could say "message.all = false" and "message.resetShowsNewHead
= always".

But maybe that's just overkill, dunno...

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

Re: [PATCH 3/4] push: make non-fast-forward help message configurable

[Jeff King]

On Mon, Sep 07, 2009 at 10:34:25AM +0200, Matthieu Moy wrote:

> > So really they are two different conceptual types of message. And while
> > I have no problem with an argument of "I _personally_ find this clutter
> > and would like to configure it off", I don't think such an option should
> > go under "advice.*". My patch had "message.all" (which will become
> > "advice.all")
> 
> To me, this is an argument in favor of keeping "message", to allow the
> same mechanism for these different types of messages.
> 
> But I think the individual message.* should not be just true/false
> switch, but could be always/auto/never :
> 
> - always: show the message, regardless of message.all
> - auto (the default): rely on message.all to decide whether to show
>   the message
> - never: never show it.
> 
> So you could say "message.all = false" and "message.resetShowsNewHead
> = always".
> 
> But maybe that's just overkill, dunno...

I'm not sure it solves the problem. The point of "message.all" was to
easily say "I'm an expert, so turn off useless advice". But now I would
have to manually re-enable any messages that I _do_ want to see. And of
course I don't see them to know that I want them, so I have to read
through the config documentation and decide on each one.

At that point, why not just get rid of "message.all" and simply say
"manually turn off the messages you don't like". Then the user can
either go through the config manually as above, or they can wait until
they become annoyed with a particular message and turn it off (and
hopefully our naming is good enough that they can easily figure out
which one it was :) ).

So I think "be verbose, but let the user quiet us" is probably
better than "be quiet, but let the user make us louder", because it is
easier to discover verbose things. Which implies to me that
"message.all", if it exists at all, should be limited in scope to just
advice.

In fact, you could mix many types in the message.* hierarchy and simply
call the umbrella variable message.advice. But that is semantically
equivalent to having advice.{all,*} and other_type_of_message.{all,*}.

-Peff

Re: [PATCH 3/4] push: make non-fast-forward help message configurable

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> I'm not sure it solves the problem. The point of "message.all" was to
> easily say "I'm an expert, so turn off useless advice". But now I would
> have to manually re-enable any messages that I _do_ want to see. And of
> course I don't see them to know that I want them, so I have to read
> through the config documentation and decide on each one.

Well, if it was _that_ important, I'd go for your suggestion of a
message hierarchy message.advice.foo, message.info.bar and so, with
the possibility of enabling/disabling a subhierarchy with a config
option. Now, I really get the feeling that this is overkill...

> So I think "be verbose, but let the user quiet us" is probably
> better than "be quiet, but let the user make us louder", because it is
> easier to discover verbose things. Which implies to me that
> "message.all", if it exists at all, should be limited in scope to just
> advice.

Yup, you convinced me for the last implication.

Otherwise, one setting "message.all = false" would never even notice
that another very cool informative message was added to Git in its
latest version.

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

Re: [PATCH 3/4] push: make non-fast-forward help message configurable

[Uri Okrent]

On 09/07/2009 01:54 AM, Jeff King wrote:
> At that point, why not just get rid of "message.all" and simply say
> "manually turn off the messages you don't like". Then the user can
> either go through the config manually as above, or they can wait until
> they become annoyed with a particular message and turn it off (and
> hopefully our naming is good enough that they can easily figure out
> which one it was :) ).
>
> So I think "be verbose, but let the user quiet us" is probably
> better than "be quiet, but let the user make us louder", because it is
> easier to discover verbose things. Which implies to me that
> "message.all", if it exists at all, should be limited in scope to just
> advice.

That seems like the most sane solution, given that it may not be obvious 
to the user what messages he/she may be missing if "expert" mode is on, 
and also from a coding perspective.

I know that by default I'd like to see new messages, and in case I'm 
doing something adventurous I'd like to see a message I may not have 
seen before. If I had turned off all messages because I got sick of 
seeing one or two in particular, then I'd never know.

Also, this thread started due to people's dislike of one particular 
message, so I think it's likely that in general someone would really 
only want to turn off at most a handful of messages, which again points 
to turning them off individually as being the best and simplest solution.

P.S. I never really introduced myself to the list... Uri Okrent here 
from L.A. Keep up the great work everyone!
-- 
    Uri

Please consider the environment before printing this message.
http://www.panda.org/how_you_can_help/

Re: [PATCH 3/4] push: make non-fast-forward help message configurable

[Jeff King]

On Tue, Sep 08, 2009 at 11:51:21AM -0700, Uri Okrent wrote:

> I know that by default I'd like to see new messages, and in case I'm
> doing something adventurous I'd like to see a message I may not have
> seen before. If I had turned off all messages because I got sick of
> seeing one or two in particular, then I'd never know.

Yeah, I think the 'advice.all' option, while clever, is not really
helping anyone. In my revised series, I am just omitting entirely (but
the code is still structured that adding it later would be very easy if
we change our minds).

> P.S. I never really introduced myself to the list... Uri Okrent here
> from L.A. Keep up the great work everyone!

Welcome. :)

-Peff

[PATCH 0/2] configurable advice messages

[Jeff King]

On Sun, Sep 06, 2009 at 03:23:22AM -0400, Jeff King wrote:

> I'll re-roll 3 and 4 based on that, but I will wait a bit to see any
> more comments. Probably you should consider patches 1 and 2 as a
> potential series for 'maint', and 3 and 4 should be spun off into their
> own series (they really only rely textually on the first two).

Looks like you applied 1 and 2 already, so here is the re-roll of the
latter two patches incorporating the changes that have been discussed.

  [1/2]: push: make non-fast-forward help message configurable
  [2/2]: status: make "how to stage" messages optional

-Peff

[PATCH 1/2] push: make non-fast-forward help message configurable

[Jeff King]

This message is designed to help new users understand what
has happened when refs fail to push. However, it does not
help experienced users at all, and significantly clutters
the output, frequently dwarfing the regular status table and
making it harder to see.

This patch introduces a general configuration mechanism for
optional messages, with this push message as the first
example.

Signed-off-by: Jeff King <peff@peff.net>
---
Changes from the original version:

 - s/message/advice/ in the config and filenames.

 - no more advice.all config option. The point of this is to shut up
   messages that annoy you; It's probably best to let users see a
   message, dislike it, and then turn it off manually after making a
   decision that they don't need or want to see it. But adding a "never
   show me any advice" option means they will never see some of the
   messages, which might otherwise be helpful.

   My thinking here is influenced by the fact that we are usually pretty
   conservative about adding messages in the first place. If we suddenly
   had a period of adding a bunch of "you are clueless, and here is how
   git works" messages, enabled by default, then I might reconsider.

 - I re-worked the code to give each preference its own variable. This
   means we can avoid maintaining a separate list of "#define
   MESSAGE_ONE 1" in addition to the array. It also means that checking
   a preference is a little nicer. Instead of:

     if (advice[ADVICE_PUSH_NONFASTFORWARD].preference)

   you get:

     if (advice_push_nonfastforward)

 Documentation/config.txt |   11 +++++++++++
 Makefile                 |    2 ++
 advice.c                 |   25 +++++++++++++++++++++++++
 advice.h                 |    8 ++++++++
 builtin-push.c           |    2 +-
 cache.h                  |    1 +
 config.c                 |    3 +++
 7 files changed, 51 insertions(+), 1 deletions(-)
 create mode 100644 advice.c
 create mode 100644 advice.h

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5256c7f..a35b918 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -113,6 +113,17 @@ For command-specific variables, you will find a more detailed description
 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:
++
+--
+	pushNonFastForward::
+		Advice shown when linkgit:git-push[1] refuses
+		non-fast-forward refs. Default: true.
+--
+
 core.fileMode::
 	If false, the executable bit differences between the index and
 	the working copy are ignored; useful on broken filesystems like FAT.
diff --git a/Makefile b/Makefile
index a614347..9d9ff45 100644
--- a/Makefile
+++ b/Makefile
@@ -397,6 +397,7 @@ export PERL_PATH
 LIB_FILE=libgit.a
 XDIFF_LIB=xdiff/lib.a
 
+LIB_H += advice.h
 LIB_H += archive.h
 LIB_H += attr.h
 LIB_H += blob.h
@@ -454,6 +455,7 @@ LIB_H += utf8.h
 LIB_H += wt-status.h
 
 LIB_OBJS += abspath.o
+LIB_OBJS += advice.o
 LIB_OBJS += alias.o
 LIB_OBJS += alloc.o
 LIB_OBJS += archive.o
diff --git a/advice.c b/advice.c
new file mode 100644
index 0000000..b5216a2
--- /dev/null
+++ b/advice.c
@@ -0,0 +1,25 @@
+#include "cache.h"
+
+int advice_push_nonfastforward = 1;
+
+static struct {
+	const char *name;
+	int *preference;
+} advice_config[] = {
+	{ "pushnonfastforward", &advice_push_nonfastforward },
+};
+
+int git_default_advice_config(const char *var, const char *value)
+{
+	const char *k = skip_prefix(var, "advice.");
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(advice_config); i++) {
+		if (strcmp(k, advice_config[i].name))
+			continue;
+		*advice_config[i].preference = git_config_bool(var, value);
+		return 0;
+	}
+
+	return 0;
+}
diff --git a/advice.h b/advice.h
new file mode 100644
index 0000000..862bae3
--- /dev/null
+++ b/advice.h
@@ -0,0 +1,8 @@
+#ifndef ADVICE_H
+#define ADVICE_H
+
+extern int advice_push_nonfastforward;
+
+int git_default_advice_config(const char *var, const char *value);
+
+#endif /* ADVICE_H */
diff --git a/builtin-push.c b/builtin-push.c
index 787011f..6eda372 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -157,7 +157,7 @@ static int do_push(const char *repo, int flags)
 			continue;
 
 		error("failed to push some refs to '%s'", url[i]);
-		if (nonfastforward) {
+		if (nonfastforward && advice_push_nonfastforward) {
 			printf("To prevent you from losing history, non-fast-forward updates were rejected\n"
 			       "Merge the remote changes before pushing again.  See the 'non-fast forward'\n"
 			       "section of 'git push --help' for details.\n");
diff --git a/cache.h b/cache.h
index 5fad24c..e1ab092 100644
--- a/cache.h
+++ b/cache.h
@@ -4,6 +4,7 @@
 #include "git-compat-util.h"
 #include "strbuf.h"
 #include "hash.h"
+#include "advice.h"
 
 #include SHA1_HEADER
 #ifndef git_SHA_CTX
diff --git a/config.c b/config.c
index e87edea..f21530c 100644
--- a/config.c
+++ b/config.c
@@ -627,6 +627,9 @@ int git_default_config(const char *var, const char *value, void *dummy)
 	if (!prefixcmp(var, "mailmap."))
 		return git_default_mailmap_config(var, value);
 
+	if (!prefixcmp(var, "advice."))
+		return git_default_advice_config(var, value);
+
 	if (!strcmp(var, "pager.color") || !strcmp(var, "color.pager")) {
 		pager_use_color = git_config_bool(var,value);
 		return 0;
-- 
1.6.5.rc0.173.g0bfef

Re: [PATCH 1/2] push: make non-fast-forward help message configurable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/advice.c b/advice.c
> new file mode 100644
> index 0000000..b5216a2
> --- /dev/null
> +++ b/advice.c
> @@ -0,0 +1,25 @@
> +#include "cache.h"
> +
> +int advice_push_nonfastforward = 1;
> +
> +static struct {
> +	const char *name;
> +	int *preference;
> +} advice_config[] = {
> +	{ "pushnonfastforward", &advice_push_nonfastforward },
> +};

Can we have the value inside this struct, instead of having a pointer
to another variable, and get rid of that variable altogether?

> diff --git a/builtin-push.c b/builtin-push.c
> index 787011f..6eda372 100644
> --- a/builtin-push.c
> +++ b/builtin-push.c
> @@ -157,7 +157,7 @@ static int do_push(const char *repo, int flags)
>  			continue;
>  
>  		error("failed to push some refs to '%s'", url[i]);
> -		if (nonfastforward) {
> +		if (nonfastforward && advice_push_nonfastforward) {

If we did so, this part needs to become

	if (nonfastforward && check_advice("pushnonfastforward")) {

which would be less efficient, but by definition advices are on the slow
path, right?

And check_advice() implementation can find programming errors by barfing
when the given string token does not exist in the table.

Re: [PATCH 1/2] push: make non-fast-forward help message configurable

[Jeff King]

On Wed, Sep 09, 2009 at 12:06:21PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > diff --git a/advice.c b/advice.c
> > new file mode 100644
> > index 0000000..b5216a2
> > --- /dev/null
> > +++ b/advice.c
> > @@ -0,0 +1,25 @@
> > +#include "cache.h"
> > +
> > +int advice_push_nonfastforward = 1;
> > +
> > +static struct {
> > +	const char *name;
> > +	int *preference;
> > +} advice_config[] = {
> > +	{ "pushnonfastforward", &advice_push_nonfastforward },
> > +};
> 
> Can we have the value inside this struct, instead of having a pointer
> to another variable, and get rid of that variable altogether?

We could, but then callers need some way of indexing into the array.
Which means either:

  - a constant offset (like "#define ADVICE_PUSH_NONFASTFORWARD 0"). The
    problem with that is you get no compile-time support for making sure
    that your index matches the variable you want. I.e., you have:

      { "pushnonfastforward", 1 } /* ADVICE_PUSH_NONFASTFORWARD */

    with the position in the array implicitly matching the manually
    assigned numbers.  We do have precedent in things like diff_colors.
    I don't remember if we have ever screwed it up.

  - a dynamic offset, like (as you noted):

> If we did so, this part needs to become
> 
> 	if (nonfastforward && check_advice("pushnonfastforward")) {
> 
> which would be less efficient, but by definition advices are on the slow
> path, right?

which, as you note, is less efficient. It also turns typo-checking into
a run-time error rather than a compile-time error, which is IMHO a bad
idea. And if you care about such things, it is worse for using something
like ctags to find variable uses.

I went the way I did because it provides compile-time checking, and
because the variable is referred to by name in the table, the matching
is explicit and thus harder to screw up.

One final option would be to get rid of the table altogether. Its
function is to allow you to iterate over all of the variables. Now that
the "advice.all" option has been dropped, the only use is during config
parsing. We could simply unroll that loop to:

  if (!strcmp(k, "pushnonfastforward")) {
          advice_push_nonfastforward = git_config_bool(var, value);
          return 0;
  }
  if (!strcmp(k, "statushints")) {
          advice_status_hints = git_config_bool(var, value);
          return 0;
  }

as we do in other config parsing.

-Peff

Re: [PATCH 1/2] push: make non-fast-forward help message configurable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... It also turns typo-checking into
> a run-time error rather than a compile-time error, which is IMHO a bad
> idea. And if you care about such things, it is worse for using something
> like ctags to find variable uses.

Fair enough.

[PATCH 2/2] status: make "how to stage" messages optional

[Jeff King]

These messages are nice for new users, but experienced git
users know how to manipulate the index, and these messages
waste a lot of screen real estate.

Signed-off-by: Jeff King <peff@peff.net>
---
Most changes are just from rebasing on the prior patch.

Since advice.statusAdvice was a bit redundant, I switched it to
advice.statusHints. Just advice.status was a bit too vague for my taste,
especially as we might want something like advice.statusCurrentBranch or
something later. statusHints is a little vague, too, so I'm open to
suggestions.

This version retains the "compact" output of the last version. I.e., no
newline between header and files, as there would be with the hints.
Like:

  # Changed but not updated:
          modified: foo

I tried looking at it both with and without the newline, and didn't feel
strongly. My reasoning was that people turning off the hints are
probably interested in compactness. Suggestions welcome.

 Documentation/config.txt |    4 ++++
 advice.c                 |    2 ++
 advice.h                 |    1 +
 wt-status.c              |    8 ++++++++
 4 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a35b918..8cbabe8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -122,6 +122,10 @@ advice.*::
 	pushNonFastForward::
 		Advice shown when linkgit:git-push[1] refuses
 		non-fast-forward refs. Default: true.
+	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.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index b5216a2..ae4b1e8 100644
--- a/advice.c
+++ b/advice.c
@@ -1,12 +1,14 @@
 #include "cache.h"
 
 int advice_push_nonfastforward = 1;
+int advice_status_hints = 1;
 
 static struct {
 	const char *name;
 	int *preference;
 } advice_config[] = {
 	{ "pushnonfastforward", &advice_push_nonfastforward },
+	{ "statushints", &advice_status_hints },
 };
 
 int git_default_advice_config(const char *var, const char *value)
diff --git a/advice.h b/advice.h
index 862bae3..e9df8e0 100644
--- a/advice.h
+++ b/advice.h
@@ -2,6 +2,7 @@
 #define ADVICE_H
 
 extern int advice_push_nonfastforward;
+extern int advice_status_hints;
 
 int git_default_advice_config(const char *var, const char *value);
 
diff --git a/wt-status.c b/wt-status.c
index 85f3fcb..38eb245 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -48,6 +48,8 @@ static void wt_status_print_unmerged_header(struct wt_status *s)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
 	color_fprintf_ln(s->fp, c, "# Unmerged paths:");
+	if (!advice_status_hints)
+		return;
 	if (!s->is_initial)
 		color_fprintf_ln(s->fp, c, "#   (use \"git reset %s <file>...\" to unstage)", s->reference);
 	else
@@ -60,6 +62,8 @@ static void wt_status_print_cached_header(struct wt_status *s)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
 	color_fprintf_ln(s->fp, c, "# Changes to be committed:");
+	if (!advice_status_hints)
+		return;
 	if (!s->is_initial) {
 		color_fprintf_ln(s->fp, c, "#   (use \"git reset %s <file>...\" to unstage)", s->reference);
 	} else {
@@ -73,6 +77,8 @@ static void wt_status_print_dirty_header(struct wt_status *s,
 {
 	const char *c = color(WT_STATUS_HEADER, s);
 	color_fprintf_ln(s->fp, c, "# Changed but not updated:");
+	if (!advice_status_hints)
+		return;
 	if (!has_deleted)
 		color_fprintf_ln(s->fp, c, "#   (use \"git add <file>...\" to update what will be committed)");
 	else
@@ -85,6 +91,8 @@ static void wt_status_print_untracked_header(struct wt_status *s)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
 	color_fprintf_ln(s->fp, c, "# Untracked files:");
+	if (!advice_status_hints)
+		return;
 	color_fprintf_ln(s->fp, c, "#   (use \"git add <file>...\" to include in what will be committed)");
 	color_fprintf_ln(s->fp, c, "#");
 }
-- 
1.6.5.rc0.173.g0bfef

[PATCH 4/4] status: make "how to stage" messages optional

[Jeff King]

These messages are nice for new users, but experienced git
users know how to manipulate the index, and these messages
waste a lot of screen real estate.

Signed-off-by: Jeff King <peff@peff.net>
---
This also actually cuts out the blank line after the instructions, so:

  # Changed but not updated:
  #   (use "git add <file>..." to update what will be committed)
  #   (use "git checkout -- <file>..." to discard changes in working directory)
  #
  #       modified:   file
  #

becomes:

  # Changed but not updated:
  #       modified:   file
  #

Arguably the blank should be left in.

 Documentation/config.txt |    4 ++++
 messages.c               |    1 +
 messages.h               |    1 +
 wt-status.c              |    8 ++++++++
 4 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ec308a6..cadbfc9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1215,6 +1215,10 @@ message.*::
 	pushNonFastForward::
 		Help message shown when linkgit:git-push[1] refuses
 		non-fast-forward refs. Default: true.
+	statusAdvice::
+		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.
 --
 
 pack.window::
diff --git a/messages.c b/messages.c
index 00fc196..d2785b2 100644
--- a/messages.c
+++ b/messages.c
@@ -3,6 +3,7 @@
 
 struct message_preference messages[] = {
 	{ "pushnonfastforward", 1 },
+	{ "statusadvice", 1 },
 };
 
 int git_default_message_config(const char *var, const char *value)
diff --git a/messages.h b/messages.h
index f175747..8edd08e 100644
--- a/messages.h
+++ b/messages.h
@@ -2,6 +2,7 @@
 #define MESSAGE_H
 
 #define MESSAGE_PUSH_NONFASTFORWARD 0
+#define MESSAGE_STATUS_ADVICE 1
 
 struct message_preference {
 	const char *name;
diff --git a/wt-status.c b/wt-status.c
index 85f3fcb..716d8cc 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -48,6 +48,8 @@ static void wt_status_print_unmerged_header(struct wt_status *s)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
 	color_fprintf_ln(s->fp, c, "# Unmerged paths:");
+	if (!messages[MESSAGE_STATUS_ADVICE].preference)
+		return;
 	if (!s->is_initial)
 		color_fprintf_ln(s->fp, c, "#   (use \"git reset %s <file>...\" to unstage)", s->reference);
 	else
@@ -60,6 +62,8 @@ static void wt_status_print_cached_header(struct wt_status *s)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
 	color_fprintf_ln(s->fp, c, "# Changes to be committed:");
+	if (!messages[MESSAGE_STATUS_ADVICE].preference)
+		return;
 	if (!s->is_initial) {
 		color_fprintf_ln(s->fp, c, "#   (use \"git reset %s <file>...\" to unstage)", s->reference);
 	} else {
@@ -73,6 +77,8 @@ static void wt_status_print_dirty_header(struct wt_status *s,
 {
 	const char *c = color(WT_STATUS_HEADER, s);
 	color_fprintf_ln(s->fp, c, "# Changed but not updated:");
+	if (!messages[MESSAGE_STATUS_ADVICE].preference)
+		return;
 	if (!has_deleted)
 		color_fprintf_ln(s->fp, c, "#   (use \"git add <file>...\" to update what will be committed)");
 	else
@@ -85,6 +91,8 @@ static void wt_status_print_untracked_header(struct wt_status *s)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
 	color_fprintf_ln(s->fp, c, "# Untracked files:");
+	if (!messages[MESSAGE_STATUS_ADVICE].preference)
+		return;
 	color_fprintf_ln(s->fp, c, "#   (use \"git add <file>...\" to include in what will be committed)");
 	color_fprintf_ln(s->fp, c, "#");
 }
-- 
1.6.4.2.266.g981efc.dirty

Re: [RFC/PATCH 0/4] make helpful messages optional

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

>   [1/4]: push: fix english in non-fast-forward message
>   [2/4]: push: re-flow non-fast-forward message

Sounds OK to me.

>   [3/4]: push: make non-fast-forward help message configurable
>   [4/4]: status: make "how to stage" messages optional

I didn't review the code in details, but I like the direction where
it's going. I'm not sure what's the best name for the configuration
option, but probably your proposal of "message" is good because it can
encompass many different cases (very detailed error messages, advices,
informative messages, ...).

And BTW, as the initial author of the push help message, I fully agree
that it is unnecessarily eating 3 lines of my terminal ;-). The
message is clearly targeted to newbies (but I do think it's very
helpfull to them).

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