Re: [PATCH] merge: --ff-one-only to apply FF if commit is one

[Taylor Blau <me@ttaylorr.com>]

Hi Ruslan,

On Wed, Oct 25, 2023 at 08:58:00AM +0000, Ruslan Yakauleu via GitGitGadget wrote:
> From: Ruslan Yakauleu <ruslan.yakauleu@gmail.com>
>
> A new option --ff-one-only to control the merging strategy.
> For one commit option works like -ff to avoid extra merge commit.
> In other cases the option works like --no-ff to create merge commit for
> complex features.

This seems like a pretty niche feature to want to introduce a new option
for. I would imagine the alternative is something like:

    ff="--no-ff"
    if test 1 -eq $(git rev-list @{u}..)
    then
        ff="--ff"
    fi

    [on upstream @{u}]
    git merge "$ff" "$branch"

I don't have a great sense of how many users might want or benefit from
something like this. My sense is that there aren't many, but I could
very easily be wrong here.

In any case, my sense is that this is probably too niche to introduce a
new command-line option just to implement this behavior when the above
implementation is pretty straightforward. Regardless, here's my review
of the patch...

> @@ -631,6 +633,8 @@ static int git_merge_config(const char *k, const char *v,
>  			fast_forward = boolval ? FF_ALLOW : FF_NO;
>  		} else if (v && !strcmp(v, "only")) {
>  			fast_forward = FF_ONLY;
> +		} else if (v && !strcmp(v, "one-only")) {
> +			fast_forward = FF_ONE_ONLY;

The configuration handling and documentation all look good.

>  		} /* do not barf on values from future versions of git */
>  		return 0;
>  	} else if (!strcmp(k, "merge.defaulttoupstream")) {
> @@ -1527,6 +1531,18 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		free(list);
>  	}
>
> +	if (fast_forward == FF_ONE_ONLY) {
> +		fast_forward = FF_NO;
> +
> +		/* check that we have one and only one commit to merge */
> +		if (squash || ((!remoteheads->next &&
> +				!common->next &&
> +				oideq(&common->item->object.oid, &head_commit->object.oid)) &&
> +				oideq(&remoteheads->item->parents->item->object.oid, &head_commit->object.oid))) {
> +			fast_forward = FF_ALLOW;
> +		}

And this rather long conditional looks right, too. This patch could
definitely benefit from some tests, though...

Thanks,
Taylor