Re: [PATCH v2 11/14] tentative: pull: change the semantics of --ff-only

From: Elijah Newren <newren@gmail.com>
To: Felipe Contreras <felipe.contreras@gmail.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
	"Vít Ondruch" <vondruch@redhat.com>,
	"Alex Henrie" <alexhenrie24@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
	"Philip Oakley" <philipoakley@iee.email>,
	"Theodore Y . Ts'o" <tytso@mit.edu>,
	"Jacob Keller" <jacob.keller@gmail.com>,
	"John Keeping" <john@keeping.me.uk>,
	"Richard Hansen" <rhansen@rhansen.org>,
	"Brian M. Carlson" <sandals@crustytoothpaste.net>,
	"W. Trevor King" <wking@tremily.us>
Subject: Re: [PATCH v2 11/14] tentative: pull: change the semantics of --ff-only
Date: Fri, 4 Dec 2020 15:39:19 -0800	[thread overview]
Message-ID: <CABPp-BG0WWvk96T757WMx=004qVqkf+SU8m3cCZsb1feTZJ80Q@mail.gmail.com> (raw)
In-Reply-To: <20201204061623.1170745-12-felipe.contreras@gmail.com>

On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> We want --ff-only to make sense only when no --merge or --rebase option
> is specified.

A lot of git commands have opposite options, and git allows them both
to be specified with the last one winning.  Thus, much like
  git log --patch --no-patch
mean show logs without patches and
  git log --no-patch --patch
means show logs with patches, I would similarly expect the following
two commands to have opposite behavior:
  git pull --merge --no-ff
  git pull --no-ff --merge

> Currently --rebase already ignores --ff-only (or any other --ff option),
> but --merge fails.
>
> Make it so --ff-only is only considered in the default mode.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/pull.c  | 4 ++++
>  t/t5520-pull.sh | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index f54ff36b57..ebf2ac687b 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -1037,6 +1037,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                 }
>         }
>
> +       /* Disable --ff-only when --merge is specified */
> +       if (!can_ff && !default_mode && !opt_rebase && opt_ff && !strcmp(opt_ff, "--ff-only"))
> +               opt_ff = NULL;
> +
>         if (opt_rebase) {
>                 int ret = 0;
>                 if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index fdd1f79b06..eec6224fb0 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -843,7 +843,7 @@ test_expect_success 'git pull non-fast-forward (ff-only)' '
>         test_must_fail git pull
>  '
>
> -test_expect_failure 'git pull non-fast-forward with merge (ff-only)' '
> +test_expect_success 'git pull non-fast-forward with merge (ff-only)' '
>         test_when_finished "git checkout master && git branch -D other test" &&
>         test_config pull.ff only &&
>         git checkout -b other master^ &&
> --
> 2.29.2
>