Re: [PATCH] merge: --ff-one-only to apply FF if commit is one
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Oct 30, 2023, 20:01 UTC
- Message-ID
- <ZUALkdSJZ70+KBYq@nand.local>
- In-Reply-To
- <pull.1599.git.git.1698224280816.gitgitgadget@gmail.com>
Hi Ruslan,
On Wed, Oct 25, 2023 at 08:58:00AM +0000, Ruslan Yakauleu via GitGitGadget wrote:
Show 6 quoted lines
> 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...
Show 6 quoted lines
> @@ -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.
Show 17 quoted lines
> } /* 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