Re: [PATCH RFC] rebase: respect --ff-only option

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 5, 2021, 09:27 UTC
Message-ID: <87a6n16r1l.fsf@evledraar.gmail.com>
In-Reply-To: <20210705044505.666977-1-alexhenrie24@gmail.com>

On Sun, Jul 04 2021, Alex Henrie wrote:

> I'm sending this patch as an RFC because I'm sure someone will find at
> least one thing that needs to be changed before it's committed.

FWIW that's true of most non-RFC patches :)

Show 5 quoted lines

> +int error_ff_impossible(void)
> +{
> +	error(_("Not possible to fast-forward, aborting."));
> +	return -1;
> +}

Here's one, the idiom is just "return error", i.e it itself returns -1.

FWIW I'd consider doing:
	
	/* earlier, static scope */
	static const char error_ff_impossible = N_("Not possible...");
	/* later, function scope */
	    return error(error_ff_impossible);

It's a small difference, but for translators who use the jump-to-source while translating not having the indirection helps, and in this case it's just used 3 times...

Show 9 quoted lines

> [...]
>  	if (parent && parse_commit(parent) < 0)
>  		/* TRANSLATORS: The first %s will be a "todo" command like
> @@ -2764,7 +2769,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
>  	else if (!strcmp(key, "options.record-origin"))
>  		opts->record_origin = git_config_bool_or_int(key, value, &error_flag);
>  	else if (!strcmp(key, "options.allow-ff"))
> -		opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
> +		opts->fast_forward = git_config_bool_or_int(key, value, &error_flag) ? FF_ALLOW : FF_NO;

Since we're on nits, we try to wrap at 80 characters.

Show 5 quoted lines

> +test_expect_success "rebase: impossible fast-forward rebase" '
> +	test_config rebase.autostash true &&
> +	git reset --hard &&
> +	echo dirty >>file1 &&
> +	(git rebase --ff-only unrelated-onto-branch || true) &&

Never "||" git commands, better as test_might_fail. We don't want to hide segfaults.

I saw Phillip Wood had some comments on the rest, I didn't test this, just skimmed it, but generally LGTM from a glance, not getting down to the nuts of more deeply inspecting the logic.