[PATCH] git-send-pack: don't consider branch lagging behind as errors.

It's really painful to have git push error out when it's just that one of
your tracking branches isn't up to date with respect to a remote branch.

Let just add a new status: "lagging", always print it to screen when we're
lagging, but don't exit with a non 0 value, as it really alarms users.

Signed-off-by: Pierre Habouzit <redacted>
---

  "lagging" is probably not a very nice name, and anyone is welcomed to use
  a better word for the concept.

  Another little glitch is that with this patch you can see:

	  $ git push
	  To {your-remote}
	   < [lagging]         {local-branch} -> {remote-branch}
	  Everything up-to-date

  The "Everything up-to-date" is slightly confusing, so maybe we should make it
  better like "Everything up-to-date or strict parent" or whatever.



 builtin-send-pack.c |   10 +++++++++-
 cache.h             |    1 +
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 8d1e7be..cfbc108 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c

@@ -339,6 +339,9 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count)
 		print_ref_status('=', "[up to date]", ref,
 				ref->peer_ref, NULL);
 		break;
+	case REF_STATUS_LAGGING_BEHIND:
+		print_ref_status('<', "[lagging]", ref, ref->peer_ref, NULL);
+		break;
 	case REF_STATUS_REJECT_NONFASTFORWARD:
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
 				"non-fast forward");

@@ -390,6 +393,7 @@ static int refs_pushed(struct ref *ref)
 		switch(ref->status) {
 		case REF_STATUS_NONE:
 		case REF_STATUS_UPTODATE:
+		case REF_STATUS_LAGGING_BEHIND:
 			break;
 		default:
 			return 1;

@@ -568,7 +572,10 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 		      || !ref_newer(new_sha1, ref->old_sha1));
 
 		if (ref->nonfastforward && !ref->force && !args.force_update) {
-			ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
+			if (ref_newer(ref->old_sha1, new_sha1))
+				ref->status = REF_STATUS_LAGGING_BEHIND;
+			else
+				ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
 			continue;
 		}
 

@@ -628,6 +635,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 		switch (ref->status) {
 		case REF_STATUS_NONE:
 		case REF_STATUS_UPTODATE:
+		case REF_STATUS_LAGGING_BEHIND:
 		case REF_STATUS_OK:
 			break;
 		default:

diff --git a/cache.h b/cache.h
index 23f3b92..b9b32eb 100644
--- a/cache.h
+++ b/cache.h

@@ -673,6 +673,7 @@ struct ref {
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
 		REF_STATUS_EXPECTING_REPORT,
+		REF_STATUS_LAGGING_BEHIND,
 	} status;
 	char *remote_status;
 	struct ref *peer_ref; /* when renaming */

-- 
1.5.6.rc3.158.g1a80c.dirty

On Thu, Jun 19, 2008 at 12:51:55PM +0200, Pierre Habouzit wrote:

It's really painful to have git push error out when it's just that one of
your tracking branches isn't up to date with respect to a remote branch.

Let just add a new status: "lagging", always print it to screen when we're
lagging, but don't exit with a non 0 value, as it really alarms users.

This has been discussed before, and the suggested term was "stale".

Check out:

  http://thread.gmane.org/gmane.comp.version-control.git/73038/focus=73186

which is uncannily identical (the difference is the name, and that I
don't show the lagged branches unless -v is given).

Among the issues that were not sorted out last time:

  - should stale branches be shown without -v?

  - calling ref_newer here is inefficient, since we have already called
    it in the other direction. We should probably do the traversal once
    in such a way as to find out which ref is newer (or if it is
    indeterminate).

  - there is a possible danger with "git push -f", in that you force
    both rejected branches as well as stale branches. Junio and I
    discussed the possibility of disallowing "-f" unless the user
    explicitly requested _what_ to push; i.e., --all, --matching,
    --mirror, or a refspec. See:

      http://thread.gmane.org/gmane.comp.version-control.git/74425/focus=74481

I was considering resurrecting my patch after working up that safety
valve.

-Peff

On Thu, Jun 19, 2008 at 01:37:48PM +0000, Jeff King wrote:

On Thu, Jun 19, 2008 at 12:51:55PM +0200, Pierre Habouzit wrote:

quoted

It's really painful to have git push error out when it's just that one of
your tracking branches isn't up to date with respect to a remote branch.

Let just add a new status: "lagging", always print it to screen when we're
lagging, but don't exit with a non 0 value, as it really alarms users.

This has been discussed before, and the suggested term was "stale".

Check out:

  http://thread.gmane.org/gmane.comp.version-control.git/73038/focus=73186

which is uncannily identical (the difference is the name, and that I
don't show the lagged branches unless -v is given).

Among the issues that were not sorted out last time:

  - should stale branches be shown without -v?

  I believe so, it's valuable information. It's as valuable as what you
get after a git fetch nowadays (like branches have diverged n and m
commits each or similar) But oh well… I don't care that much.

  - calling ref_newer here is inefficient, since we have already called
    it in the other direction. We should probably do the traversal once
    in such a way as to find out which ref is newer (or if it is
    indeterminate).

  Well, true, though I don't expect people to have tons of local
branches that match a refspec _and_ lag behind. I suspect this is a very
minor performance loss.

  - there is a possible danger with "git push -f", in that you force
    both rejected branches as well as stale branches. Junio and I
    discussed the possibility of disallowing "-f" unless the user
    explicitly requested _what_ to push; i.e., --all, --matching,
    --mirror, or a refspec. See:

      http://thread.gmane.org/gmane.comp.version-control.git/74425/focus=74481

  Well afaict this is a separate issue, as we're (with such a patch)
only changing what gets printed on the console, not the internal
behavior. So solving this second issue should not really be a
precondition to the inclusion of such a patch.

I was considering resurrecting my patch after working up that safety
valve.

  Please please please do :)
  The exit 1 of git-push is really annoying me these days.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

On Thu, Jun 19, 2008 at 03:52:00PM +0200, Pierre Habouzit wrote:

quoted

  http://thread.gmane.org/gmane.comp.version-control.git/73038/focus=73186

[...]

quoted

  - should stale branches be shown without -v?

  I believe so, it's valuable information. It's as valuable as what you
get after a git fetch nowadays (like branches have diverged n and m
commits each or similar) But oh well… I don't care that much.

If you read the beginning of that thread, the original impetus was
people cloning repos that had dozens of branches, then doing a push.
If they hadn't recently done a fetch, they got dozens of lines of
"rejected".

quoted

  - calling ref_newer here is inefficient, since we have already called
    it in the other direction. We should probably do the traversal once
    in such a way as to find out which ref is newer (or if it is
    indeterminate).

  Well, true, though I don't expect people to have tons of local
branches that match a refspec _and_ lag behind. I suspect this is a very
minor performance loss.

Yeah, maybe it is not worth worrying about; I haven't actually measured
any performance issue. I'll try to look and see how painful it is to
combine the traversals.

quoted

  - there is a possible danger with "git push -f", in that you force
    both rejected branches as well as stale branches. Junio and I

  Well afaict this is a separate issue, as we're (with such a patch)
only changing what gets printed on the console, not the internal
behavior. So solving this second issue should not really be a
precondition to the inclusion of such a patch.

It is a separate issue, but it is exacerbated by hiding stale refs.
Imagine:

$ git push
To /path/to/repo
   ! [rejected]        master -> master (non-fast forward)

$ git push -f
To /path/to/repo
   + 0abfa88...c1ed93b master -> master (forced update)
   + 0329485...3498576 stale_branch -> stale_branch (forced update)

I think that is a nasty surprise to spring on an unsuspecting user.
Another solution might be "-f" not pushing rewound branches, but then we
need a way to specify "no, really, push this rewound branch". Perhaps
"-f -f"?

  Please please please do :)
  The exit 1 of git-push is really annoying me these days.

OK, I will try to take a look in the next few days.

-Peff

On Thu, Jun 19, 2008 at 03:11:10PM +0000, Jeff King wrote:

On Thu, Jun 19, 2008 at 03:52:00PM +0200, Pierre Habouzit wrote:

quoted

quoted

  - there is a possible danger with "git push -f", in that you force
    both rejected branches as well as stale branches. Junio and I

  Well afaict this is a separate issue, as we're (with such a patch)
only changing what gets printed on the console, not the internal
behavior. So solving this second issue should not really be a
precondition to the inclusion of such a patch.

It is a separate issue, but it is exacerbated by hiding stale refs.
Imagine:

$ git push
To /path/to/repo
   ! [rejected]        master -> master (non-fast forward)

$ git push -f
To /path/to/repo
   + 0abfa88...c1ed93b master -> master (forced update)
   + 0329485...3498576 stale_branch -> stale_branch (forced update)

I think that is a nasty surprise to spring on an unsuspecting user.
Another solution might be "-f" not pushing rewound branches, but then we
need a way to specify "no, really, push this rewound branch". Perhaps
"-f -f"?

  Well then we could keep the [stalled] lines for now until this issue
is resolved then, despite what the people at the beginning of the other
thread complained about. My real issue is that I have my shell
configured so that my prompt becomes inverted if the last command
failed. So do many people I know, and well, git push for stalled
references should just not generate an error. _this_ is my sole concern
:)

