patchgit-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 <madcoder@debian.org>
---

  "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(-)

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

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:

  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.

  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.

  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.

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

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

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".

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.

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"?

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:

  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
:)

Pierre Habouzit <madcoder@debian.org> writes:

There are two cases the push does not fast forward. The case where you are truly behind (aka "stale") and you and the pushed-to repository have diverged history. Reporting success when you did not push due to the latter is unacceptable. I personally rely on the fast-forward safety in my push-out scripts, but I do not think it is just me. The exit status from commands are designed to be used that way.

The issue of "many refs in the repo but I work only on a few" has already been resolved by being able to say "I push only the current branch" in the previous thread, I think, but I am too busy to go back to re-study the history, so could you kindly do that for us?

The thing is, the user asked to push certain refs, and some did not get updated. The user has the right to expect a failure indication from the command. If you choose to _ignore_ the failure, that is _your_ choice, like:

	$ git push || :

You might argue that the case where you are truly behind _could_ be ignored and pretend as if the user did not even _ask_ to push it (hence, return success without doing anything to that branch), but I am not convinced even that is a good idea.

On jeu, jun 19, 2008 at 06:33:12 +0000, Junio C Hamano wrote:

  Sure, but when you use `git push` or `git push <remote>` forms of the
commands, you usually don't mean to push "everything", but rather
"everything that has new stuff". Stalled branches that you didn't
fast-forwarded yet should not generate issues IMHO.

  Of course, for git-push <remote> <branch> this could be different.

  I don't want to ignore push failures. It's just that in my
workflow[0], stalled branches are _not_ a problem, whereas non
fast-forward are. I cannot rely on the exit status, to know if all was
fine, and that's annoying, really (Not a _major_ PITA, but an annoyance
nonetheless).

  [0] we have quite a few branches for our different releases and
      production branches. I rarely do fixes there, but still have
      tracking branches for those, and when I'm pushing my master, I get
      errors each time another coworker pushed a production fix, until I
      git stash, git checkout <stalled-branch>, git merge
      <origin/stalled-branch>, git checkout back, git stash apply
      *PHEEEW* all that for git push[1] ? No thanks.

  [1] I know I can `git update-ref refs/heads/<stalled-branch> $(git
      rev-parse origin/<stalled-branch>)` to fix that in one step but:
        - it's plumbing ;
	- if you forget the refs/heads it don't DWIM ;
	- it's easy to mess things up with such use patterns
	  (sub-problem of `it's plumbing`).
      So I don't really think this is the proper answer to my problem.

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

I almost agreed with Pierre before I read your response, but as always you are right. If the only thing I know is that my current branch is ready to be pushed and I do not know if other branches are, I do not have to push all branches (I am not sure how exactly it works. Do we push all the branches, or do we push branches that exist on both sides?). Pushing the one I want to push is the right thing to do, and I can easily say it.

But I think that is a reasonable new feature and is close to what Pierre is asking for. He wants to be able to push "all except for the ones that he does not want to push", wants git to guess which ones are the ones he does not, and wants to make the logic of guessing to consider stale ones are unwanted.

But I do agree with you that it is wrong to replace existing "push all" with such a new feature. When people want to push all, they do want to push all and see the command report error when some branches are not updated. Even though I think what Pierre wants to have may be a reasonable new feature, it should not break "push all" for people who rely on the existing behavior.

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

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

Jeff King <peff@peff.net> writes:

That "strict rewind" workaround alone made me so disgusted beyond words.

And then made me step back and think again.

If it is stale, it is stale. A revert is just a different history that did not have some things that already happened, while a fork has some other things that did not happen in the original history on top of that. They are the same thing -- alternate history that drops some things that already happened. It does not matter if you are strictly stale or if you built a few (or hundreds of) commits on top of something that is stale. You are losing history if you tried to push such a thing.

And if you do not want to trigger "what you are pushing is not up to date", the solution is simple. You do not push them.

I however think what Pierre wanted to do is different, and you do not have to solve "force this strict rewind" problem to solve it.

What we need is (with a configuration option) to change the meaning of "matching refs" from the current definition "branches with the same name between here and there" to "branches with the same name between here and there, but excluding the ones that do not fast forward (or strictly behind)". So if you have tons of stale branches you checked out but haven't touched most of them, your "matching push" won't even try to push what you haven't touched.

I do not think we would ever want to allow a configuration that disables the exit status from push upon a push failure when somebody explicitly says "git push $there $this" and $this is not non-fast-forward. If you asked $this to be pushed, and if $this cannot be pushed, we _should_ error out.

So...

 (1) when you say "git push" to trigger the "matching refs" push, with the
     new configuration, you do not error out nor even try to push out
     things that do not fast forward.  Pretend that you did not even
     attempt to push them, and do not error out.  This may result in not
     pushing anything out, but that is what the people who have such a
     configuration is asking for.

     If you have configured which branches are pushed when you are on your
     current branch, and that branch --- most likely it is that current
     branch --- does not fast forward, it is your problem.

 (2) even with such a configuration, you can "git push $there $this" and
     "git push $there +$this" to explicitly ask refs to be pushed.  Such a
     push won't be interfered by the new configuration and correctly fail
     (or force).

On Thu, Jun 26, 2008 at 08:19:02AM +0000, Junio C Hamano wrote:

  Of course if I: git push $remote $branch, if it's stalled, I want it
to fail (exit with !0). *BUT* if I don't specify branches, I don't want
the stalled ones to generate noise. That's exactly the behaviour I'd
like to see.

On Thu, Jun 26, 2008 at 01:19:02AM -0700, Junio C Hamano wrote:

Heh.

OK. Isn't this basically just another way of restating my third "solution"? Mine marks strict rewinds as "up to date" instead of "matching", but in either case, the result is that there is nothing to even try pushing. And the proposal I gave was "allow -f to force a strict rewind [which would be as it is now] but only if the refspec is mentioned explicitly on the command line, and not part of an automatic match."

So I think the behavior is the same, but the difference is the mental model.

And not counting stale branches as matching doesn't seem like quite the right mental model. The matching is about the ref _names_, not about their contents. So I think the better mental model is that these branches are simply "up to date" already -- the remote has everything we have, and more.

Not to mention that ref matching can affect _other_ refs, too, can it not? I think there are some checks for having two refspec srcs pushing to the same dest.

We do this already for refs that have no changes to push. IOW, we are not masking an error so much as saying "this condition is _not_ an error" (and letting the user say that with a config option).

I think (1) should be implemented not as "this src ref and this dst ref do not match" but rather "this pair is up to date". And given that, I think that is basically the same as my proposal, except that the "push.rewindIsUpToDate" config variable would only be respected for a "matching refs" push. Which does feel a little inconsistent, as I noted in my previous mail, but I think it DWYM.

-Peff

On Sat, Jun 28, 2008 at 12:33:38AM -0400, Jeff King wrote:

Where of course if we take "Y" in DWYM to be Pierre, he has already said that's what he meant. :)

-Peff