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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 21, 2021, 20:18 UTC
Message-ID: <xmqqlf5zctey.fsf@gitster.g>
In-Reply-To: <1a821d3b1ddf22b62b14d3b573015c3d8c90e2de.1626831744.git.gitgitgadget@gmail.com>

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 17 quoted lines

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

Show 12 quoted lines

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