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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 19, 2011, 20:16 UTC
Message-ID: <7vfwggmi50.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20111219103746.GB1736@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

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

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

Show 14 quoted lines

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

OK, I think we are in agreement.

Show 10 quoted lines

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

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

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

Show 18 quoted lines

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

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

Thanks.