Add semi-linear support by smartin-vossloh · Pull Request #39 · sequoia-pgp/fast-forward

smartin-vossloh · 2025-01-16T16:13:21Z

This short series adds support for semi-linear merge strategy.

To do so, the PR must be fast-forward and a merge-commit must be created for the merge.
The fast-forward action is leveraged to run the check when merging, and enhanced by adding
the --no-ff option to the git merge command when configured so.

Closes #36

nwalfield · 2025-01-16T16:35:38Z

Thanks for working on it! I want to let you know that I've seen it but I won't have time to look at it until after FOSDEM. If I don't respond by Feb 7, please feel free to ping me!

smartin-vossloh · 2025-01-17T09:40:26Z

@nwalfield Thanks for the notice :) Meanwhile, I will run a bit more tests on it until get back on the PR.

Enjoy FOSDEM! 👍 (I won't be there this year, but plan to check the videos.)

smartin-vossloh · 2025-02-03T09:31:24Z

Tests and semi-linear history example available here: https://github.com/smartin-vossloh/fast-forward/network

github-actions · 2025-02-03T09:31:39Z

Triggered from #39 (comment) by @smartin-vossloh.

Trying to fast forward main (6f66e12) to sma/no-ff-merge (f0e7200).

Target branch (main):

commit 6f66e12ccb351059900f82fcd87e8d9e9c095087 (HEAD -> main, origin/main, origin/HEAD)
Author: Angelo Gregorio Lovatto (GitHub) <12701614+0xangelo@users.noreply.github.com>
Date:   Sat Jan 25 08:36:26 2025 -0300

    docs: add Troubleshooting section to README

Pull request (sma/no-ff-merge):

commit f0e72001d3bfd44568b228166bd9b610e066fdd9 (pull_request/sma/no-ff-merge)
Author: Samuel MARTIN <samuel.martin@vossloh.com>
Date:   Fri Jan 17 09:30:56 2025 +0100

    README.md: Clean-up double-spaces

Can't fast forward main (6f66e12) to sma/no-ff-merge (f0e7200). main (6f66e12) is not a direct ancestor of sma/no-ff-merge (f0e7200). Branches appear to have diverged at 042cd23:

* 6f66e12ccb351059900f82fcd87e8d9e9c095087 docs: add Troubleshooting section to README
| * f0e72001d3bfd44568b228166bd9b610e066fdd9 README.md: Clean-up double-spaces
| * 0fc327963c5707b6a16404abf8fe8b06e110fa61 README.md: Add a note about changing the merge trigger
| * 0ece24b2cd6c2f4aa1fdfc4c24074dd32ec0d535 README.md: Add section about the generated merge commit message
| * 22c8a7724a3ff7faecda4b07de2d14178bd20183 fast-forward: Add option for no-ff merge commit generation
| * 4b9fdf3c20522231156d8163c7c4f5f412867b43 README.md: Add section about semi-linear history
| * 9d4fb137d1244c67620efd38c4f2a528ea52bcf0 fast-forward: Add --no-ff merge support
|/  
* 042cd23fbf9d5ed1400497a106c8abe4b45408ab ci: Increase debugging output.

commit 042cd23fbf9d5ed1400497a106c8abe4b45408ab
Author: Neal H. Walfield <neal@pep.foundation>
Date:   Thu Aug 31 13:42:35 2023 +0200

    ci: Increase debugging output.

Rebase locally, and then force push to sma/no-ff-merge.

smartin-vossloh · 2025-02-18T08:33:52Z

Up! 🙂

@nwalfield Do you know when you will be able to review this PR?

smartin-vossloh · 2025-03-18T13:54:03Z

@nwalfield Any news about this PR? when I could expect seeing it merged (or reworking it)?

This change allows to choose the merge strategy to use for merging the pull-request. A merge-strategy parameter is added, allowing to choose the no-ff way (i.e. introducing a merge commit). The default strategy remains fast-forwarding the branch. Fixes sequoia-pgp#36

This change introduce a workflow setting (input) allowing to chose the kind of generated merge commit. This setting offers similar choices as those available under the GitHub *Allow merge commit* option. On pull-request events, the JSON keys set by the GitHub *Allow merge commit* are available through the GitHub API, but not on PR comment event. Hence, we have to set it in the workflow itself instead of relying on the repository settings from GitHub.

smartin-vossloh · 2025-04-14T13:46:29Z

@nwalfield I'd really like to see this integrate and available in the fast-forward action. Hope you'll find sometime to review this PR.

smartin-vossloh · 2025-05-26T06:39:39Z

@nwalfield Any news about this PR?

smartin-vossloh · 2025-06-26T06:04:33Z

Hi @nwalfield 🙂

Do you have any news about this PR?
(whether it is wanted for the fast-forward action? if so do you have an idea when you will be available for reviewing this development?)

I'm looking forward to get your feedback.
TIA

smartin-vossloh · 2025-07-21T07:41:55Z

@nwalfield up! 🙂

nwalfield

First, my apologies for the long time it took me to review these changes.

Most importantly, this change is missing a unit test. The unit tests are run from this action, which calls this script. It would be good to add a second script that checks that a valid semi-linear merge works, and that an invalid one (one that can't be fast forwarded) is rejected.

The option handling should be improved. An invalid value should always be rejected, and default should be an alias for the default value.

I appreciate that the two main changes are in separate commits. I'd prefer that the documentation related to a commit be added in the same commit.

Finally, the stylistic change is unrelated to this set of changes and should be proposed in a separate MR. That said, I'm not interested in that change. I prefer having two spaces after a full stop, and I don't want the churn.

nwalfield · 2025-07-26T09:19:47Z

        then
-            ${{ github.action_path }}/src/fast-forward.sh --merge
+            ${{ github.action_path }}/src/fast-forward.sh --merge \
+                "${{ inputs.merge-method }}"


I'd prefer to have this be a named argument, e.g., --merge-stategy. This means fixing up fast-forward.sh to use getopts.

nwalfield · 2025-07-26T09:20:59Z

-            )
-            echo '```'
-            echo 0 >$EXIT_CODE
+            else


This should check that the strategy is really fast-forward. We should also add an else branch that errors out if the value of this option is not known.

nwalfield · 2025-07-26T09:24:57Z

+](https://devblogs.microsoft.com/devops/pull-requests-with-rebase/#semi-linear-merge)
+on your repository, i.e. merging a fast-forward branch with a merge
+commit, add `.github/workflows/fast-forward.yml` to your repository
+with the [following contents](.github/workflows/fast-forward.yml):


I don't think that the linked file matches the following contents.

I'm also not so excited about having an almost exact copy of the above section. I think that in the very least we should emphasize that the only difference between this workflow and the above workflow is changing to the merge-method variable, or remove the redundancy completely and just say change merge-method to xxx.

nwalfield · 2025-07-26T09:26:09Z

      indicating whether it is possible to fast forward the target
      branch.
    default: false
+  merge-method:


This doesn't match the commit message, which says that a merge-strategy parameter is added. Let's change this to merge-strategy.

nwalfield · 2025-07-26T16:34:59Z

+      When merging using the merge-commit method, set the merge commit
+      message content.
+
+      Possible values are: default, pr-title or pr-title-and-body.


The description should describe what the three options do.

nwalfield · 2025-07-26T16:35:46Z

+                MESSAGE=$(mktemp)
+                PR_NUMBER="$(github_pull_request .number)"
+                PR_HTML_URL="$(github_pull_request .html_url)"
+                # debug stuff


What does this comment mean?

nwalfield · 2025-07-26T16:37:18Z

+                            echo "* ${PR_HTML_URL}"
+                            echo "* from ${PR_REF} into ${BASE_REF}"
+                            ;;
+                        *)


