[v2,4/8] pull: since --ff-only overrides, handle it first

Commit Message

Elijah Newren July 21, 2021, 1:42 a.m. UTC

From: Elijah Newren <[e-mail]>

There are both merge and rebase branches in the logic, and previously
both had to handle fast-forwarding.  Merge handled that implicitly
(because git merge handles it directly), while in rebase it was
explicit.  Given that the --ff-only flag is meant to override any
--rebase or --no-rebase, make the code reflect that by handling
--ff-only before the merge-vs-rebase logic.

It turns out that this also fixes a bug for submodules.  Previously,
when --ff-only was given, the code would run `merge --ff-only` on the
main module, and then run `submodule update --recursive --rebase` on the
submodules.  With this change, we still run `merge --ff-only` on the
main module, but now run `submodule update --recursive --checkout` on
the submodules.  I believe this better reflects the intent of --ff-only
to have it apply to both the main module and the submodules.

(Sidenote: It is somewhat interesting that all merges pass `--checkout`
to submodule update, even when `--no-ff` is specified, meaning that it
will only do fast-forward merges for submodules.  This was discussed in
commit a6d7eb2c7a ("pull: optionally rebase submodules (remote submodule
changes only)", 2017-06-23).  The same limitations apply now as then, so
we are not trying to fix this at this time.)

Signed-off-by: Elijah Newren <[e-mail]>
---
 builtin/pull.c  | 24 +++++++++---------------
 t/t5520-pull.sh |  6 +++---
 2 files changed, 12 insertions(+), 18 deletions(-)

Comments

Matthias Baumgarten July 21, 2021, 7:18 p.m. UTC | #1

On 7/21/21 3:42 AM, Elijah Newren via GitGitGadget wrote:
> There are both merge and rebase branches in the logic, and previously
> both had to handle fast-forwarding.  Merge handled that implicitly
> (because git merge handles it directly), while in rebase it was
> explicit.  Given that the --ff-only flag is meant to override any
> --rebase or --no-rebase, make the code reflect that by handling
> --ff-only before the merge-vs-rebase logic.

Great. That now will work as I would expect it to.

Junio C Hamano July 21, 2021, 8:18 p.m. UTC | #2

"Elijah Newren via GitGitGadget" <[e-mail]> writes:

> -	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();
> -		}
> +	/* ff-only takes precedence over rebase */
> +	if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
> +		if (!can_ff)
> +			die_ff_impossible();
> +		opt_rebase = REBASE_FALSE;
>  	}
> +	/* If no action specified and we can't fast forward, then warn. */
> +	if (!opt_ff && rebase_unspecified && !can_ff)
> +		show_advice_pull_non_ff();

This part makes sense, but ...

> @@ -1069,13 +1069,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>  		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
>  			die(_("cannot rebase with locally recorded submodule modifications"));
>  
> -		if (can_ff) {
> -			/* we can fast-forward this without invoking rebase */
> -			opt_ff = "--ff-only";
> -			ret = run_merge();
> -		} else {
> -			ret = run_rebase(&newbase, &upstream);
> -		}
> +		ret = run_rebase(&newbase, &upstream);

... as I already pointed out, this does not seem to belong to the
change.

What makes this hunk necessary?

We used to use run_merge() to fast-forward, now we let run_rebase()
to first "checkout" their tip, which ends up to be a fast-forward in
the "can_ff" situation.  As a side effect, opt_ff gets contaminated
with the current code, but that would not affect what happens after
this part (i.e. call to rebase_submodules()).

Felipe Contreras July 21, 2021, 9:18 p.m. UTC | #3

Matthias Baumgarten wrote:
> On 7/21/21 3:42 AM, Elijah Newren via GitGitGadget wrote:
> > There are both merge and rebase branches in the logic, and previously
> > both had to handle fast-forwarding.  Merge handled that implicitly
> > (because git merge handles it directly), while in rebase it was
> > explicit.  Given that the --ff-only flag is meant to override any
> > --rebase or --no-rebase, make the code reflect that by handling
> > --ff-only before the merge-vs-rebase logic.
> 
> Great. That now will work as I would expect it to.

Yes, but what about the current users that have setup pull.ff=only
already?

They are already relying on existing behavior.

What do you think users currently expect this to do?

  [pull]
    ff = only
    rebase = true

  git pull --no-rebase

