| Message ID | pull.1599.git.git.1698224280816.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | merge: --ff-one-only to apply FF if commit is one | expand |
Hi On Wed, Oct 25, 2023, at 10:58, Ruslan Yakauleu via GitGitGadget wrote: > 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. I do something similar for my pull requests: - If more than one commit: `--no-ff` - One commit: rebase/squash Your change would (according to the commit message) only *not* create a merge if it so happens to be the case that you can fast-forward the branch. So it would create a merge commit if (say) your branch was three commits behind and one ahead (your commit) of the target branch. But isn’t it fine to just rebase/squash in this case? That seems more general. (Maybe `--squash-one-only`.) But it seems that your new option would work nicely with “semi-linear” merges:[1] - Rebase on the target branch - `--ff-one-only` - Now you either get: - a “semi-linear merge”; or - a single commit (as opposed to a merge which joins together a branch which is 0 behind and 1 ahead of the target branch)
Hi If we use squash we will lose some context and occasionally, we need multiple small features combined into one big release. We would rather not mix it into one monolithic non-readable blob. For us, sometimes it is better to rebase something to make history more accurate than squash everything into one commit. We can use squash only for one story. Anyway, squash is a different feature. Same as rebase (of course we're doing the rebase before merge to clarify history and to make some regression tests) Then we have a set of branches. Some branches contain only one commit. For example, we have 4 branches: - two commits - one commit - one commit - three commits With --no-ff (only merges) we have next graph with extra merges (E', F') for branches with one commit B---C E F G---H---I / \ / \ / \ / \ A-------D---E'--F'----------J With --ff (fast-forward everything) we miss merge branches (D, J) and it's harder to fast revert some problematic releases properly, because it's not clear that commits G-H-I - is one release A---B---C---E---F---G---H---I Story which --ff-one-only should build without manual control B---C G---H---I / \ / \ A-------D---E---F-----------J There we have merge commits (D, J) only for complex branches. Branches E and F fast-forwarded to prevent extra merges. Sorry if my explanation isn't clear enough -- Ruslan
On Wed, Oct 25, 2023, at 20:31, Ruslan Yakauleu wrote: > Hi > > If we use squash we will lose some context and occasionally, we need > multiple small features combined into one big release. We would rather > not mix it into one monolithic non-readable blob. The only context you lose is the point where your branch started, if that matters. (Later in the email you say that you rebase before you merge so apparently it does not matter to you.) To squash one single commit doesn’t make a “monolithic” commit—it is exactly as monolithic as the commit you started with. > For us, sometimes it is better to rebase something to make history more > accurate than squash everything into one commit. We can use squash only > for one story. No, squash one single commit. Not multiple. > Anyway, squash is a different feature. Same as rebase (of course we're > doing the rebase before merge to clarify history and to make some > regression tests) Squash and rebase are functionally identical in this case. And since you rebase before merge anyway it is identical to the flow of 1. Rebase before merge 2. Merge with your new option Except you don’t have to split it into two steps.
> Squash and rebase are functionally identical in this case. Sorry, but for me `git merge --squash` doesn't work. Currently, I have a global option --no-ff for master $ git config branch.master.mergeoptions --no-ff In this way `git merge --squash` crashes with message > fatal: options '--squash' and '--no-ff.' cannot be used together In other way it writes something like > Fast-forward > Squash commit -- not updating HEAD and... not updates parent branch. For example, GitHub propose for PR's: - Create a merge commit - the same as `git merge --no-ff` - Rebase and merge - the same as `git rebase ...; git merge --ff-only` - Squash AND commit - like two different operations. So after squash we still have to merge our commit properly into parent branch. The new option just dynamically selects between --ff and --no-ff for `git merge`. Nothing else.
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
Taylor Blau <me@ttaylorr.com> writes: > 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. Another more fundamental objection is "Why do we special case only a singleton commit?" Why isn't a trivial two-patch series also OK to fast-forward? Three? There is no inherent reason to draw a line on one commit topic---given that a single commit could be a large and involved one that could have been a multi commmit series. And you cannot decide if the "topic" is large enough to deserve a binding merge commit even if it is a single commit topic, or if is small enough and you want to allow fast-forward, without looking at it first. So from that point of view, too, I do not think this new option is a good idea. Let's not add an option to encourage a bad discipline. Thanks.
Hi Junio,
Command of developers can decide which policy use and why.
If a team decides to avoid extra merges - why not?
> a single commit could be a large and involved
Create one non-reviewable commit or losing feature scope is much worse
practice
For me `git log --graph` is the most representative form to describe
what's going on with a project and I prefer not to complicate it. But
it must show changes properly.
--ff-one-only is handy for keeping the appearance of a linear commit
history and makes merge commit messages (from other branches) more
meaningful because they actually do represent some branch being folded
into the master.
As an example, merge commit for one commit is overkill and doesn't bring
new meaning. All one-commit meanings described in the commit message. In
this case extra branching (extra merges) just adds confusion. Plenty of
extra merges turns into merge hell when you have countless merge messages,
but you can't get sense what's going on with project.
People use rebase and fast-forward to make commit history more linear,
avoid merge hell and keep the repository tree clear - it's fine.
But it's a big mistake to merge complex features as one non-reviewable
commit. And even worse to make all history absolutely linear when you
can't find where a complex feature starts and where it finishes. If it
wasn't combined in a special branch, it looks in the main branch like a
set of different, not connected features.
In the case of problems with absolutely linear history, you don't know
how many commits you have to revert.
In case when one commit (merge commit for complex features) brings all
changes into the main branch - you know that you can revert feature or,
for complex changes, even part of a specific feature. You can revert just
A/B tests for example, if you don't need them anymore but keep complex
features - this case makes squash even more useless.
You can try to use branch names to find where each branch merged into the
main branch, but it's not as descriptive as a graph and every so often you
can lose branch names. As an example, GitLab has an option "Delete source
branch when merge request accepted" and many teams use it.
--
Ruslan
Hi Taylor, Thank you so much for the review and feedback. We use a mixed environment (Linux, MacOS and Windows) on multiple projects. And some members can work some days from home and some days from the office (on different PC). So, using of scripts a bit uncomfortable. Better to apply once for everybody in the domain git config --global merge.ff one-only I'll try to fill the proper test a bit later. -- Ruslan
Ruslan Yakauleu <ruslan.yakauleu@gmail.com> writes: > Command of developers can decide which policy use and why. > If a team decides to avoid extra merges - why not? I never said "--ff" is a bad option that should not exist, and I am not sure why I had to be thrown such a rhetorical question at.
On Tue, Oct 31, 2023, at 01:19, Junio C Hamano wrote: > Another more fundamental objection is "Why do we special case only a > singleton commit?" > > Why isn't a trivial two-patch series also OK to fast-forward? > Three? There is no inherent reason to draw a line on one commit > topic---given that a single commit could be a large and involved one > that could have been a multi commmit series. I think it’s about the `--first-parent` view. Then you get a one-commit view of each pull request that was merged. For a merge commit it serves as a summary of multiple commits. And a merge commit of one commit would serve as a summary of one commit. So in that case you remove that extra level of indirection. > And you cannot decide if the "topic" is large enough to deserve a > binding merge commit even if it is a single commit topic, or if is > small enough and you want to allow fast-forward, without looking at > it first. But the pull request is already given: it either has one commit or several. And you can for sure look at it and either argue that it should be reduced (squashed) to one commit or maybe expanded (split out) into several commits. The point at which you use such a merge feature is when you are already happy with the pull request (or patch series or whatever else). And then you (according to this strategy) prefer to avoid merge commits for single-commit pull requests. — — This is not an argument for making this a built-in strategy. Just an explanation from my vantage point.
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes: > I think it’s about the `--first-parent` view. Then you get a one-commit > view of each pull request that was merged. For a merge commit it serves as > a summary of multiple commits. And a merge commit of one commit would > serve as a summary of one commit. So in that case you remove that extra > level of indirection. Yeah, I understand that position very well. After all, I was heavily involved in the introduction of the first-parent view to the system at around 0053e902 (git-log --first-parent: show only the first parent log, 2007-03-13). Soon after that, d66424c4 (git-merge: add --ff and --no-ff options, 2007-09-24) added --no-ff to make it easier to maintain the first-parent worldview. Strictly speaking, the log message on a merge commit serves two purposes, one is to summarize commit(s) on the side branch that gets merged with the merge, and as you said above, it is not needed when merging a topic with just one commit. But the other is to justify why the topic suits the objective of the line of history (which is needed even when merging a single commit topic---imagine a commit that is not incorrect per-se. It may or may not be suitable for the maintenance track, and a merge commit of such a commit into the track can explain if/how the commit being merged is maint-worthy). A project that does not need the latter can do without a "--no-ff" merge of a single commit topic. > But the pull request is already given: it either has one commit or > several. And you can for sure look at it and either argue that it should > be reduced (squashed) to one commit or maybe expanded (split out) into > several commits. > > The point at which you use such a merge feature is when you are already > happy with the pull request (or patch series or whatever else). And then > you (according to this strategy) prefer to avoid merge commits for > single-commit pull requests. But that argues against the "--ff-one-only" option, doesn't it? If you looked at the side branch before you decide to merge it, you know if the topic has only one commit (in which case you decide not to use "--no-ff"), or if the topic consists of multiple commits (in which case you decide to use "--no-ff"). And the only effect to have the "--ff-one-only" option is to allow you *not* to look at what is on the side branch. Thanks.
> And the only effect to have the "--ff-one-only" option is to allow > you *not* to look at what is on the side branch. Moreover, it allows not to forget choose the right way and allow use a lot of external tools (GitExtensions for example) as not all in command are console ninja Currently, when complex features merged with --ff accidentally we have to do in main branch something like $ git reset ... $ git checkout <some_old_commit_which_we_have_to_find> $ git merge --no-ff <latest commit> $ git push --force And that's what I prefer to avoid -- Ruslan
On Wed, Nov 1, 2023, at 02:42, Junio C Hamano wrote: > Strictly speaking, the log message on a merge commit serves two > purposes, one is to summarize commit(s) on the side branch that gets > merged with the merge, and as you said above, it is not needed when > merging a topic with just one commit. But the other is to justify > why the topic suits the objective of the line of history (which is > needed even when merging a single commit topic---imagine a commit > that is not incorrect per-se. It may or may not be suitable for the > maintenance track, and a merge commit of such a commit into the > track can explain if/how the commit being merged is maint-worthy). Yes. If you have multiple release/maintenance branches which you need to apply something to then you can’t use this . >> The point at which you use such a merge feature is when you are already >> happy with the pull request (or patch series or whatever else). And then >> you (according to this strategy) prefer to avoid merge commits for >> single-commit pull requests. > > But that argues against the "--ff-one-only" option, doesn't it? > > If you looked at the side branch before you decide to merge it, you > know if the topic has only one commit (in which case you decide not > to use "--no-ff"), or if the topic consists of multiple commits (in > which case you decide to use "--no-ff"). And the only effect to > have the "--ff-one-only" option is to allow you *not* to look at > what is on the side branch. No. The only effect is that you streamline the process of “decide not to use `--no-ff`” since the strategy does it for you. It would act like a small `git my-merge` alias. That is all.
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes: > On Wed, Nov 1, 2023, at 02:42, Junio C Hamano wrote: >> Strictly speaking, the log message on a merge commit serves two >> purposes, one is to summarize commit(s) on the side branch that gets >> merged with the merge, and as you said above, it is not needed when >> merging a topic with just one commit. But the other is to justify >> why the topic suits the objective of the line of history (which is >> needed even when merging a single commit topic---imagine a commit >> that is not incorrect per-se. It may or may not be suitable for the >> maintenance track, and a merge commit of such a commit into the >> track can explain if/how the commit being merged is maint-worthy). > > Yes. If you have multiple release/maintenance branches which you need to > apply something to then you can’t use this . OK. I do not mind a feature to help maintain the first-parent worldview better to exist, but have a few comments on the patch. * Nowhere in the name of feature --ff-one-only, the proposed commit log message, added documentation and in-code comments, it is made clear to readers that it is to maintain the first-parent view better. The "first-parent" was only brought up between you and I as our conjecture on what the feature is for. The should explain the feature a bit better to our readers and users. SIDE NOTE: in general, it is not the best way to name and explain a feature after what it does (e.g., "fast-forward only when it has one commit"); it is better to include why the user want it to do what it does. It it especially true because "fast-forward only when the other branch is ahead by one commit" may later turn out not to be the best design to ensure "maintain first-parent worldview", if the latter is what the feature is really about. * The proposed commit log message needs a bit of proofreading and polishing, paying attention to the grammar. * The "allow fast-forward only when the other branch is ahead by one commit" design misses an important case you would want to, and you can detect easily, fast-forward. Imagine that a developer has a rather complex topic with multiple commits, asks the maintainer (or the auto-merger at their forge) to pull, but due to modification on the upstream side, there are heavy conflicts. The maintainer can tell (and Git was designed to support this mode of operation better---it is called "distributed development") the developer: Since you know your topic much better than I do, can you do the merge into the upstream for me? The contributor would then help the maintainer, perhaps like so: $ git checkout origin/main $ git merge [--no-ff] my-topic to pretend as if the contributer were the maintainer, merge and resolve the conflicts, and then summarizes the topic in the log message of the merge commit. The contributor then updates their topic locally, perhaps with $ git push . HEAD:my-topic which would of course fast-forward, and then ask the maintainer (or the auto-merger at their forge) to pull again from "my-topic". Now, the updated "my-topic" is ahead of the origin by many commits (i.e., the number of commits on the topic, plus the merge commit the controbutor created to help the maintainer), but if we want to see the resulting history as if the original pull request was handled with the "--ff-one-only" option by the maintainer who did the merge themself, then we should fast-forward this merge. Even though the tip commit of "my-topic" has more commits behind it, it is already the binding merge of the side topic that "--ff-one-only" would have forced to create if the maintainer did the merge. So, a better design than "allow fast-forward, only if the branch being merged is ahead by one commit" is to allow fast-forward when the branch's first-parent is the current tip of the branch pull/merge is trying to update. "only by one commit" can be handled as a natural degenerate case of this more general criteria, and a good thing is that it is much easier and more efficient to compute (i.e., in "git merge OTHER", allow ff if "OTHER^1" and "HEAD" are the same). As I said, I do not mind a feature to help maintain the first-parent worldview better to exist; thanks for working on the topic.
diff --git a/Documentation/config/merge.txt b/Documentation/config/merge.txt index 8851b6cedef..6cd5daa4d64 100644 --- a/Documentation/config/merge.txt +++ b/Documentation/config/merge.txt @@ -31,6 +31,9 @@ merge.ff:: a case (equivalent to giving the `--no-ff` option from the command line). When set to `only`, only such fast-forward merges are allowed (equivalent to giving the `--ff-only` option from the + command line). When set to `one-only`, fast-forward merge allowed + only for one commit, in other way extra merge commit should be + created (equivalent to giving the `--ff-one-only` option from the command line). merge.verifySignatures:: diff --git a/builtin/merge.c b/builtin/merge.c index d748d46e135..100f0021e56 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -110,7 +110,8 @@ static const char *pull_twohead, *pull_octopus; enum ff_type { FF_NO, FF_ALLOW, - FF_ONLY + FF_ONLY, + FF_ONE_ONLY }; static enum ff_type fast_forward = FF_ALLOW; @@ -258,6 +259,7 @@ static struct option builtin_merge_options[] = { N_("edit message before committing")), OPT_CLEANUP(&cleanup_arg), OPT_SET_INT(0, "ff", &fast_forward, N_("allow fast-forward (default)"), FF_ALLOW), + OPT_SET_INT(0, "ff-one-only", &fast_forward, N_("allow fast-forward if only one commit"), FF_ONE_ONLY), OPT_SET_INT_F(0, "ff-only", &fast_forward, N_("abort if fast-forward is not possible"), FF_ONLY, PARSE_OPT_NONEG), @@ -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; } /* 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; + } + } + update_ref("updating ORIG_HEAD", "ORIG_HEAD", &head_commit->object.oid, NULL, 0, UPDATE_REFS_DIE_ON_ERR);