quoted

  Please please please do :)
  The exit 1 of git-push is really annoying me these days.

OK, I will try to take a look in the next few days.

-Peff

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

On Thu, Jun 19, 2008 at 11:11:10AM -0400, Jeff King wrote:

quoted

  Please please please do :)
  The exit 1 of git-push is really annoying me these days.

OK, I will try to take a look in the next few days.

I finally got a chance to look at this [distinguishing stale refs from
other non-ff refs]. I was originally planning on doing a multi-patch
series:

 1/4 distinguish visually between stale / non-ff refs
 2/4 config option to let stale refs not count towards exit code of 1
 3/4 disallow "push -f" without explicit refspec or option (like --all)
 4/4 config option not to show stale refs unless "-v" is given (which
     is now reasonably safe, since invisible refs won't get pushed when
     you just repeat your "git push" with a "-f")

But I didn't like how it was turning out. Specifically:

  - two config options doesn't really make sense. Either you care about
    stale refs, or you don't. If you do, you should see them and have
    them impact your exit code.

  - I thought 3/4 would introduce a generally useful safety valve. But I
    realized I really _like_ the current behavior. Most of the time I
    force a push, it is something like "git push", "oops, that is a
    non-ff, but I know it is OK", "git push -f". It makes sense to me
    that I can repeat my last command and simply say "OK, force this."
    But with such a safety valve, I have to realize the shortcut that
    "git push" was performing (i.e., "git push --matching"), and then
    explicitly retype it. Which is not hard mentally, but it makes the
    interface seem very clunky.

Instead of that, I am considering something more like this:

  - we always visually distinguish stale and other non-ff refs

  - if a config option is set, we treat stale refs as "up to date". That
    is, we don't show them (unless -v is set), and they don't affect the
    exit code. The option is unset by default, giving the same as
    current behavior.

  - If the config option is not set, then forcing works as before.

  - If the config option is set, then we _do not_ force stale refs, but
    only other non-ff. Meaning we are truly treating them like our "up
    to date" refs and saying "there is nothing of interest to push".

This leaves one open issue. If you have the "treat stale as up to date"
config option set, how do you force a strict rewind (if the occasion
comes up that in some instance, you do want to treat it differently)?

One solution is for the user to unset the config, do the forced push,
and then reset it. It makes some sense; the user has said, via the
config, that they consider stale refs uninteresting. So to actually
perform such a push, they need to "unsay" that temporarily.

Another solution would be an additional flag for forcing strict rewinds
(or even "-f -f", though I'm not sure that makes sense). This seems a
little hack-ish, since it would _only_ be used if this other config flag
is set.

Yet another solution would be to allow "-f" to force a strict rewind,
but only if the refspec is mentioned explicitly on the command line, and
not part of an automatic match. Reasonably DWIM, but I think it kills
the consistency we have now (that "--all" or "--matching" are really
just shorthands for spelling out all of the respective refspecs).

Does this seem like a good approach overall? Existing behavior should be
identical unless the config option is set, and with it set, I think it
should satisfy Pierre and posters from the original thread. If that is
sensible, which of the solutions for "no, I really want to force this
strict rewind" is the most palatable?

-Peff