[PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Alex Henrie]

The warning about pulling without specifying how to reconcile divergent
branches says that after setting pull.rebase to true, --ff-only can
still be passed on the command line to require a fast-forward. Make that
actually work.

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 advice.c                     |  5 +++++
 advice.h                     |  1 +
 builtin/merge.c              |  2 +-
 builtin/pull.c               | 11 ++++++++---
 t/t7601-merge-pull-config.sh | 24 ++++++++++++++++++++++++
 5 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/advice.c b/advice.c
index 0b9c89c48a..337e8f342b 100644
--- a/advice.c
+++ b/advice.c
@@ -286,6 +286,11 @@ void NORETURN die_conclude_merge(void)
 	die(_("Exiting because of unfinished merge."));
 }
 
+void NORETURN die_ff_impossible(void)
+{
+	die(_("Not possible to fast-forward, aborting."));
+}
+
 void advise_on_updating_sparse_paths(struct string_list *pathspec_list)
 {
 	struct string_list_item *item;
diff --git a/advice.h b/advice.h
index bd26c385d0..1624043838 100644
--- a/advice.h
+++ b/advice.h
@@ -95,6 +95,7 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...);
 int error_resolve_conflict(const char *me);
 void NORETURN die_resolve_conflict(const char *me);
 void NORETURN die_conclude_merge(void);
+void NORETURN die_ff_impossible(void);
 void advise_on_updating_sparse_paths(struct string_list *pathspec_list);
 void detach_advice(const char *new_name);
 
diff --git a/builtin/merge.c b/builtin/merge.c
index a8a843b1f5..aa920ac524 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1620,7 +1620,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	}
 
 	if (fast_forward == FF_ONLY)
