Re: [PATCH 6/9] pull: make --rebase and --no-rebase override pull.ff=only

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 20, 2021, 23:53 UTC
Message-ID: <xmqqpmvcfsol.fsf@gitster.g>
In-Reply-To: <b379fea097d65a28f1791f7f2f9432b6689a977f.1626536508.git.gitgitgadget@gmail.com>

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

> From: Elijah Newren <newren@gmail.com>
>
> Fix the last few precedence tests failing in t7601 by now implementing
> the logic to have --[no-]rebase override a pull.ff=only config setting.

Because we only have opt_rebase and opt_ff, unless we are in a very early part of the code, we cannot tell if "opt_rebase >= 0" we see came from the command line or from the configuration, and because pull.rebase, pull.ff, --rebase=<kind>, and <--ff|--ff-only|--no-ff> have intricate interactions among them, we'd probably need to behave differently depending on where "opt_rebase >= 0" came from.

The new code works only because we haven't called config_get_rebase() yet in this part. If we swapped the order of "if ff is not set, then figure it out from the configuration" (the code you are extending) and "if rebase is not set, read from the configuration", then the logic would not work.

The resulting code looks correct, but it is subtle and may deserve an in-code comment.

Show 17 quoted lines

> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  builtin/pull.c               | 5 ++++-
>  t/t7601-merge-pull-config.sh | 4 ++--
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 5ba376a7487..7e7c90f6a10 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -966,8 +966,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>  
>  	parse_repo_refspecs(argc, argv, &repo, &refspecs);
>  
> -	if (!opt_ff)
> +	if (!opt_ff) {
>  		opt_ff = xstrdup_or_null(config_get_ff());

		/*
		 * Do we have --rebase=<kind> or --no-rebase from the
		 * command line (note that we haven't read pull.rebase yet)?
		 * If so, defeat the configured pull.ff=only; instead of
		 * failing to integrate divergent histories, we want the
		 * rebase or merge to happen.
		 */

> +		if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only"))
> +			opt_ff = "--ff";
> +	}

Thanks.