Elijah Newren July 22, 2021, 3:42 a.m. UTC | #4

On Wed, Jul 21, 2021 at 1:18 PM Junio C Hamano <[e-mail]> wrote:
>
> "Elijah Newren via GitGitGadget" <[e-mail]> writes:
>
> > -     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();
> > -             }
> > +     /* ff-only takes precedence over rebase */
> > +     if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
> > +             if (!can_ff)
> > +                     die_ff_impossible();
> > +             opt_rebase = REBASE_FALSE;
> >       }
> > +     /* If no action specified and we can't fast forward, then warn. */
> > +     if (!opt_ff && rebase_unspecified && !can_ff)
> > +             show_advice_pull_non_ff();
>
> This part makes sense, but ...
>
> > @@ -1069,13 +1069,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> >                   submodule_touches_in_range(the_repository, &upstream, &curr_head))
> >                       die(_("cannot rebase with locally recorded submodule modifications"));
> >
> > -             if (can_ff) {
> > -                     /* we can fast-forward this without invoking rebase */
> > -                     opt_ff = "--ff-only";
> > -                     ret = run_merge();
> > -             } else {
> > -                     ret = run_rebase(&newbase, &upstream);
> > -             }
> > +             ret = run_rebase(&newbase, &upstream);
>
> ... as I already pointed out, this does not seem to belong to the
> change.
>
> What makes this hunk necessary?
>
> We used to use run_merge() to fast-forward, now we let run_rebase()
> to first "checkout" their tip, which ends up to be a fast-forward in
> the "can_ff" situation.  As a side effect, opt_ff gets contaminated
> with the current code, but that would not affect what happens after
> this part (i.e. call to rebase_submodules()).

Indeed, you are right.  Sorry about that, I'll re-roll with this hunk
removed, and the modifications to t5520 pulled out as well.

Patch

diff --git a/builtin/pull.c b/builtin/pull.c
index d9796604825..92150f976cd 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1046,15 +1046,15 @@  int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	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();
-		}
+	/* ff-only takes precedence over rebase */
+	if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
+		if (!can_ff)
+			die_ff_impossible();
+		opt_rebase = REBASE_FALSE;
 	}
+	/* If no action specified and we can't fast forward, then warn. */
+	if (!opt_ff && rebase_unspecified && !can_ff)
+		show_advice_pull_non_ff();
 
 	if (opt_rebase) {
 		int ret = 0;
@@ -1069,13 +1069,7 @@  int cmd_pull(int argc, const char **argv, const char *prefix)
 		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
 			die(_("cannot rebase with locally recorded submodule modifications"));
 
-		if (can_ff) {
-			/* we can fast-forward this without invoking rebase */
-			opt_ff = "--ff-only";
-			ret = run_merge();
-		} else {
-			ret = run_rebase(&newbase, &upstream);
-		}
+		ret = run_rebase(&newbase, &upstream);
 
 		if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
 			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index e2c0c510222..4b50488141f 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -295,7 +295,7 @@  test_expect_success '--rebase (merge) fast forward' '
 	# The above only validates the result.  Did we actually bypass rebase?
 	git reflog -1 >reflog.actual &&
 	sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
-	echo "OBJID HEAD@{0}: pull --rebase . ff: Fast-forward" >reflog.expected &&
+	echo "OBJID HEAD@{0}: pull --rebase . ff (finish): returning to refs/heads/to-rebase" >reflog.expected &&
 	test_cmp reflog.expected reflog.fuzzy
 '
 
@@ -307,8 +307,8 @@  test_expect_success '--rebase (am) fast forward' '
 
 	# The above only validates the result.  Did we actually bypass rebase?
 	git reflog -1 >reflog.actual &&
-	sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
-	echo "OBJID HEAD@{0}: pull --rebase . ff: Fast-forward" >reflog.expected &&
+	sed -e "s/^[0-9a-f][0-9a-f]*/OBJID/" -e "s/[0-9a-f][0-9a-f]*$/OBJID/" reflog.actual >reflog.fuzzy &&
+	echo "OBJID HEAD@{0}: rebase finished: refs/heads/to-rebase onto OBJID" >reflog.expected &&
 	test_cmp reflog.expected reflog.fuzzy
 '