-		die(_("Not possible to fast-forward, aborting."));
+		die_ff_impossible();
 
 	if (autostash)
 		create_autostash(the_repository,
diff --git a/builtin/pull.c b/builtin/pull.c
index 3e13f81084..d979660482 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1046,9 +1046,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	if (rebase_unspecified && !opt_ff && !can_ff) {
-		if (opt_verbosity >= 0)
-			show_advice_pull_non_ff();
+	if (!can_ff) {
+		if (opt_ff) {
+			if (!strcmp(opt_ff, "--ff-only"))
+				die_ff_impossible();
+		} else {
+			if (rebase_unspecified && opt_verbosity >= 0)
+				show_advice_pull_non_ff();
+		}
 	}
 
 	if (opt_rebase) {
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 52e8ccc933..b5a09a60f9 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -183,6 +183,30 @@ test_expect_success 'pull prevents non-fast-forward with "only" in pull.ff' '
 	test_must_fail git pull . c3
 '
 
+test_expect_success 'pull prevents non-fast-forward with pull.ff=only and pull.rebase=true' '
+	git reset --hard c1 &&
+	test_config pull.ff only &&
+	test_config pull.rebase true &&
+	test_must_fail git pull . c3
+'
+
+test_expect_success 'pull prevents non-fast-forward with pull.ff=only and pull.rebase=false' '
+	git reset --hard c1 &&
+	test_config pull.ff only &&
+	test_config pull.rebase false &&
+	test_must_fail git pull . c3
+'
+
+test_expect_success 'pull prevents non-fast-forward with --rebase --ff-only' '
+	git reset --hard c1 &&
+	test_must_fail git pull --rebase --ff-only . c3
+'
+
+test_expect_success 'pull prevents non-fast-forward with --no-rebase --ff-only' '
+	git reset --hard c1 &&
+	test_must_fail git pull --no-rebase --ff-only . c3
+'
+
 test_expect_success 'merge c1 with c2 (ours in pull.twohead)' '
 	git reset --hard c1 &&
 	git config pull.twohead ours &&
-- 
2.32.0.171.gfa4a44ff46

RE: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Felipe Contreras]

Alex Henrie wrote:
> The warning about pulling without specifying how to reconcile divergent
> branches says that after setting pull.rebase to true, --ff-only can
> still be passed on the command line to require a fast-forward. Make that
> actually work.

I don't know where that is being said, but it's wrong: --ff-only is
meant for merge only.

> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -1046,9 +1046,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>  
>  	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
>  
> -	if (rebase_unspecified && !opt_ff && !can_ff) {
> -		if (opt_verbosity >= 0)
> -			show_advice_pull_non_ff();
> +	if (!can_ff) {
> +		if (opt_ff) {
> +			if (!strcmp(opt_ff, "--ff-only"))
> +				die_ff_impossible();

As I've mentioned multiple times already, this is wrong.

The advice clearly says:

  You can also pass --rebase, --no-rebase, or --ff-only on the command
  line to override the configured default per invocation.

With your patch now this is even less true:

  git -c pull.ff=only pull --rebase

> +		} else {
> +			if (rebase_unspecified && opt_verbosity >= 0)
> +				show_advice_pull_non_ff();
> +		}
>  	}
>  
>  	if (opt_rebase) {
> diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
> index 52e8ccc933..b5a09a60f9 100755
> --- a/t/t7601-merge-pull-config.sh
> +++ b/t/t7601-merge-pull-config.sh
> @@ -183,6 +183,30 @@ test_expect_success 'pull prevents non-fast-forward with "only" in pull.ff' '
>  	test_must_fail git pull . c3
>  '

Can you add this test [1] so I don't have to explain the same thing over
and over?

  test_expect_success 'pull allows non-fast-forward with "only" in pull.ff if --rebase' '
    git reset --hard c1 &&
    test_config pull.ff only &&
    git pull --rebase . c3
  '

Cheers.

> +test_expect_success 'pull prevents non-fast-forward with pull.ff=only and pull.rebase=true' '
> +	git reset --hard c1 &&
> +	test_config pull.ff only &&
> +	test_config pull.rebase true &&
> +	test_must_fail git pull . c3
> +'
> +
> +test_expect_success 'pull prevents non-fast-forward with pull.ff=only and pull.rebase=false' '
> +	git reset --hard c1 &&
> +	test_config pull.ff only &&
> +	test_config pull.rebase false &&
> +	test_must_fail git pull . c3
> +'
> +
> +test_expect_success 'pull prevents non-fast-forward with --rebase --ff-only' '
> +	git reset --hard c1 &&
> +	test_must_fail git pull --rebase --ff-only . c3
> +'
> +
> +test_expect_success 'pull prevents non-fast-forward with --no-rebase --ff-only' '
> +	git reset --hard c1 &&
> +	test_must_fail git pull --no-rebase --ff-only . c3
> +'
> +
>  test_expect_success 'merge c1 with c2 (ours in pull.twohead)' '
>  	git reset --hard c1 &&
>  	git config pull.twohead ours &&
> -- 

[1] https://lore.kernel.org/git/20210711170703.651081-1-felipe.contreras@gmail.com/

-- 
Felipe Contreras

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Alex Henrie]

On Sun, Jul 11, 2021 at 11:08 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Alex Henrie wrote:
> > The warning about pulling without specifying how to reconcile divergent
> > branches says that after setting pull.rebase to true, --ff-only can
> > still be passed on the command line to require a fast-forward. Make that
> > actually work.
>
> I don't know where that is being said, but it's wrong: --ff-only is
> meant for merge only.
>
> > --- a/builtin/pull.c
> > +++ b/builtin/pull.c
> > @@ -1046,9 +1046,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> >
> >       can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
> >
> > -     if (rebase_unspecified && !opt_ff && !can_ff) {
> > -             if (opt_verbosity >= 0)
> > -                     show_advice_pull_non_ff();
> > +     if (!can_ff) {
> > +             if (opt_ff) {
> > +                     if (!strcmp(opt_ff, "--ff-only"))
> > +                             die_ff_impossible();
>
> As I've mentioned multiple times already, this is wrong.
>
> The advice clearly says:
>
>   You can also pass --rebase, --no-rebase, or --ff-only on the command
>   line to override the configured default per invocation.
>
> With your patch now this is even less true:
>
>   git -c pull.ff=only pull --rebase

I think it's an improvement over the current situation. --no-rebase
does not override pull.ff=only, so it makes sense that --rebase does
not override pull.ff=only either. Besides, it's generally better to
abort instead of rewriting history if it's not perfectly clear that
the user meant to rewrite the history.

-Alex

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Felipe Contreras]

Alex Henrie wrote:
> On Sun, Jul 11, 2021 at 11:08 AM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Alex Henrie wrote:
> > > The warning about pulling without specifying how to reconcile divergent
> > > branches says that after setting pull.rebase to true, --ff-only can
> > > still be passed on the command line to require a fast-forward. Make that
> > > actually work.
> >
> > I don't know where that is being said, but it's wrong: --ff-only is
> > meant for merge only.
> >
> > > --- a/builtin/pull.c
> > > +++ b/builtin/pull.c
> > > @@ -1046,9 +1046,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> > >
> > >       can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
> > >
> > > -     if (rebase_unspecified && !opt_ff && !can_ff) {
> > > -             if (opt_verbosity >= 0)
> > > -                     show_advice_pull_non_ff();
> > > +     if (!can_ff) {
> > > +             if (opt_ff) {
> > > +                     if (!strcmp(opt_ff, "--ff-only"))
> > > +                             die_ff_impossible();
> >
> > As I've mentioned multiple times already, this is wrong.
> >
> > The advice clearly says:
> >
> >   You can also pass --rebase, --no-rebase, or --ff-only on the command
> >   line to override the configured default per invocation.
> >
> > With your patch now this is even less true:
> >
> >   git -c pull.ff=only pull --rebase
> 
> I think it's an improvement over the current situation. --no-rebase
> does not override pull.ff=only, so it makes sense that --rebase does
> not override pull.ff=only either.

I disagree, but that's not the point, the point is that now the advice
message is wrong since --rebase doesn't override pull.ff=only.

Additionally the documentation is inaccurate too because at no point
does pull.ff mention anything about rebase:

pull.ff::
	By default, Git does not create an extra merge commit when merging
	a commit that is a descendant of the current commit. Instead, the
	tip of the current branch is fast-forwarded. When set to `false`,
	this variable tells Git to create an extra merge commit in such
	a case (equivalent to giving the `--no-ff` option from the command
	line). When set to `only`, only such fast-forward merges are
	allowed (equivalent to giving the `--ff-only` option from the
	command line). This setting overrides `merge.ff` when pulling.

-- 
Felipe Contreras

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Phillip Wood]

Hi Alex


On 11/07/2021 02:26, Alex Henrie wrote:
> The warning about pulling without specifying how to reconcile divergent
> branches says that after setting pull.rebase to true, --ff-only can
> still be passed on the command line to require a fast-forward. Make that
> actually work.

Thanks for revising this patch, I like this approach much better. I do 
however have some concerns about the interaction of pull.ff with the 
rebase config and command line options. I'd naively expect the following 
behavior (where rebase can fast-forward if possible)

   pull.ff  pull.rebase  commandline  action
    only     not false                rebase
    only     not false   --no-rebase  fast-forward only
     *       not false    --ff-only   fast-forward only
    only     not false    --ff        merge --ff
    only     not false    --no-ff     merge --no-ff
    only       false                  fast-forward only
    only       false      --rebase    rebase
    only       false      --ff        merge --ff
    only       false      --no-ff     merge --no-ff

I don't think enforcing fast-forward only for rebases makes sense unless 
it is given on the command line. If the user gives `--rebase` 
`--ff-only` on the command line then we should either error out or take 
the last one in which case `pull --rebase --ff-only` would fast-forward 
only but `pull --ff-only --rebase` would rebase. We should also decide 
what to do when the user has pull.ff set to something other than only 
and also has pull.rebase to something other than false set - I'd guess 
we'd want to rebase unless there is a merge option on the command line 
but I haven't thought about those cases.

Best Wishes

Phillip

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>   advice.c                     |  5 +++++
>   advice.h                     |  1 +
>   builtin/merge.c              |  2 +-
>   builtin/pull.c               | 11 ++++++++---
>   t/t7601-merge-pull-config.sh | 24 ++++++++++++++++++++++++
>   5 files changed, 39 insertions(+), 4 deletions(-)
> 
> diff --git a/advice.c b/advice.c
> index 0b9c89c48a..337e8f342b 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -286,6 +286,11 @@ void NORETURN die_conclude_merge(void)
>   	die(_("Exiting because of unfinished merge."));
>   }
>   
> +void NORETURN die_ff_impossible(void)
> +{
> +	die(_("Not possible to fast-forward, aborting."));
> +}
> +
>   void advise_on_updating_sparse_paths(struct string_list *pathspec_list)
>   {
>   	struct string_list_item *item;
> diff --git a/advice.h b/advice.h
> index bd26c385d0..1624043838 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -95,6 +95,7 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...);
>   int error_resolve_conflict(const char *me);
>   void NORETURN die_resolve_conflict(const char *me);
>   void NORETURN die_conclude_merge(void);
> +void NORETURN die_ff_impossible(void);
>   void advise_on_updating_sparse_paths(struct string_list *pathspec_list);
>   void detach_advice(const char *new_name);
>   
> diff --git a/builtin/merge.c b/builtin/merge.c
> index a8a843b1f5..aa920ac524 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1620,7 +1620,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>   	}
>   
>   	if (fast_forward == FF_ONLY)
> -		die(_("Not possible to fast-forward, aborting."));
> +		die_ff_impossible();
>   
>   	if (autostash)
>   		create_autostash(the_repository,
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 3e13f81084..d979660482 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -1046,9 +1046,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>   
>   	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
>   
> -	if (rebase_unspecified && !opt_ff && !can_ff) {
> -		if (opt_verbosity >= 0)
> -			show_advice_pull_non_ff();
> +	if (!can_ff) {
> +		if (opt_ff) {
> +			if (!strcmp(opt_ff, "--ff-only"))
> +				die_ff_impossible();
> +		} else {
> +			if (rebase_unspecified && opt_verbosity >= 0)
> +				show_advice_pull_non_ff();
> +		}
>   	}
>   
>   	if (opt_rebase) {
> diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
> index 52e8ccc933..b5a09a60f9 100755
> --- a/t/t7601-merge-pull-config.sh
> +++ b/t/t7601-merge-pull-config.sh
> @@ -183,6 +183,30 @@ test_expect_success 'pull prevents non-fast-forward with "only" in pull.ff' '
>   	test_must_fail git pull . c3
>   '
>   
> +test_expect_success 'pull prevents non-fast-forward with pull.ff=only and pull.rebase=true' '
> +	git reset --hard c1 &&
> +	test_config pull.ff only &&
> +	test_config pull.rebase true &&
> +	test_must_fail git pull . c3
> +'
> +
> +test_expect_success 'pull prevents non-fast-forward with pull.ff=only and pull.rebase=false' '
> +	git reset --hard c1 &&
> +	test_config pull.ff only &&
> +	test_config pull.rebase false &&
> +	test_must_fail git pull . c3
> +'
> +
> +test_expect_success 'pull prevents non-fast-forward with --rebase --ff-only' '
> +	git reset --hard c1 &&
> +	test_must_fail git pull --rebase --ff-only . c3
> +'
> +
> +test_expect_success 'pull prevents non-fast-forward with --no-rebase --ff-only' '
> +	git reset --hard c1 &&
> +	test_must_fail git pull --no-rebase --ff-only . c3
> +'
> +
>   test_expect_success 'merge c1 with c2 (ours in pull.twohead)' '
>   	git reset --hard c1 &&
>   	git config pull.twohead ours &&
> 

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Felipe Contreras]

Phillip Wood wrote:
> On 11/07/2021 02:26, Alex Henrie wrote:
> > The warning about pulling without specifying how to reconcile divergent
> > branches says that after setting pull.rebase to true, --ff-only can
> > still be passed on the command line to require a fast-forward. Make that
> > actually work.
> 
> Thanks for revising this patch, I like this approach much better. I do 
> however have some concerns about the interaction of pull.ff with the 
> rebase config and command line options. I'd naively expect the following 
> behavior (where rebase can fast-forward if possible)
> 
>    pull.ff  pull.rebase  commandline  action
>     only     not false                rebase

Agreed. (pull.ff applies only for --merge)

>     only     not false   --no-rebase  fast-forward only

Agreed. (--no-rebase is --merge, and pull.ff applies)

>      *       not false    --ff-only   fast-forward only

Disagree. (--ff-only is for --merge)

We would need to change the documentation and the advice warning for
this to be correct.

>     only     not false    --ff        merge --ff

Disagree.

This is a rebase, --ff should be ignored.

Junio already proposed --ff and other options to imply a merge [1], but
I already explained why that is problematic [2].

>     only     not false    --no-ff     merge --no-ff

Disagree. (ditto)

>     only       false                  fast-forward only
>     only       false      --rebase    rebase
>     only       false      --ff        merge --ff
>     only       false      --no-ff     merge --no-ff

Agreed.

> I don't think enforcing fast-forward only for rebases makes sense unless 
> it is given on the command line.

But why? This is inconsistent.

Everywhere else in git the configuration is another way of specifying
the command line. This would be the first instance where it would not be
the case.

> If the user gives `--rebase` `--ff-only` on the command line then we
> should either error out or take the last one in which case `pull
> --rebase --ff-only` would fast-forward only but `pull --ff-only
> --rebase` would rebase.

Following the same logic `pull --ff-only --merge` would ignore the
previous --ff-only, wouldn't it?


This is a pretty significant semantic change and nowhere in this patch
it's explained who this is supposed to help, or what is the motivtion
behind it.

[1] https://lore.kernel.org/git/xmqqmtyf8hfm.fsf@gitster.c.googlers.com/
[2] https://lore.kernel.org/git/5fd8aa6a52e81_190cd7208c8@natae.notmuch/

-- 
Felipe Contreras

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Alex Henrie]

On Mon, Jul 12, 2021 at 4:21 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 11/07/2021 02:26, Alex Henrie wrote:
> > The warning about pulling without specifying how to reconcile divergent
> > branches says that after setting pull.rebase to true, --ff-only can
> > still be passed on the command line to require a fast-forward. Make that
> > actually work.
>
> Thanks for revising this patch, I like this approach much better. I do
> however have some concerns about the interaction of pull.ff with the
> rebase config and command line options. I'd naively expect the following
> behavior (where rebase can fast-forward if possible)
>
>    pull.ff  pull.rebase  commandline  action
>     only     not false                rebase
>     only     not false   --no-rebase  fast-forward only
>      *       not false    --ff-only   fast-forward only
>     only     not false    --ff        merge --ff
>     only     not false    --no-ff     merge --no-ff
>     only       false                  fast-forward only
>     only       false      --rebase    rebase
>     only       false      --ff        merge --ff
>     only       false      --no-ff     merge --no-ff
>
> I don't think enforcing fast-forward only for rebases makes sense unless
> it is given on the command line. If the user gives `--rebase`
> `--ff-only` on the command line then we should either error out or take
> the last one in which case `pull --rebase --ff-only` would fast-forward
> only but `pull --ff-only --rebase` would rebase. We should also decide
> what to do when the user has pull.ff set to something other than only
> and also has pull.rebase to something other than false set - I'd guess
> we'd want to rebase unless there is a merge option on the command line
> but I haven't thought about those cases.

I was thinking of --rebase and --ff-only as orthogonal variables.
Nevertheless, we could make --rebase imply --ff, which would be pretty
easy to explain in the documentation for the command-line options.
That way, even though pull.rebase=true with pull.ff=only would enforce
fast-forward-only, the user could easily override it with `git pull
-r`. Would you accept that compromise?

-Alex

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Felipe Contreras]

Alex Henrie wrote:
> On Mon, Jul 12, 2021 at 4:21 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >
> > On 11/07/2021 02:26, Alex Henrie wrote:
> > > The warning about pulling without specifying how to reconcile divergent
> > > branches says that after setting pull.rebase to true, --ff-only can
> > > still be passed on the command line to require a fast-forward. Make that
> > > actually work.
> >
> > Thanks for revising this patch, I like this approach much better. I do
> > however have some concerns about the interaction of pull.ff with the
> > rebase config and command line options. I'd naively expect the following
> > behavior (where rebase can fast-forward if possible)
> >
> >    pull.ff  pull.rebase  commandline  action
> >     only     not false                rebase
> >     only     not false   --no-rebase  fast-forward only
> >      *       not false    --ff-only   fast-forward only
> >     only     not false    --ff        merge --ff
> >     only     not false    --no-ff     merge --no-ff
> >     only       false                  fast-forward only
> >     only       false      --rebase    rebase
> >     only       false      --ff        merge --ff
> >     only       false      --no-ff     merge --no-ff
> >
> > I don't think enforcing fast-forward only for rebases makes sense unless
> > it is given on the command line. If the user gives `--rebase`
> > `--ff-only` on the command line then we should either error out or take
> > the last one in which case `pull --rebase --ff-only` would fast-forward
> > only but `pull --ff-only --rebase` would rebase. We should also decide
> > what to do when the user has pull.ff set to something other than only
> > and also has pull.rebase to something other than false set - I'd guess
> > we'd want to rebase unless there is a merge option on the command line
> > but I haven't thought about those cases.
> 
> I was thinking of --rebase and --ff-only as orthogonal variables.
> Nevertheless, we could make --rebase imply --ff, which would be pretty
> easy to explain in the documentation for the command-line options.
> That way, even though pull.rebase=true with pull.ff=only would enforce
> fast-forward-only, the user could easily override it with `git pull
> -r`. Would you accept that compromise?

What happens if the user has configured `pull.ff=no`?

-- 
Felipe Contreras

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Thanks for revising this patch, I like this approach much better. I do
> however have some concerns about the interaction of pull.ff with the 
> rebase config and command line options. I'd naively expect the
> following behavior (where rebase can fast-forward if possible)
>
>   pull.ff  pull.rebase  commandline  action
>    only     not false                rebase
>    only     not false   --no-rebase  fast-forward only
>     *       not false    --ff-only   fast-forward only
>    only     not false    --ff        merge --ff
>    only     not false    --no-ff     merge --no-ff
>    only       false                  fast-forward only
>    only       false      --rebase    rebase
>    only       false      --ff        merge --ff
>    only       false      --no-ff     merge --no-ff

Do you mean by "not false" something other than "true"?  Are you
trying to capture what should happen when these configuration
options are unspecified as well (and your "not false" is "either set
to true or unspecified")?  I ask because the first row does not make
any sense to me.  It seems to say

    "If pull.ff is set to 'only', pull.rebase is not set to 'false',
    and the command line does not say anything, we will rebase".

I do agree with you that the command line options

    --ff-only
    --ff (aka "allow ff")
    --no-ff

should override pull.ff and

    --rebase
    --no-rebase (aka "merge")

should override pull.rebase configuration settings and also override
pull.ff set to 'only' (i.e. the user explicitly wants intregration
of the two histories and at that point "I usually just follow along"
which is "pull.ff=only" no longer applies).

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Felipe Contreras]

Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
> > Thanks for revising this patch, I like this approach much better. I do
> > however have some concerns about the interaction of pull.ff with the 
> > rebase config and command line options. I'd naively expect the
> > following behavior (where rebase can fast-forward if possible)
> >
> >   pull.ff  pull.rebase  commandline  action
> >    only     not false                rebase
> >    only     not false   --no-rebase  fast-forward only
> >     *       not false    --ff-only   fast-forward only
> >    only     not false    --ff        merge --ff
> >    only     not false    --no-ff     merge --no-ff
> >    only       false                  fast-forward only
> >    only       false      --rebase    rebase
> >    only       false      --ff        merge --ff
> >    only       false      --no-ff     merge --no-ff
> 
> Do you mean by "not false" something other than "true"?  Are you
> trying to capture what should happen when these configuration
> options are unspecified as well (and your "not false" is "either set
> to true or unspecified")?  I ask because the first row does not make
> any sense to me.  It seems to say
> 
>     "If pull.ff is set to 'only', pull.rebase is not set to 'false',
>     and the command line does not say anything, we will rebase".

No, pull.rebase can have values other than true, like "merges".

  git -c pull.ff=only -c pull.rebase=merges pull

This should rebase because pull.ff=only is meant only for --merge.

> I do agree with you that the command line options
> 
>     --ff-only
>     --ff (aka "allow ff")
>     --no-ff
> 
> should override pull.ff and

Yes.

>     --rebase
>     --no-rebase (aka "merge")
> 
> should override pull.rebase configuration settings

Yes.

> and also override pull.ff set to 'only'

No.

pull.ff=only is specifically set for when the user wants to do a merge,
*not* a rebase.

  git -c pull.ff=only pull # fast-forward merge
  git -c pull.ff=only pull --rebase # rebase
  git -c pull.ff=only pull --no-rebase # fast-forward merge

Whether the merge is implied becuse it's the default, or the user has
explicitely specified it with --no-rebase (should be --merge) does not
matter.

> (i.e. the user explicitly
> wants intregration of the two histories and at that point "I usually
> just follow along" which is "pull.ff=only" no longer applies).

Nowhere in the description of pull.ff does it say that --no-rebase turns
off pull.ff=only.

pull.ff::
	By default, Git does not create an extra merge commit when merging
	a commit that is a descendant of the current commit. Instead, the
	tip of the current branch is fast-forwarded. When set to `false`,
	this variable tells Git to create an extra merge commit in such
	a case (equivalent to giving the `--no-ff` option from the command
	line). When set to `only`, only such fast-forward merges are
	allowed (equivalent to giving the `--ff-only` option from the
	command line). This setting overrides `merge.ff` when pulling.

That would be a pretty convoluted semantic change.

And BTW, why would these four produce different results?

  git -c pull.ff=only -c pull.rebase=false pull
  git -c pull.ff=only pull --no-rebase
  git -c pull.rebase=false pull --ff-only
  git pull --ff-only --no-rebase

-- 
Felipe Contreras

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Elijah Newren]

On Mon, Jul 12, 2021 at 10:08 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> > Thanks for revising this patch, I like this approach much better. I do
> > however have some concerns about the interaction of pull.ff with the
> > rebase config and command line options. I'd naively expect the
> > following behavior (where rebase can fast-forward if possible)
> >
> >   pull.ff  pull.rebase  commandline  action
> >    only     not false                rebase
> >    only     not false   --no-rebase  fast-forward only
> >     *       not false    --ff-only   fast-forward only
> >    only     not false    --ff        merge --ff
> >    only     not false    --no-ff     merge --no-ff
> >    only       false                  fast-forward only
> >    only       false      --rebase    rebase
> >    only       false      --ff        merge --ff
> >    only       false      --no-ff     merge --no-ff
>
> Do you mean by "not false" something other than "true"?  Are you
> trying to capture what should happen when these configuration
> options are unspecified as well (and your "not false" is "either set
> to true or unspecified")?  I ask because the first row does not make
> any sense to me.  It seems to say
>
>     "If pull.ff is set to 'only', pull.rebase is not set to 'false',
>     and the command line does not say anything, we will rebase".

I think Phillip is trying to answer what to do when pull.ff and
pull.rebase conflict.  If I read his "not false" means "is set to
something other than false", then I agree with his table, but I think
he missed covering some cases.

I think his table says that pull.rebase=false cannot conflict with
pull.ff settings, but any other value for pull.rebase can.  That makes
sense to me.

I'd similarly say that pull.ff=true cannot conflict with any
pull.rebase settings...but that both pull.ff=only AND pull.ff=false
conflict with pull.rebase={true,merges}.

My opinion would be:
  * conflicting command line flags results in the last one winning.
  * --no-rebase makes pull.ff determine the action.
  * --ff makes pull.rebase determine the action.
  * any other command line flag (-r|--rebase|--no-ff|--ff-only)
overrides both pull.ff and pull.rebase
  * If no command line option is given, and pull.ff and pull.rebase
conflict, then error out.

I believe my recommendation above is consistent with every entry in
Phillip's table except the first line (where I suggest erroring out
instead).

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Felipe Contreras]

Elijah Newren wrote:
> On Mon, Jul 12, 2021 at 10:08 AM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Phillip Wood <phillip.wood123@gmail.com> writes:
> >
> > > Thanks for revising this patch, I like this approach much better. I do
> > > however have some concerns about the interaction of pull.ff with the
> > > rebase config and command line options. I'd naively expect the
> > > following behavior (where rebase can fast-forward if possible)
> > >
> > >   pull.ff  pull.rebase  commandline  action
> > >    only     not false                rebase
> > >    only     not false   --no-rebase  fast-forward only
> > >     *       not false    --ff-only   fast-forward only
> > >    only     not false    --ff        merge --ff
> > >    only     not false    --no-ff     merge --no-ff
> > >    only       false                  fast-forward only
> > >    only       false      --rebase    rebase
> > >    only       false      --ff        merge --ff
> > >    only       false      --no-ff     merge --no-ff
> >
> > Do you mean by "not false" something other than "true"?  Are you
> > trying to capture what should happen when these configuration
> > options are unspecified as well (and your "not false" is "either set
> > to true or unspecified")?  I ask because the first row does not make
> > any sense to me.  It seems to say
> >
> >     "If pull.ff is set to 'only', pull.rebase is not set to 'false',
> >     and the command line does not say anything, we will rebase".
> 
> I think Phillip is trying to answer what to do when pull.ff and
> pull.rebase conflict.  If I read his "not false" means "is set to
> something other than false", then I agree with his table, but I think
> he missed covering some cases.
> 
> I think his table says that pull.rebase=false cannot conflict with
> pull.ff settings, but any other value for pull.rebase can.  That makes
> sense to me.
> 
> I'd similarly say that pull.ff=true cannot conflict with any
> pull.rebase settings...but that both pull.ff=only AND pull.ff=false
> conflict with pull.rebase={true,merges}.
> 
> My opinion would be:
>   * conflicting command line flags results in the last one winning.
>   * --no-rebase makes pull.ff determine the action.
>   * --ff makes pull.rebase determine the action.
>   * any other command line flag (-r|--rebase|--no-ff|--ff-only)
> overrides both pull.ff and pull.rebase
>   * If no command line option is given, and pull.ff and pull.rebase
> conflict, then error out.
> 
> I believe my recommendation above is consistent with every entry in
> Phillip's table except the first line (where I suggest erroring out
> instead).

No:

  git -c pull.ff=only -c pull.rebase=true pull --ff

In Phillip's table that does a merge, in your rules that's a rebase.


Moreover, since when does git do something different depending if the
action was configured or specified in the command line? They are
supposed to be two ways of doing th same thing:

  git -c pull.ff=only -c pull.rebase=true pull
  git -c pull.ff=only pull --rebase

Why would those do different things?

The documentation is pretty clear:

  See `pull.rebase`, `branch.<name>.rebase` and `branch.autoSetupRebase` in
  linkgit:git-config[1] if you want to make `git pull` always use
  `--rebase` instead of merging.

If you start adding exceptions on top of exceptions the documentation
will become a mess.

-- 
Felipe Contreras

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Alex Henrie]

On Mon, Jul 12, 2021 at 11:51 AM Elijah Newren <newren@gmail.com> wrote:
>
> On Mon, Jul 12, 2021 at 10:08 AM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Phillip Wood <phillip.wood123@gmail.com> writes:
> >
> > > Thanks for revising this patch, I like this approach much better. I do
> > > however have some concerns about the interaction of pull.ff with the
> > > rebase config and command line options. I'd naively expect the
> > > following behavior (where rebase can fast-forward if possible)
> > >
> > >   pull.ff  pull.rebase  commandline  action
> > >    only     not false                rebase
> > >    only     not false   --no-rebase  fast-forward only
> > >     *       not false    --ff-only   fast-forward only
> > >    only     not false    --ff        merge --ff
> > >    only     not false    --no-ff     merge --no-ff
> > >    only       false                  fast-forward only
> > >    only       false      --rebase    rebase
> > >    only       false      --ff        merge --ff
> > >    only       false      --no-ff     merge --no-ff
> >
> > Do you mean by "not false" something other than "true"?  Are you
> > trying to capture what should happen when these configuration
> > options are unspecified as well (and your "not false" is "either set
> > to true or unspecified")?  I ask because the first row does not make
> > any sense to me.  It seems to say
> >
> >     "If pull.ff is set to 'only', pull.rebase is not set to 'false',
> >     and the command line does not say anything, we will rebase".
>
> I think Phillip is trying to answer what to do when pull.ff and
> pull.rebase conflict.  If I read his "not false" means "is set to
> something other than false", then I agree with his table, but I think
> he missed covering some cases.
>
> I think his table says that pull.rebase=false cannot conflict with
> pull.ff settings, but any other value for pull.rebase can.  That makes
> sense to me.
>
> I'd similarly say that pull.ff=true cannot conflict with any
> pull.rebase settings...but that both pull.ff=only AND pull.ff=false
> conflict with pull.rebase={true,merges}.
>
> My opinion would be:
>   * conflicting command line flags results in the last one winning.
>   * --no-rebase makes pull.ff determine the action.
>   * --ff makes pull.rebase determine the action.
>   * any other command line flag (-r|--rebase|--no-ff|--ff-only)
> overrides both pull.ff and pull.rebase
>   * If no command line option is given, and pull.ff and pull.rebase
> conflict, then error out.
>
> I believe my recommendation above is consistent with every entry in
> Phillip's table except the first line (where I suggest erroring out
> instead).

I'm not sure that --no-ff should imply --no-rebase because `git
rebase` actually has a --no-ff option to rewrite commits even when
fast-forwarding is possible. And it's not really necessary to make
--ff-only imply --no-rebase because we're going to make `git pull`
handle --ff-only itself without invoking `git merge`. However, the
rest of this proposal could be implemented in a straightforward manner
by making --rebase on the command line imply --ff, and I think that
would be a fine solution.

-Alex

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Alex Henrie]

On Mon, Jul 12, 2021 at 12:20 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
>
> And it's not really necessary to make
> --ff-only imply --no-rebase because we're going to make `git pull`
> handle --ff-only itself without invoking `git merge`.

Sorry, I misspoke. I was thinking of the case where fast-forwarding is
impossible. If fast-forwarding is possible, --ff-only already
effectively implies --no-rebase, and we might want to make that
explicit in the documentation.

-Alex

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

> Sorry, I misspoke. I was thinking of the case where fast-forwarding is
> impossible.

When we cannot fast-forward (i.e. we have our own development that
is not in the tip of their history),

 --ff-only would cause the operation fail
 --ff would become no-op (as it merely allows fast-forwarding)
 --no-ff would become no-op (as it merely forbids fast-forwarding)

and the latter two case, we'd either merge or rebase (with possibly
specified mode like --preserve-merges).  I thought the current
documentation is already fairly clear on this point?

> If fast-forwarding is possible, --ff-only already effectively
> implies --no-rebase, and we might want to make that explicit in
> the documentation.

When we fast-forward (i.e. their history is descendant from ours,
and the user did not give --no-ff), it does not matter if it is done
using the merge backend, the rebase backend, or by the "git pull"
wrapper. The end user does not care.  The end result is that the tip
of the branch now points at the tip of the history we pulled from
the other side and that is all what matters.

So, from that point of view, I do not think we want to say rebase or
merge or anything else for such a case in the documentation.

THanks.

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Felipe Contreras]

Junio C Hamano wrote:
> Alex Henrie <alexhenrie24@gmail.com> writes:
> 
> > Sorry, I misspoke. I was thinking of the case where fast-forwarding is
> > impossible.
> 
> When we cannot fast-forward (i.e. we have our own development that
> is not in the tip of their history),
> 
>  --ff-only would cause the operation fail
>  --ff would become no-op (as it merely allows fast-forwarding)
>  --no-ff would become no-op (as it merely forbids fast-forwarding)
> 
> and the latter two case, we'd either merge or rebase (with possibly
> specified mode like --preserve-merges).  I thought the current
> documentation is already fairly clear on this point?

Correct. But the documentation says absolutly nothing about
`--ff-only --rebase`.

> > If fast-forwarding is possible, --ff-only already effectively
> > implies --no-rebase, and we might want to make that explicit in
> > the documentation.
> 
> When we fast-forward (i.e. their history is descendant from ours,
> and the user did not give --no-ff), it does not matter if it is done
> using the merge backend, the rebase backend, or by the "git pull"
> wrapper. The end user does not care.  The end result is that the tip
> of the branch now points at the tip of the history we pulled from
> the other side and that is all what matters.
> 
> So, from that point of view, I do not think we want to say rebase or
> merge or anything else for such a case in the documentation.

Correct again. But currently the only way a fast-forward is possible is
by doing `git merge --ff-only`, which is a merge done in a fast-forward
way.

If we add my proposed `git fast-forward` [1], then the documentation
could be updated to say that --ff-only does a `git fast-forward`, and
thus there's no need for `git merge` (or `git rebase`).

[1] https://lore.kernel.org/git/20210705123209.1808663-4-felipe.contreras@gmail.com/

-- 
Felipe Contreras

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Elijah Newren]

On Mon, Jul 12, 2021 at 12:55 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > Sorry, I misspoke. I was thinking of the case where fast-forwarding is
> > impossible.
>
> When we cannot fast-forward (i.e. we have our own development that
> is not in the tip of their history),
>
>  --ff-only would cause the operation fail
>  --ff would become no-op (as it merely allows fast-forwarding)
>  --no-ff would become no-op (as it merely forbids fast-forwarding)
>
> and the latter two case, we'd either merge or rebase (with possibly
> specified mode like --preserve-merges).  I thought the current
> documentation is already fairly clear on this point?

git pull's --no-ff is documented to "create a merge commit in all
cases", and thus as worded, seems incompatible with rebasing to me.
Treating --no-ff as a no-op when we cannot fast-forward (i.e. allowing
rebasing to happen) could be seen as a backwards incompatible change
at this point.

Having --ff be compatible with rebasing works because the end result
will be the same as described in the existing documentation.

> > If fast-forwarding is possible, --ff-only already effectively
> > implies --no-rebase, and we might want to make that explicit in
> > the documentation.
>
> When we fast-forward (i.e. their history is descendant from ours,
> and the user did not give --no-ff), it does not matter if it is done
> using the merge backend, the rebase backend, or by the "git pull"
> wrapper. The end user does not care.  The end result is that the tip
> of the branch now points at the tip of the history we pulled from
> the other side and that is all what matters.
>
> So, from that point of view, I do not think we want to say rebase or
> merge or anything else for such a case in the documentation.

All three of --ff, --no-ff, and --ff-only come from
Documentation/merge-options.txt and are shared between git-merge and
git-pull.  The description of each of those items mentions "merges" or
"merging" at least once in every sentence.

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> When we cannot fast-forward (i.e. we have our own development that
>> is not in the tip of their history),
>>
>>  --ff-only would cause the operation fail
>>  --ff would become no-op (as it merely allows fast-forwarding)
>>  --no-ff would become no-op (as it merely forbids fast-forwarding)
>>
>> and the latter two case, we'd either merge or rebase (with possibly
>> specified mode like --preserve-merges).  I thought the current
>> documentation is already fairly clear on this point?
>
> git pull's --no-ff is documented to "create a merge commit in all
> cases", and thus as worded, seems incompatible with rebasing to me.

It smells like a "too literally to be useful" interpretation of a
pice of documentation that has no relevance to "pull --rebase" to
me, though.  It comes from merge-options.txt and would not be
relevant to "git pull --rebase" to begin with.

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Felipe Contreras]

Junio C Hamano wrote:
> Elijah Newren <newren@gmail.com> writes:
> 
> >> When we cannot fast-forward (i.e. we have our own development that
> >> is not in the tip of their history),
> >>
> >>  --ff-only would cause the operation fail
> >>  --ff would become no-op (as it merely allows fast-forwarding)
> >>  --no-ff would become no-op (as it merely forbids fast-forwarding)
> >>
> >> and the latter two case, we'd either merge or rebase (with possibly
> >> specified mode like --preserve-merges).  I thought the current
> >> documentation is already fairly clear on this point?
> >
> > git pull's --no-ff is documented to "create a merge commit in all
> > cases", and thus as worded, seems incompatible with rebasing to me.
> 
> It smells like a "too literally to be useful" interpretation of a
> pice of documentation that has no relevance to "pull --rebase" to
> me, though.  It comes from merge-options.txt and would not be
> relevant to "git pull --rebase" to begin with.

But Elijah's statement is correct.

`git pull --no-ff --rebase` is undocumented. Period.

-- 
Felipe Contreras

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Elijah Newren]

On Mon, Jul 12, 2021 at 4:00 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> >> When we cannot fast-forward (i.e. we have our own development that
> >> is not in the tip of their history),
> >>
> >>  --ff-only would cause the operation fail
> >>  --ff would become no-op (as it merely allows fast-forwarding)
> >>  --no-ff would become no-op (as it merely forbids fast-forwarding)
> >>
> >> and the latter two case, we'd either merge or rebase (with possibly
> >> specified mode like --preserve-merges).  I thought the current
> >> documentation is already fairly clear on this point?
> >
> > git pull's --no-ff is documented to "create a merge commit in all
> > cases", and thus as worded, seems incompatible with rebasing to me.
>
> It smells like a "too literally to be useful" interpretation of a
> pice of documentation that has no relevance to "pull --rebase" to
> me, though.  It comes from merge-options.txt and would not be
> relevant to "git pull --rebase" to begin with.

"git pull --rebase" isn't a very interesting case -- it's missing
--no-ff, and has an explicit flag declaring user intent making both
pull.ff and pull.rebase irrelevant.  It doesn't really help us handle
the tougher cases at all.

Let me back up and see if I can explain a bit better.

1. In general, we allow command line options to countermand either
configuration options or other conflicting command line options, e.g.
--no-gpg-sign can countermand --gpg-sign=$KEY or commit.gpgSign
setting.

2. Starting with simple conflicts, git-pull has two sets of them of
the above form:
  * --rebase, --no-rebase, pull.rebase
  * --no-ff, --ff-only, --ff, pull.ff

3. git-pull *also* has conflicting options across these types.
  * --rebase[={true,merges,preserve,interactive}] and --ff-only
  * possibly also --no-ff with any of those rebase flags
  * the underlying pull.rebase and pull.ff which are meant to mirror
these flags thus also can conflict

4. The way we handle conflicting options in git is typically:
  * The last command line option countermands any earlier ones on the
command line
  * Command line options countermand anything in config
  * We do not assume any ordering in config, so if two configuration
options conflict, it's either an error or we document which one
overrides the other (e.g. diff.renames=false overrides
diff.renameLimit)

So my proposal is just to do 4 above, noting that:
  * pull.rebase set to anything other than "false" conflicts with
pull.ff set to anything other than "true" (and similarly for the
equivalent command line options)
  * If both pull.rebase and pull.ff are set (in conflicting fashion)
and no countermanding command line flags are set, I recommend throwing
an error; it's not clear that having one override the other would
match user intent.

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Elijah Newren]

On Mon, Jul 12, 2021 at 11:20 AM Alex Henrie <alexhenrie24@gmail.com> wrote:
>
> On Mon, Jul 12, 2021 at 11:51 AM Elijah Newren <newren@gmail.com> wrote:
> >
> > On Mon, Jul 12, 2021 at 10:08 AM Junio C Hamano <gitster@pobox.com> wrote:
> > >
> > > Phillip Wood <phillip.wood123@gmail.com> writes:
> > >
> > > > Thanks for revising this patch, I like this approach much better. I do
> > > > however have some concerns about the interaction of pull.ff with the
> > > > rebase config and command line options. I'd naively expect the
> > > > following behavior (where rebase can fast-forward if possible)
> > > >
> > > >   pull.ff  pull.rebase  commandline  action
> > > >    only     not false                rebase
> > > >    only     not false   --no-rebase  fast-forward only
> > > >     *       not false    --ff-only   fast-forward only
> > > >    only     not false    --ff        merge --ff
> > > >    only     not false    --no-ff     merge --no-ff
> > > >    only       false                  fast-forward only
> > > >    only       false      --rebase    rebase
> > > >    only       false      --ff        merge --ff
> > > >    only       false      --no-ff     merge --no-ff
> > >
> > > Do you mean by "not false" something other than "true"?  Are you
> > > trying to capture what should happen when these configuration
> > > options are unspecified as well (and your "not false" is "either set
> > > to true or unspecified")?  I ask because the first row does not make
> > > any sense to me.  It seems to say
> > >
> > >     "If pull.ff is set to 'only', pull.rebase is not set to 'false',
> > >     and the command line does not say anything, we will rebase".
> >
> > I think Phillip is trying to answer what to do when pull.ff and
> > pull.rebase conflict.  If I read his "not false" means "is set to
> > something other than false", then I agree with his table, but I think
> > he missed covering some cases.
> >
> > I think his table says that pull.rebase=false cannot conflict with
> > pull.ff settings, but any other value for pull.rebase can.  That makes
> > sense to me.
> >
> > I'd similarly say that pull.ff=true cannot conflict with any
> > pull.rebase settings...but that both pull.ff=only AND pull.ff=false
> > conflict with pull.rebase={true,merges}.
> >
> > My opinion would be:
> >   * conflicting command line flags results in the last one winning.
> >   * --no-rebase makes pull.ff determine the action.
> >   * --ff makes pull.rebase determine the action.
> >   * any other command line flag (-r|--rebase|--no-ff|--ff-only)
> > overrides both pull.ff and pull.rebase
> >   * If no command line option is given, and pull.ff and pull.rebase
> > conflict, then error out.
> >
> > I believe my recommendation above is consistent with every entry in
> > Phillip's table except the first line (where I suggest erroring out
> > instead).
>
> I'm not sure that --no-ff should imply --no-rebase because `git
> rebase` actually has a --no-ff option to rewrite commits even when
> fast-forwarding is possible. And it's not really necessary to make
> --ff-only imply --no-rebase because we're going to make `git pull`
> handle --ff-only itself without invoking `git merge`. However, the
> rest of this proposal could be implemented in a straightforward manner
> by making --rebase on the command line imply --ff, and I think that
> would be a fine solution.

git rebase has a --no-ff but it doesn't do anything like what git
pull's --no-ff has long been documented to do.  git pull's --no-ff
very clearly states that a merge commit will be created, and thus is
tied to merging instead of rebasing.

Also, I don't think pull needs to provide a union of all merge and
rebase options; so there's no need to add a way to invoke a 'rebase
--no-ff' from pull.  (Folks can run individual fetch & merge or rebase
steps, after all.)  I think we should concentrate on making sure that
we provide reasonable behavior when conflicting options/command-lines
from the already provided set are specified.

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Felipe Contreras]

Elijah Newren wrote:

> I think we should concentrate on making sure that we provide
> reasonable behavior when conflicting options/command-lines from the
> already provided set are specified.

The only way to do that in a way that a) does not significantly alter
existing semantics, b) can be clearly documented, and c) can be
implemented without a plethora of exceptions, is to create a new
configuration orthogonal to all the existing ones:

  pull.mode=fast-forward

-- 
Felipe Contreras

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Phillip Wood]

On 12/07/2021 18:08, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> Thanks for revising this patch, I like this approach much better. I do
>> however have some concerns about the interaction of pull.ff with the
>> rebase config and command line options. I'd naively expect the
>> following behavior (where rebase can fast-forward if possible)
>>
>>    pull.ff  pull.rebase  commandline  action
>>     only     not false                rebase
>>     only     not false   --no-rebase  fast-forward only
>>      *       not false    --ff-only   fast-forward only
>>     only     not false    --ff        merge --ff
>>     only     not false    --no-ff     merge --no-ff
>>     only       false                  fast-forward only
>>     only       false      --rebase    rebase
>>     only       false      --ff        merge --ff
>>     only       false      --no-ff     merge --no-ff
> 
> Do you mean by "not false" something other than "true"?

I mean pull.rebase is set to true/interactive/merges/preserve

>  Are you
> trying to capture what should happen when these configuration
> options are unspecified as well (and your "not false" is "either set
> to true or unspecified")?  I ask because the first row does not make
> any sense to me.  It seems to say
> 
>      "If pull.ff is set to 'only', pull.rebase is not set to 'false',
>      and the command line does not say anything, we will rebase".

Yes because if pull.rebase is not false then the user wants to rebase.

> I do agree with you that the command line options
> 
>      --ff-only
>      --ff (aka "allow ff")
>      --no-ff
> 
> should override pull.ff and
> 
>      --rebase
>      --no-rebase (aka "merge")
> 
> should override pull.rebase configuration settings and also override
> pull.ff set to 'only' (i.e. the user explicitly wants intregration
> of the two histories and at that point "I usually just follow along"
> which is "pull.ff=only" no longer applies).

I'd assumed --no-rebase just meant "merge respecting pull.ff instead of 
rebasing" rather than "merge ignoring pull.ff instead of rebasing"

Best Wishes

Phillip

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Son Luong Ngoc]

Hi folks,

I am out of the loop in this thread but I have been seeing strange behaviors
with pull.rebase=true in the 'next' branch and also in the 'master'
branch in recent days.

  > git version
  git version 2.32.0.432.gabb21c7263
  > git config -l | grep pull
  pull.rebase=true
  pull.ff=false

But a git pull would still run fast-forward.
Some of our users (including myself) rely on disabling fast-forward to emit the
per-file change log summary after each git-pull

  Updating 245f278cb729..5e8d960db7b3
  Fast-forward
   some/file/dir.ext         | 44 ++++++++++++++++++++++++++++++++++++++++++++
   another/file/dir.ext     |  6 +++---
  2 files changed, 47 insertions(+), 3 deletions(-)

In a big, fast moving monorepo, this summary is a lot of noise and
switching to pull.rebase=true
used to be the way to turn it off. If the change is intended for next
version release, is there a
workaround for this?

Cheers,
Son Luong

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Felipe Contreras]

Hello,

Son Luong Ngoc wrote:
> I am out of the loop in this thread but I have been seeing strange behaviors
> with pull.rebase=true in the 'next' branch and also in the 'master'
> branch in recent days.
> 
>   > git version
>   git version 2.32.0.432.gabb21c7263
>   > git config -l | grep pull
>   pull.rebase=true
>   pull.ff=false
> 
> But a git pull would still run fast-forward.
> Some of our users (including myself) rely on disabling fast-forward to emit the
> per-file change log summary after each git-pull
> 
>   Updating 245f278cb729..5e8d960db7b3
>   Fast-forward
>    some/file/dir.ext         | 44 ++++++++++++++++++++++++++++++++++++++++++++
>    another/file/dir.ext     |  6 +++---
>   2 files changed, 47 insertions(+), 3 deletions(-)
> 
> In a big, fast moving monorepo, this summary is a lot of noise and
> switching to pull.rebase=true
> used to be the way to turn it off.

This is probably due to 340062243a (pull: cleanup autostash check,
2021-06-17).

I bet you have `rebase.autostash=true` configured as well.

It seems to me you were relying on a bug.

> If the change is intended for next
> version release, is there a
> workaround for this?

git pull -n ?

-- 
Felipe Contreras

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Elijah Newren]

On Wed, Jul 14, 2021 at 1:37 AM Son Luong Ngoc <sluongng@gmail.com> wrote:
>
> Hi folks,
>
> I am out of the loop in this thread but I have been seeing strange behaviors
> with pull.rebase=true in the 'next' branch and also in the 'master'
> branch in recent days.

I'm not surprised it happens with recent versions, but I'd expect this
to have happened with older versions too.  Is this not reproducible
with git-2.32.0 or older git versions?

>   > git version
>   git version 2.32.0.432.gabb21c7263
>   > git config -l | grep pull
>   pull.rebase=true
>   pull.ff=false

So, you have conflicting configuration options set.  pull.ff=false
maps to --no-ff which is documented to create a merge.
pull.rebase=true maps to --rebase which says to run a rebase.

You probably want to drop one of these.

> But a git pull would still run fast-forward.
> Some of our users (including myself) rely on disabling fast-forward to emit the
> per-file change log summary after each git-pull
>
>   Updating 245f278cb729..5e8d960db7b3
>   Fast-forward
>    some/file/dir.ext         | 44 ++++++++++++++++++++++++++++++++++++++++++++
>    another/file/dir.ext     |  6 +++---
>   2 files changed, 47 insertions(+), 3 deletions(-)
>
> In a big, fast moving monorepo, this summary is a lot of noise and
> switching to pull.rebase=true
> used to be the way to turn it off. If the change is intended for next
> version release, is there a
> workaround for this?

Thanks for the report.  This particular commit has not yet been picked
up, not even in seen.  But it's a good example of how conflicting
configuration really ought to result in an error rather than randomly
picking one to trump, and suggests why we should complete the patch.

However, since I'm commenting on this and the stat information appears
to be important to you, note that there are also merge.stat and
rebase.stat configuration variables for controlling whether those are
shown at the end of merge and rebase operations.

Hope that helps,
Elijah

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> However, since I'm commenting on this and the stat information appears
> to be important to you, note that there are also merge.stat and
> rebase.stat configuration variables for controlling whether those are
> shown at the end of merge and rebase operations.

True.  It is troubling that people try to avoid fast-forward (which
is a very cheap operation) and rebase their work on top of the other
side (which could unnecessarily smudge their object store with
pointless replaying of the commits) only because they want to avoid
the informational messages *and* there is an easy way to squelch
these messages.  There is some gap in documentation?

Thanks.

Re: [PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

[Felipe Contreras]

Elijah Newren wrote:
> On Wed, Jul 14, 2021 at 1:37 AM Son Luong Ngoc <sluongng@gmail.com> wrote:
> > I am out of the loop in this thread but I have been seeing strange behaviors
> > with pull.rebase=true in the 'next' branch and also in the 'master'
> > branch in recent days.
> 
> I'm not surprised it happens with recent versions, but I'd expect this
> to have happened with older versions too.  Is this not reproducible
> with git-2.32.0 or older git versions?

I already provided an accurate target [1].

> >   > git version
> >   git version 2.32.0.432.gabb21c7263
> >   > git config -l | grep pull
> >   pull.rebase=true
> >   pull.ff=false
> 
> So, you have conflicting configuration options set.  pull.ff=false
> maps to --no-ff which is documented to create a merge.
> pull.rebase=true maps to --rebase which says to run a rebase.
> 
> You probably want to drop one of these.

`pull.ff` will be honored by `git pull --merge`.

[1] https://lore.kernel.org/git/60eeff69293fb_10e52087a@natae.notmuch/

-- 
Felipe Contreras