Re: [PATCH 0/3] Reject non-ff pulls by default

From: John Keeping <john@keeping.me.uk>
Date: Sep 9, 2013, 21:10 UTC
Message-ID: <20130909211052.GK2582@serenity.lan>
In-Reply-To: <20130909204415.GC14536@sigill.intra.peff.net>

On Mon, Sep 09, 2013 at 04:44:16PM -0400, Jeff King wrote:

Show 29 quoted lines

> On Mon, Sep 09, 2013 at 09:24:35PM +0100, John Keeping wrote:
> 
> > > I think that would address the concern I raised, because it does not
> > > create a roadblock to new users accomplishing their task. They can
> > > ignore the warning, or choose "merge" as the default to shut up the
> > > warning (and it is easy to choose that if you are confused, because it
> > > is what git is doing by default alongside the warning).
> > 
> > I think we need to make sure that we give instructions for how to go
> > back if the default hasn't done what you wanted.  Something like this:
> > 
> >     Your pull did not fast-forward, so Git has merged '$upstream' into
> >     your branch, which may not be correct for your project.  If you
> >     would rather rebase your changes, run
> > 
> >         git rebase
> > 
> >     See "pull.mode" in git-config(1) to suppress this message in the
> >     future.
> 
> Yes, that's a good point. I don't know if just "git rebase" is the right
> advice, though; it would depend on whether we were actually pulling from
> the upstream or not.
> 
> I wonder if we have sufficient information at the time of the warning to
> print out the actual "git rebase" invocation that would rebase as if
> they had run "pull --rebase". I think we may have to do a little
> refactoring around the base selection from the reflog (IIRC, git-pull
> does not even calculate it at all if you are not using --rebase).

We can probably do something like:

    opts=
    if git merge-base --is-ancestor "$orig_head" "$merge_head"
    then
        opts=$merge_head
    else
        opts="$orig_head --onto $merge_head"
    fi

so that "git rebase $opts" is the right thing. Most users then get the simple "git rebase $merge_head" variant.

Show 11 quoted lines

> It is also depending on "git rebase" throwing away the merge commit we
> just created. Which I think should happen always if you have not
> configured anything (though perhaps we will eventually support a pull
> mode that does "rebase -p", you would not see this warning with that
> option anyway). But another option would be to simply tell them:
> 
>   git reset --keep HEAD^
>   git pull --rebase [X...]
> 
> where "[X...]" is the arguments they gave to rebase in the first place.
> That looks a little less friendly, though.

Yeah, I think we should keep it simple if possible. In my experience people are relatively happy to run a single "make things right" command but less so if there's a sequence of steps to be performed.