Feed of "Mathieu Fenniak" https://codeberg.org/mfenniak 2026-04-15T04:00:34+02:00 <p dir="auto">Open Source Software Developer. My hobbies include poorly maintaining too many bicycles, running distances that are too far, GM-ing TTRPGs, and, well, more software development obviously.</p> mfenniak commented on issue forgejo/discussions#457 2026-04-15T02:30:22+02:00 123240714: https://codeberg.org/forgejo/discussions/issues/457#issuecomment-13153986 PR Review Comment Placement - Advanced Testing? <p dir="auto"><a href="/fnetX" class="mention" rel="nofollow">@fnetX</a> wrote in <a href="https://codeberg.org/forgejo/discussions/issues/457#issuecomment-13152465" class="ref-issue" rel="nofollow">#457 (comment)</a>:</p> PR Review Comment Placement - Advanced Testing? <p dir="auto"><a href="/fnetX" class="mention" rel="nofollow">@fnetX</a> wrote in <a href="https://codeberg.org/forgejo/discussions/issues/457#issuecomment-13152465" class="ref-issue" rel="nofollow">#457 (comment)</a>:</p> mfenniak mfenniak@noreply.codeberg.org mfenniak approved forgejo/forgejo#12114 2026-04-14T18:49:32+02:00 123084867: https://codeberg.org/forgejo/forgejo/pulls/12114#issuecomment-13133019 [v15.0/forgejo] fix(ui): a few small runners UI fixes [v15.0/forgejo] fix(ui): a few small runners UI fixes mfenniak mfenniak@noreply.codeberg.org mfenniak commented on issue forgejo/forgejo#3571 2026-04-14T18:48:52+02:00 123084222: https://codeberg.org/forgejo/forgejo/issues/3571#issuecomment-13133004 feat: give more power to the Action TOKEN <p dir="auto"><a href="/Denux" class="mention" rel="nofollow">@Denux</a> wrote in <a href="https://codeberg.org/forgejo/forgejo/issues/3571#issuecomment-13052322" class="ref-issue" rel="nofollow">#3571 (comment)</a>:</p> feat: give more power to the Action TOKEN <p dir="auto"><a href="/Denux" class="mention" rel="nofollow">@Denux</a> wrote in <a href="https://codeberg.org/forgejo/forgejo/issues/3571#issuecomment-13052322" class="ref-issue" rel="nofollow">#3571 (comment)</a>:</p> mfenniak mfenniak@noreply.codeberg.org mfenniak commented on issue forgejo/forgejo#12121 2026-04-14T18:46:30+02:00 123082938: https://codeberg.org/forgejo/forgejo/issues/12121#issuecomment-13132938 problem: Decouple container auth from package registry, error handling <p dir="auto">As <a href="/patdyn" class="mention" rel="nofollow">@patdyn</a> pinged me on this in Forgejo Chat, I&#39;ll throw my thoughts here... but I don&#39;t think I have a lot to contribute with my current knowledge. I am much more interested in <em>authorization</em> related to packages (which I have plans to <a href="https://codeberg.org/forgejo/forgejo/issues/3571#issuecomment-12355017" rel="nofollow">extend into package-specific authorization</a>), but related to authentication...</p> <p dir="auto">I don&#39;t see a big problem with <code>verifyAuth</code> and <code>verifyContainerAuth</code>. They have to be subtly different because clients have different expectations. There&#39;s a bit of code overlap that could be reduced, but it&#39;s negligible in scope (eg. <code>if setting.Service.EnableReverseProxyAuth {</code>). There wasn&#39;t sufficient automated testing to catch the leak from container changes to other package registries -- <a href="/forgejo/forgejo/issues/11733" class="ref-issue" rel="nofollow">#11733</a> added some testing to cover that gap but didn&#39;t extensively touch every other registry, which seems reasonable but could still leave some future risks.</p> <p dir="auto">In general, affecting web, API, and packages, I don&#39;t like the fact that all our authentication mechanisms do two things: they return &#34;who the user is&#34; (great!), and then they populate the context with random other facets of the authentication (<code>&#34;IsActionsToken&#34;</code>, <code>&#34;ApiTokenScope&#34;</code>, <code>&#34;IsPasswordLogin&#34;</code>, etc.). An ideal refactor of authentication would turn this into structured data -- an <code>Authentication</code> interface that could provide all these facets of the current authentication without having to return unstructured side-data from the auth methods. The current implementation has a huge risk to me of populating some data, and then some error occurring, and then the partially populated data being used... or, a future change in the populated data being incomplete, leaving some parts of the app accessing auth data that isn&#39;t populated anymore, which could elevate permissions.</p> <p dir="auto">It would be nice if the package registry as a whole had a some separation between each registry and <code>packages/api.go</code> -- so that I could look at one registry&#39;s endpoints without being overwhelmed by every other registry. I think that would be fairly easy code to move around, while maintaining some usage of common authentication and authorization middlewares. But this doesn&#39;t feel in scope of this issue, just comes to mind while viewing this code.</p> problem: Decouple container auth from package registry, error handling <p dir="auto">As <a href="/patdyn" class="mention" rel="nofollow">@patdyn</a> pinged me on this in Forgejo Chat, I&#39;ll throw my thoughts here... but I don&#39;t think I have a lot to contribute with my current knowledge. I am much more interested in <em>authorization</em> related to packages (which I have plans to <a href="https://codeberg.org/forgejo/forgejo/issues/3571#issuecomment-12355017" rel="nofollow">extend into package-specific authorization</a>), but related to authentication...</p> <p dir="auto">I don&#39;t see a big problem with <code>verifyAuth</code> and <code>verifyContainerAuth</code>. They have to be subtly different because clients have different expectations. There&#39;s a bit of code overlap that could be reduced, but it&#39;s negligible in scope (eg. <code>if setting.Service.EnableReverseProxyAuth {</code>). There wasn&#39;t sufficient automated testing to catch the leak from container changes to other package registries -- <a href="/forgejo/forgejo/issues/11733" class="ref-issue" rel="nofollow">#11733</a> added some testing to cover that gap but didn&#39;t extensively touch every other registry, which seems reasonable but could still leave some future risks.</p> <p dir="auto">In general, affecting web, API, and packages, I don&#39;t like the fact that all our authentication mechanisms do two things: they return &#34;who the user is&#34; (great!), and then they populate the context with random other facets of the authentication (<code>&#34;IsActionsToken&#34;</code>, <code>&#34;ApiTokenScope&#34;</code>, <code>&#34;IsPasswordLogin&#34;</code>, etc.). An ideal refactor of authentication would turn this into structured data -- an <code>Authentication</code> interface that could provide all these facets of the current authentication without having to return unstructured side-data from the auth methods. The current implementation has a huge risk to me of populating some data, and then some error occurring, and then the partially populated data being used... or, a future change in the populated data being incomplete, leaving some parts of the app accessing auth data that isn&#39;t populated anymore, which could elevate permissions.</p> <p dir="auto">It would be nice if the package registry as a whole had a some separation between each registry and <code>packages/api.go</code> -- so that I could look at one registry&#39;s endpoints without being overwhelmed by every other registry. I think that would be fairly easy code to move around, while maintaining some usage of common authentication and authorization middlewares. But this doesn&#39;t feel in scope of this issue, just comes to mind while viewing this code.</p> mfenniak mfenniak@noreply.codeberg.org mfenniak commented on pull request forgejo/website#843 2026-04-14T18:18:22+02:00 123072918: https://codeberg.org/forgejo/website/pulls/843#issuecomment-13132362 Blog post for release of Forgejo v15.0 <p dir="auto">The scale of screenshots in the blog post are quite inconsistent -- screen-sized, big, bigger, screen-sized, biggest, as you scroll through. Not a big deal, but a little strange to scroll through?</p> Blog post for release of Forgejo v15.0 <p dir="auto">The scale of screenshots in the blog post are quite inconsistent -- screen-sized, big, bigger, screen-sized, biggest, as you scroll through. Not a big deal, but a little strange to scroll through?</p> mfenniak mfenniak@noreply.codeberg.org mfenniak commented on pull request forgejo/website#843 2026-04-14T18:18:20+02:00 123072819: https://codeberg.org/forgejo/website/pulls/843#issuecomment-13132281 Blog post for release of Forgejo v15.0 <p dir="auto">There are currently three sections with breaking, and so mentioning &#34;Breaking features&#34; and &#34;Breaking bug fixes&#34; (and hopefully not &#34;Breaking changes without a feature or bug label&#34;) would be warranted here.</p> Blog post for release of Forgejo v15.0 <p dir="auto">There are currently three sections with breaking, and so mentioning &#34;Breaking features&#34; and &#34;Breaking bug fixes&#34; (and hopefully not &#34;Breaking changes without a feature or bug label&#34;) would be warranted here.</p> mfenniak mfenniak@noreply.codeberg.org mfenniak commented on pull request forgejo/website#843 2026-04-14T18:18:17+02:00 123072708: https://codeberg.org/forgejo/website/pulls/843#issuecomment-13132227 Blog post for release of Forgejo v15.0 <p dir="auto">s/instructins/instructions/</p> Blog post for release of Forgejo v15.0 <p dir="auto">s/instructins/instructions/</p> mfenniak mfenniak@noreply.codeberg.org mfenniak commented on pull request forgejo/website#843 2026-04-14T18:18:13+02:00 123072519: https://codeberg.org/forgejo/website/pulls/843#issuecomment-13132197 Blog post for release of Forgejo v15.0 <p dir="auto">Alt-text: &#34;Screenshot demonstrating the form to add a git note on an existing commit&#34;</p> Blog post for release of Forgejo v15.0 <p dir="auto">Alt-text: &#34;Screenshot demonstrating the form to add a git note on an existing commit&#34;</p> mfenniak mfenniak@noreply.codeberg.org mfenniak commented on pull request forgejo/website#843 2026-04-14T18:18:10+02:00 123072387: https://codeberg.org/forgejo/website/pulls/843#issuecomment-13132113 Blog post for release of Forgejo v15.0 <p dir="auto">&#34;Alt key on <strong>the</strong> keyboard&#34;</p> Blog post for release of Forgejo v15.0 <p dir="auto">&#34;Alt key on <strong>the</strong> keyboard&#34;</p> mfenniak mfenniak@noreply.codeberg.org mfenniak commented on pull request forgejo/website#843 2026-04-14T18:18:07+02:00 123072270: https://codeberg.org/forgejo/website/pulls/843#issuecomment-13132080 Blog post for release of Forgejo v15.0 <p dir="auto">Add PR link?</p> Blog post for release of Forgejo v15.0 <p dir="auto">Add PR link?</p> mfenniak mfenniak@noreply.codeberg.org mfenniak commented on pull request forgejo/website#843 2026-04-14T18:18:03+02:00 123072156: https://codeberg.org/forgejo/website/pulls/843#issuecomment-13132287 Blog post for release of Forgejo v15.0 <p dir="auto">s/accept/accepts/</p> Blog post for release of Forgejo v15.0 <p dir="auto">s/accept/accepts/</p> mfenniak mfenniak@noreply.codeberg.org mfenniak commented on pull request forgejo/website#843 2026-04-14T18:06:16+02:00 123066669: https://codeberg.org/forgejo/website/pulls/843#issuecomment-13132011 Blog post for release of Forgejo v15.0 <p dir="auto">This link could have the <code>#repository-and-organization-access</code> anchor which is a bit more direct to the related updates.</p> Blog post for release of Forgejo v15.0 <p dir="auto">This link could have the <code>#repository-and-organization-access</code> anchor which is a bit more direct to the related updates.</p> mfenniak mfenniak@noreply.codeberg.org mfenniak commented on pull request forgejo/website#843 2026-04-14T18:04:02+02:00 123065394: https://codeberg.org/forgejo/website/pulls/843#issuecomment-13131897 Blog post for release of Forgejo v15.0 <p dir="auto">There are items that are &#34;Breaking changes without a feature or bug label&#34; in the release notes which are kinda subtle because they don&#39;t appear at the top of the milestone page. Can we fix that somehow?</p> <p dir="auto">The majority of breaking changes can be summarized here to help clarify for users if you&#39;d like, as the details in the release notes are kinda step-by-step but add up to one simple change:</p> <blockquote> <p dir="auto">Security improvements have changed the behaviour of &#34;public-only&#34; API access tokens, restricting APIs which previously weren&#39;t restricted.</p> </blockquote> <p dir="auto">(Or this can be placed in &#34;Breaking change&#34; below?)</p> Blog post for release of Forgejo v15.0 <p dir="auto">There are items that are &#34;Breaking changes without a feature or bug label&#34; in the release notes which are kinda subtle because they don&#39;t appear at the top of the milestone page. Can we fix that somehow?</p> <p dir="auto">The majority of breaking changes can be summarized here to help clarify for users if you&#39;d like, as the details in the release notes are kinda step-by-step but add up to one simple change:</p> <blockquote> <p dir="auto">Security improvements have changed the behaviour of &#34;public-only&#34; API access tokens, restricting APIs which previously weren&#39;t restricted.</p> </blockquote> <p dir="auto">(Or this can be placed in &#34;Breaking change&#34; below?)</p> mfenniak mfenniak@noreply.codeberg.org mfenniak commented on pull request forgejo/website#843 2026-04-14T17:59:12+02:00 123062928: https://codeberg.org/forgejo/website/pulls/843#issuecomment-13131741 Blog post for release of Forgejo v15.0 <p dir="auto">FIXME: Something something. I&#39;ll come back and edit this comment with a proposed summary after I read through the rest.</p> Blog post for release of Forgejo v15.0 <p dir="auto">FIXME: Something something. I&#39;ll come back and edit this comment with a proposed summary after I read through the rest.</p> mfenniak mfenniak@noreply.codeberg.org mfenniak commented on pull request forgejo/website#843 2026-04-14T17:58:43+02:00 123062589: https://codeberg.org/forgejo/website/pulls/843#issuecomment-13131729 Blog post for release of Forgejo v15.0 <p dir="auto">FIXME: Something something. <span class="emoji" aria-label="slightly smiling face" data-alias="slightly_smiling_face">🙂</span></p> Blog post for release of Forgejo v15.0 <p dir="auto">FIXME: Something something. <span class="emoji" aria-label="slightly smiling face" data-alias="slightly_smiling_face">🙂</span></p> mfenniak mfenniak@noreply.codeberg.org mfenniak opened issue forgejo/discussions#457 2026-04-14T17:55:59+02:00 123061134: https://codeberg.org/forgejo/discussions/issues/457 <p dir="auto">Between the v15 branch cut and release, I&#39;ve been dedicating most of my Forgejo-time to bug fixing, which led me into work to address the annoying experiences around PR comment placements. I&#39;ve fixed <a href="https://codeberg.org/forgejo/forgejo/projects/45480" rel="nofollow">five or so issues</a> with where comments get placed on pull requests, and diff generation attached to those comments.</p> <p dir="auto">My original plan was to backport these changes into v15, but as soon as the first PR was completed I felt uncomfortable with the risk of these changes. While they&#39;re covered by an automated testing suite that I&#39;m quite happy with (<a href="https://codeberg.org/forgejo/forgejo/src/commit/1b6e12408702c81b1ad762be02f7b304d8d73161/tests/integration/pull_review_test.go#L1075" rel="nofollow"><code>TestPullRequestCommentPlacement</code></a>), there is so much flexibility in where comments can be placed and how PRs can evolve. I can&#39;t be confident I&#39;ve captured a lot of real-world cases.</p> <p dir="auto">In general, I think the risks are:</p> <ul dir="auto"> <li>errors occur when running the new git commands which prevent creating a new comment, viewing a PR and its comments, or the internal processes to mark a comment as invalid</li> <li>alternative comment methods, other than the web ui (APIs, email), place comments at unexpected places or experience errors</li> <li>comments disappear when they shouldn&#39;t (which is already true in v15, so, <span class="emoji" aria-label="person shrugging" data-alias="shrug">🤷</span>)</li> <li>comments appear in the wrong place (which is already true in v15, so, <span class="emoji" aria-label="person shrugging" data-alias="shrug">🤷</span>)</li> </ul> <p dir="auto">While I still have one or two tweaks to make, I&#39;m starting to think about how to gain confidence on these changes while they live in the development branch. These are changes that need exposure to real users making pull requests, with other users commenting on them. Ideally those users would have enough awareness to notice things going wrong, and capacity to replay their actions enough to create and report actionable issues. An <strong>extra ideal</strong> situation would be that Forgejo developers can be provided with database extracts to support reproduction and fixing, if required. I don&#39;t think that these circumstances will arise organically with our current pre-release testing regime.</p> <p dir="auto">Here are some thoughts I have on how we could fill this gap:</p> <ul dir="auto"> <li>Run code.forgejo.org on the latest <code>forgejo</code> build (eg. same as v16.next), perpetually. <ul dir="auto"> <li>Development here has the right users with the qualities mentioned above.</li> <li>The work being done (usually on the runner, as the most active repo there) is usually complex enough.</li> <li>If issues occur like the recent docker authentication problem, then we&#39;d detect it earlier in the development cycle.</li> <li>The latest build would be the one that completed forgejo-integration/forgejo&#39;s build-release.yml, so, we do have some confidence that it fundamentally works.</li> </ul> </li> <li>Run code.forgejo.org on the latest <code>forgejo</code> build (eg. same as v16.next), but just for the v16 dev cycle. <ul dir="auto"> <li>Re-evaluate after three months, and either settle in to v16, or decide this was a good idea and continue.</li> </ul> </li> <li>Run code.forgejo.org on latest v15 + the PR comment patches <ul dir="auto"> <li>If running on <code>forgejo</code> seems like it has too high of a risk, we could meet the same testing goals by customizing the build.</li> <li>This requires some additional build &amp; release processes that seem awkward.</li> </ul> </li> <li>Community involvement -- if a community member can volunteer a Forgejo instance that meets the testing profile described above, then we could rely on them with either v16 or v15+patch testing.</li> <li>Backport all the review comment fixes to Codeberg.</li> <li>Backport all the review comment fixes to v15 before the v15 release.</li> </ul> <p dir="auto">My personal preference would depend on an implementation detail that I don&#39;t know a lot about: hypothetically, if code.forgejo.org was broken by a software update, what would happen to data.forgejo.org? If data.forgejo.org is &#34;pretty coupled&#34;, such that a broken code.forgejo.org would immediately bring down data.forgejo.org, then the risks to Forgejo Actions users would be pretty high with running pre-release software. However, if they&#39;re &#34;pretty decoupled&#34; and data.forgejo.org can survive a code.forgejo.org outage (still serve content, but not be updated, for example), then I&#39;d advocate in favour of &#34;run code.forgejo.org on the latest <code>forgejo</code> build perpetually&#34;.</p> <p dir="auto">More thoughts, suggestions, and input are very welcome. <span class="emoji" aria-label="slightly smiling face" data-alias="slightly_smiling_face">🙂</span></p> 457#PR Review Comment Placement - Advanced Testing?# mfenniak mfenniak@noreply.codeberg.org mfenniak approved forgejo/forgejo#12115 2026-04-14T17:24:42+02:00 123047979: https://codeberg.org/forgejo/forgejo/pulls/12115#issuecomment-13130889 [forgejo] fix(ui): a few small runners UI fixes [forgejo] fix(ui): a few small runners UI fixes mfenniak mfenniak@noreply.codeberg.org mfenniak deleted branch git-diff-corrections-on-comments from mfenniak/forgejo 2026-04-14T17:18:30+02:00 123045237: https://codeberg.org/mfenniak/forgejo mfenniak mfenniak@noreply.codeberg.org mfenniak merged pull request forgejo/forgejo#12107 2026-04-14T17:18:29+02:00 123044985: https://codeberg.org/forgejo/forgejo/pulls/12107 fix: when reviewing in PRs, make comments relative to viewed base &amp; head, not just viewed head fix: when reviewing in PRs, make comments relative to viewed base & head, not just viewed head mfenniak mfenniak@noreply.codeberg.org mfenniak pushed to forgejo at forgejo/forgejo 2026-04-14T17:18:26+02:00 123044910: https://codeberg.org/forgejo/forgejo/commit/179fbdb04e90a73f64e1910f8ca157fa6e1f678f <a href="https://codeberg.org/forgejo/forgejo/commit/179fbdb04e90a73f64e1910f8ca157fa6e1f678f" rel="nofollow">179fbdb04e90a73f64e1910f8ca157fa6e1f678f</a> fix: when reviewing in PRs, make comments relative to viewed base &amp; head, not just viewed head (#12107) <a href="https://codeberg.org/forgejo/forgejo/commit/179fbdb04e90a73f64e1910f8ca157fa6e1f678f">179fbdb04e90a73f64e1910f8ca157fa6e1f678f</a> fix: when reviewing in PRs, make comments relative to viewed base &amp; head, not just viewed head (#12107) mfenniak mfenniak@noreply.codeberg.org mfenniak commented on pull request forgejo/forgejo#12114 2026-04-14T16:42:46+02:00 123029787: https://codeberg.org/forgejo/forgejo/pulls/12114#issuecomment-13129833 [v15.0/forgejo] fix(ui): a few small runners UI fixes <p dir="auto"><a href="https://codeberg.org/forgejo/forgejo/pulls/12115/files#issuecomment-13129809" class="ref-issue" rel="nofollow">#12115/files (comment)</a></p> [v15.0/forgejo] fix(ui): a few small runners UI fixes <p dir="auto"><a href="https://codeberg.org/forgejo/forgejo/pulls/12115/files#issuecomment-13129809" class="ref-issue" rel="nofollow">#12115/files (comment)</a></p> mfenniak mfenniak@noreply.codeberg.org mfenniak commented on pull request forgejo/forgejo#12115 2026-04-14T16:42:14+02:00 123028965: https://codeberg.org/forgejo/forgejo/pulls/12115#issuecomment-13129809 [forgejo] fix(ui): a few small runners UI fixes <p dir="auto">I don&#39;t think this link makes sense. &#34;For configuring Forgejo Runner running in containers or advanced configurations ...&#34;, and then it links to the page that describes the process that the user just went through, and doesn&#39;t have any information on &#34;containers or advanced configurations&#34;? <a href="/aahlenst" class="mention" rel="nofollow">@aahlenst</a></p> [forgejo] fix(ui): a few small runners UI fixes <p dir="auto">I don&#39;t think this link makes sense. &#34;For configuring Forgejo Runner running in containers or advanced configurations ...&#34;, and then it links to the page that describes the process that the user just went through, and doesn&#39;t have any information on &#34;containers or advanced configurations&#34;? <a href="/aahlenst" class="mention" rel="nofollow">@aahlenst</a></p> mfenniak mfenniak@noreply.codeberg.org mfenniak commented on pull request forgejo/website#843 2026-04-14T16:36:36+02:00 123027729: https://codeberg.org/forgejo/website/pulls/843#issuecomment-13129743 Blog post for release of Forgejo v15.0 <p dir="auto">I&#39;m not planning to rewrite this -- I&#39;d like to have step-by-step &#34;how to use this&#34; content, but it probably belongs in docs to be kept up-to-date over time. <span class="emoji" aria-label="thumbs up" data-alias="+1">👍</span></p> Blog post for release of Forgejo v15.0 <p dir="auto">I&#39;m not planning to rewrite this -- I&#39;d like to have step-by-step &#34;how to use this&#34; content, but it probably belongs in docs to be kept up-to-date over time. <span class="emoji" aria-label="thumbs up" data-alias="+1">👍</span></p> mfenniak mfenniak@noreply.codeberg.org mfenniak commented on issue forgejo/website#729 2026-04-14T16:34:34+02:00 123026949: https://codeberg.org/forgejo/website/issues/729#issuecomment-13129692 Snippets for Forgejo v15.0.0 blog post <p dir="auto"><a href="/Gusted" class="mention" rel="nofollow">@Gusted</a> wrote in <a href="https://codeberg.org/forgejo/website/issues/729#issuecomment-13119690" class="ref-issue" rel="nofollow">#729 (comment)</a>:</p> Snippets for Forgejo v15.0.0 blog post <p dir="auto"><a href="/Gusted" class="mention" rel="nofollow">@Gusted</a> wrote in <a href="https://codeberg.org/forgejo/website/issues/729#issuecomment-13119690" class="ref-issue" rel="nofollow">#729 (comment)</a>:</p> mfenniak mfenniak@noreply.codeberg.org mfenniak approved forgejo/forgejo#12111 2026-04-14T04:23:43+02:00 122803683: https://codeberg.org/forgejo/forgejo/pulls/12111#issuecomment-13104165 chore: don&#39;t load settings twice for running `web` <p dir="auto">Seems reasonable to me. I think that manual testing is probably the only practical approach -- and as you&#39;ve noted, the install page functioning is a good edge case to check.</p> chore: don't load settings twice for running `web` <p dir="auto">Seems reasonable to me. I think that manual testing is probably the only practical approach -- and as you&#39;ve noted, the install page functioning is a good edge case to check.</p> mfenniak mfenniak@noreply.codeberg.org mfenniak created pull request forgejo/forgejo#12107 2026-04-13T18:33:48+02:00 122629578: https://codeberg.org/forgejo/forgejo/pulls/12107 <p dir="auto">While developing tests for <a href="/forgejo/forgejo/issues/12092" class="ref-issue" rel="nofollow">#12092</a>, I came across a case where making a comment on a single-commit doesn&#39;t include the correct diff for the comment. This is because code comment placement occurs between the PR&#39;s base and the commit being viewed, but, that diff could be different from the commit&#39;s parent to the commit, which is what is being viewed on a single-commit diff.</p> <p dir="auto">Similar to <a href="/forgejo/forgejo/issues/12055" class="ref-issue" rel="nofollow">#12055</a>, this PR changes code comments to be more precise in their diff generation by providing the backend with both the base commit (<code>before_commit_id</code>) and head commit (<code>after_commit_id</code>) currently being viewed. As a result, the diffs attached to comments should exactly match the diffs being viewed by the user when the comment was placed.</p> <h2 id="user-content-checklist" dir="auto">Checklist</h2> <p dir="auto">The <a href="https://forgejo.org/docs/next/contributor/" rel="nofollow">contributor guide</a> contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo&#39;s <a href="https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md" rel="nofollow">AI Agreement</a>. There also are a few <a href="https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md" rel="nofollow">conditions for merging Pull Requests in Forgejo repositories</a>. You are also welcome to join the <a href="https://matrix.to/#/%23forgejo-development:matrix.org" rel="nofollow">Forgejo development chatroom</a>.</p> <h3 id="user-content-tests-for-go-changes" dir="auto">Tests for Go changes</h3> <ul dir="auto"> <li>I added test coverage for Go changes... <ul dir="auto"> <li class="task-list-item"><input type="checkbox" disabled="" data-source-position="1384"/>in their respective <code>*_test.go</code> for unit tests.</li> <li class="task-list-item"><input type="checkbox" disabled="" data-source-position="1440" checked=""/>in the <code>tests/integration</code> directory if it involves interactions with a live Forgejo server.</li> </ul> </li> <li>I ran... <ul dir="auto"> <li class="task-list-item"><input type="checkbox" disabled="" data-source-position="1552" checked=""/><code>make pr-go</code> before pushing</li> </ul> </li> </ul> <h3 id="user-content-documentation" dir="auto">Documentation</h3> <ul dir="auto"> <li class="task-list-item"><input type="checkbox" disabled="" data-source-position="1606"/>I created a pull request <a href="https://codeberg.org/forgejo/docs" rel="nofollow">to the documentation</a> to explain to Forgejo users how to use this change.</li> <li class="task-list-item"><input type="checkbox" disabled="" data-source-position="1747" checked=""/>I did not document these changes and I do not expect someone else to do it.</li> </ul> <h3 id="user-content-release-notes" dir="auto">Release notes</h3> <ul dir="auto"> <li class="task-list-item"><input type="checkbox" disabled="" data-source-position="1849" checked=""/>This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change.</li> <li class="task-list-item"><input type="checkbox" disabled="" data-source-position="2002"/>This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change.</li> </ul> <h2 id="user-content-release-notes-1" dir="auto">Release notes</h2> <ul dir="auto"> <li>Bug fixes <ul dir="auto"> <li><a href="https://codeberg.org/forgejo/forgejo/pulls/12107" rel="nofollow">PR</a>: when reviewing in PRs, make comments relative to viewed base &amp; head, not just viewed head</li> </ul> </li> </ul> 12107#fix: when reviewing in PRs, make comments relative to viewed base & head, not just viewed head# mfenniak mfenniak@noreply.codeberg.org mfenniak pushed to git-diff-corrections-on-comments at mfenniak/forgejo 2026-04-13T18:33:38+02:00 122629509: https://codeberg.org/mfenniak/forgejo/commit/7f466f148d156af882540a3b6a16cea42505b081 <a href="https://codeberg.org/mfenniak/forgejo/commit/7f466f148d156af882540a3b6a16cea42505b081" rel="nofollow">7f466f148d156af882540a3b6a16cea42505b081</a> fix: attach diff fragment to PR comment based upon viewed diff <a href="https://codeberg.org/mfenniak/forgejo/commit/7f466f148d156af882540a3b6a16cea42505b081">7f466f148d156af882540a3b6a16cea42505b081</a> fix: attach diff fragment to PR comment based upon viewed diff mfenniak mfenniak@noreply.codeberg.org mfenniak pushed to git-diff-corrections-on-comments at mfenniak/forgejo 2026-04-13T18:27:22+02:00 122626419: https://codeberg.org/mfenniak/forgejo/compare/9becef8574a6330f3c3d48c37d6875ddd1d71308...cd3687d9563ae60f2208578ab38fd620b77abda9 <a href="https://codeberg.org/mfenniak/forgejo/commit/cd3687d9563ae60f2208578ab38fd620b77abda9" rel="nofollow">cd3687d9563ae60f2208578ab38fd620b77abda9</a> fix: attach diff fragment to PR comment based upon viewed diff <a href="https://codeberg.org/mfenniak/forgejo/commit/92781ee898bd22efe0ef3cf273b955227edc26c2" rel="nofollow">92781ee898bd22efe0ef3cf273b955227edc26c2</a> test: comments on specific commits snapshot diffs at time of commit <a href="https://codeberg.org/mfenniak/forgejo/commit/a797a71dea127edd3d93d976131d21d93988c246" rel="nofollow">a797a71dea127edd3d93d976131d21d93988c246</a> fix: display code comments on removed lines-of-code to correct locations in PR view (#12092) <a href="https://codeberg.org/mfenniak/forgejo/commit/86898a7d0570205eeb751711426db2f1113ca659" rel="nofollow">86898a7d0570205eeb751711426db2f1113ca659</a> Revert &#34;Improve repo file list table semantics for screen readers (#11846)&#34; (#12088) <a href="https://codeberg.org/mfenniak/forgejo/commit/a1be012c4a1e99cfc482a723ebb32af26baad65c" rel="nofollow">a1be012c4a1e99cfc482a723ebb32af26baad65c</a> Lock file maintenance (forgejo) (#12101) <a href="https://codeberg.org/mfenniak/forgejo/commit/cd3687d9563ae60f2208578ab38fd620b77abda9">cd3687d9563ae60f2208578ab38fd620b77abda9</a> fix: attach diff fragment to PR comment based upon viewed diff <a href="https://codeberg.org/mfenniak/forgejo/commit/92781ee898bd22efe0ef3cf273b955227edc26c2">92781ee898bd22efe0ef3cf273b955227edc26c2</a> test: comments on specific commits snapshot diffs at time of commit <a href="https://codeberg.org/mfenniak/forgejo/commit/a797a71dea127edd3d93d976131d21d93988c246">a797a71dea127edd3d93d976131d21d93988c246</a> fix: display code comments on removed lines-of-code to correct locations in PR view (#12092) <a href="https://codeberg.org/mfenniak/forgejo/commit/86898a7d0570205eeb751711426db2f1113ca659">86898a7d0570205eeb751711426db2f1113ca659</a> Revert &#34;Improve repo file list table semantics for screen readers (#11846)&#34; (#12088) <a href="https://codeberg.org/mfenniak/forgejo/commit/a1be012c4a1e99cfc482a723ebb32af26baad65c">a1be012c4a1e99cfc482a723ebb32af26baad65c</a> Lock file maintenance (forgejo) (#12101) mfenniak mfenniak@noreply.codeberg.org mfenniak pushed to forgejo at forgejo/forgejo 2026-04-13T18:27:10+02:00 122625831: https://codeberg.org/forgejo/forgejo/commit/a797a71dea127edd3d93d976131d21d93988c246 <a href="https://codeberg.org/forgejo/forgejo/commit/a797a71dea127edd3d93d976131d21d93988c246" rel="nofollow">a797a71dea127edd3d93d976131d21d93988c246</a> fix: display code comments on removed lines-of-code to correct locations in PR view (#12092) <a href="https://codeberg.org/forgejo/forgejo/commit/a797a71dea127edd3d93d976131d21d93988c246">a797a71dea127edd3d93d976131d21d93988c246</a> fix: display code comments on removed lines-of-code to correct locations in PR view (#12092) mfenniak mfenniak@noreply.codeberg.org mfenniak deleted branch git-blame-removed-lines-relocation from mfenniak/forgejo 2026-04-13T18:27:01+02:00 122625339: https://codeberg.org/mfenniak/forgejo mfenniak mfenniak@noreply.codeberg.org