Re: [PATCH] push: Provide situational hints for non-fast-forward errors

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 16, 2012, 18:07 UTC
Message-ID: <7vk42kh11k.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120316172013.GA8119@gmail.com>

Christopher Tiwald <christiwald@gmail.com> writes:

Show 6 quoted lines

> One quick, slightly-off-topic question: I'd like to take another crack
> at the patch's commit message, to implement some of your language
> suggestions and clean it up further. Is it reasonable for me to wait a
> few days for additional comments or updates, squash together these
> fixups into a single v2 patch (assuming one patch is a logical unit for
> it), then resubmit?

Surely.

After reading the fix-up patch again, I actually have a couple of comments/reservations myself.

 (1) I suggested (and the fix-up patch does so) to use a single existing
     advice configuration, but if you read the description in my response
     to Clemens carefully, you may realize that at least the configuration
     for "Here is how to deal with your current branch" and "Here is how
     for the rest of your branch" might be better if they are separate
     variables. The user may fix current branch (i.e. "pull then push"),
     set the advice.pushNonFastForward to false thinking that he learned
     everything there to know about non fast-forward, and then get another
     failure from "git push" because other branches are still behind, but
     with my "fix-up" patch, we would no longer give advice to him.

 (2) The advice to "Your current branch is OK but you are also pushing
     others that do not fast-forward" only talks about "check out, pull
     and then push", but an equally plausible solution may be "don't push
     other branches---you are not working on them right now".  Both of our
     versions have this issue.

     I didn't trace the logic flow, though. If this advice is issued only
     to the user who explicitly said he wants to push these other branches
     (e.g. has "push.default = matching" in the config and gave no command
     line options, or gave refspec on the command line to tell us to push
     these other branches), then the wording is OK.

     But for the purpose of helping people who may be surprised by the
     current "matching" default, I think we should detect this very narrow
     case:

     - The user did not give us any refspec from the command line; and

     - The user does not have push.default set to matching (either the
       user does not have any push.default, or it is set to something
       else); and

     - The remote.$name.push would not push the branch other than the
       current branch.

     When these three conditions hold, we can be sure that the user worked
     on more than one branch and did "git push $there" without telling us
     what to push, and we defaulted to push "matching" and failed on stale
     branches that the user hasn't been working on.  In that case, "don't
     push other branches---perhaps push.default needs to be set" may be a
     far more appropriate advice.

     So, the third case may have to be split further into two.