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

"Elijah Newren via GitGitGadget" [off-list ref] 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 ...

... 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()).

On Wed, Jul 21, 2021 at 1:18 PM Junio C Hamano [off-list ref] wrote:

"Elijah Newren via GitGitGadget" [off-list ref] writes:

quoted

-     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 ...

quoted

@@ -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.