This isn't great. This should be an explicitly named option or default. If the value doesn't match any of the known values, we should emit an error.

nwalfield · 2025-07-26T16:39:41Z


 This repository contains a GitHub action that merges a pull request by
-fast forwarding the target branch.  The action is triggered when an
+fast forwarding the target branch. The action is triggered when an


I'm a bit confused about this and other similar changes. It seems that this is doing the opposite of what the commit message says: instead of fixing the double spaces after full stops, it is removing them!

smartin-vossloh · 2025-09-18T08:07:01Z

Hi @nwalfield ,

First, apologies for the delay.
I am still willing to get this dev in, but priority are not here right now.

I hope I will be able to rework following your review in the coming weeks.

Thanks for your patience.

nwalfield · 2025-09-18T08:08:32Z

No worries.

smartin-vossloh force-pushed the sma/no-ff-merge branch 2 times, most recently from 89db994 to e25f92e Compare January 17, 2025 09:35

smartin-vossloh force-pushed the sma/no-ff-merge branch 3 times, most recently from 9b8f09c to f0e7200 Compare January 22, 2025 13:55

smartin-vossloh force-pushed the sma/no-ff-merge branch from f0e7200 to 8339539 Compare February 3, 2025 15:15

smartin-vossloh added 6 commits April 14, 2025 15:38

README.md: Add section about semi-linear history

e9fe53c

README.md: Add section about the generated merge commit message

efe9ffe

README.md: Add a note about changing the merge trigger

39fef6f

README.md: Clean-up double-spaces

4d956d0

smartin-vossloh force-pushed the sma/no-ff-merge branch from 8339539 to 4d956d0 Compare April 14, 2025 13:42

nwalfield mentioned this pull request Jul 25, 2025

Add semi-linear support #42

Merged

nwalfield requested changes Jul 26, 2025

View reviewed changes

Uh oh!

Conversation

smartin-vossloh commented Jan 16, 2025

Uh oh!

nwalfield commented Jan 16, 2025

Uh oh!

smartin-vossloh commented Jan 17, 2025

Uh oh!

smartin-vossloh commented Feb 3, 2025

Uh oh!

github-actions Bot commented Feb 3, 2025

Uh oh!

smartin-vossloh commented Feb 18, 2025

Uh oh!

smartin-vossloh commented Mar 18, 2025

Uh oh!

smartin-vossloh commented Apr 14, 2025

Uh oh!

smartin-vossloh commented May 26, 2025

Uh oh!

smartin-vossloh commented Jun 26, 2025

Uh oh!

smartin-vossloh commented Jul 21, 2025

Uh oh!

nwalfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smartin-vossloh commented Sep 18, 2025

Uh oh!

nwalfield commented Sep 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants