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

classic Classic list List threaded Threaded
48 messages Options
123
Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>
---
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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

MichaelJGruber
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 <[hidden email]>
> ---
> 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);
>  

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
In reply to this post by Matthieu Moy
Matthieu Moy <[hidden email]> 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 <[hidden email]>
> ---
> 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
 --------
 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Nicolas Sebrecht-3
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
Nicolas Sebrecht <[hidden email]> 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.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Matthieu Moy
In reply to this post by MichaelJGruber
Michael J Gruber <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Matthieu Moy
In reply to this post by Junio C Hamano
Junio C Hamano <[hidden email]> writes:

> Matthieu Moy <[hidden email]> 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 <[hidden email]>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

MichaelJGruber
In reply to this post by Matthieu Moy
Matthieu Moy venit, vidit, dixit 07.08.2009 21:21:
> Michael J Gruber <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
In reply to this post by Matthieu Moy
Matthieu Moy <[hidden email]> writes:

> Junio C Hamano <[hidden email]> 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.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Matthieu Moy
Junio C Hamano <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Matthieu Moy
In reply to this post by 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 <[hidden email]>
---
(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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Matthieu Moy
Teemu Likonen <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
Matthieu Moy <[hidden email]> writes:

> Teemu Likonen <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Nicolas Sebrecht-3
The 08/08/09, Junio C Hamano wrote:

> Matthieu Moy <[hidden email]> writes:
> > Teemu Likonen <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Matthieu Moy
In reply to this post by Junio C Hamano
Junio C Hamano <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
Matthieu Moy <[hidden email]> 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.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Matthieu Moy
Junio C Hamano <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
Matthieu Moy <[hidden email]> writes:

> Junio C Hamano <[hidden email]> 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"?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Matthieu Moy
Junio C Hamano <[hidden email]> writes:

> Matthieu Moy <[hidden email]> writes:
>
>> Junio C Hamano <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
123