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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 30, 2014, 17:55 UTC
Message-ID: <xmqqr44eg0s0.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1398770226-9686-1-git-send-email-felipe.contreras@gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> writes:

> These are the steps needed to achieve this:

The overall progression (this comment is only about the design, not the implementation) looks almost sensible, but I may have missed some issues because the presentation was done in reverse.

In the following comment, I'll flip the presentation order to better show natural progression of what the users will see.

Show 20 quoted lines

> 1) Rename pull.rename to pull.mode and
>    branch.<name>.rebase to branch.<name>.pullmode
>
> This way the configurations and options remain consistent:
>
>   git pull --merge
>   pull.mode = merge
>   branch.<name>.pullmode = merge
>
>   git pull --rebase
>   pull.mode = rebase
>   branch.<name>.pullmode = rebase
>
>   git pull --rebase=preserve
>   pull.mode = rebase-preserve
>   branch.<name>.pullmode = rebase-preserve
>
>   git pull
>   pull.mode = merge-ff-only
>   branch.<name>.pullmode = merge-ff-only

Until the "--merge" option is added, "pull.mode = merge" cannot be the same as "git pull --merge". I think you either need to squash these two steps into one, or flip the order of them.

Show 5 quoted lines

> 2) Add --merge option
>
> Since we have a message that says "If unsure, run 'git pull --merge'", which is
> more friendly than 'git pull --no-rebase', we should add this option, and
> deprecate --no-rebase.

Obviously s/have a/will have a/, but the intention is good.

Show 5 quoted lines

> However, the documentation would become confusing if --merge is configured in
> pull.rebase, instead, we want something like this:
>
>   See `pull.mode`, `branch.<name>.pullmode` in linkgit:git-config[1] if you want
>   to make `git pull` always use `--merge`.

It gets unclear to me how the transition is planned around here. Is this a correct paraphrasing of these four steps?

    - Add pull.mode (and its branch-specific friend) and "pull
      --merge" so that people can set the former to "merge" or train
      their fingers to type the latter to keep doing the
      fetch-and-merge (your steps 1 and 2)

    - Add ff-only to pull.mode (your step 3)

    - With the endgame of "out of box Git without any configuration
      refuses 'git pull' (without --merge/--rebase) that does not
      fast forward" in mind, start warning "In the future you will
      have to either set pull.mode (and/or its friends) or type
      "pull --merge" (or "pull --rebase") when the endgame version
      of 'git pull' would fail with the error message, but still do
      as was asked to do as before.  At this step, existing users
      can set pull.mode to "merge" or "rebase" or whatever to
      squelch the warning.

    - Flip the default.  By the time this happens, thanks to the
      previous step to warn beforehand, nobody needs to see the
      warning. (your step 4)

If that is the rough outline, I think it is sensible.

Show 6 quoted lines

> 3) Add merge-ff-only config
>
> This option would trigger a check inside `git pull` itself, and error out with
> the aforementioned message if it's not possible to do a fast-forward merge.
>
> However, this option conflicts with --rebase, and --no-rebase. Solution below.

Am I reading you correctly that setting "pull.mode = ff-only" will require you to explicitly say "git pull --merge/--rebase"? If that is the case, I think the step makes sense.

Show 13 quoted lines

> 4) Only allow fast-forward merges by default
>
> We could pass --ff-only to `git merge`, however, if we do that we'll get an error like this:
>
>   Not possible to fast-forward, aborting.
>
> This is not friendly; we want an error that is user-friendly:
>
>   The pull was not fast-forward, please either merge or rebase.
>   If unsure, run 'git pull --merge'.
>
> When we do this we want to give the users the option to go back to the previous
> behavior, so a new configuration is needed.