Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that all unmerged files are merged when conflict checking #20528

Merged
merged 3 commits into from
Jul 28, 2022

Conversation

zeripath
Copy link
Contributor

There is a subtle bug in the code relating to collating the results of
git ls-files -u -z in unmergedFiles(). The code here makes the
mistake of assuming that every unmerged file will always have a stage 1
conflict, and this results in conflicts that occur in stage 3 only being
dropped.

This PR simply adjusts this code to ensure that any empty unmergedFile
will always be passed down the channel.

The PR also adds a lot of Trace commands to attempt to help find future
bugs in this code.

Fix #19527

Signed-off-by: Andrew Thornton art27@cantab.net

There is a subtle bug in the code relating to collating the results of
`git ls-files -u -z` in `unmergedFiles()`. The code here makes the
mistake of assuming that every unmerged file will always have a stage 1
conflict, and this results in conflicts that occur in stage 3 only being
dropped.

This PR simply adjusts this code to ensure that any empty unmergedFile
will always be passed down the channel.

The PR also adds a lot of Trace commands to attempt to help find future
bugs in this code.

Fix go-gitea#19527

Signed-off-by: Andrew Thornton <art27@cantab.net>
Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 LOC bug fix, 26 lines for debugging code. LGTM!

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jul 28, 2022
@zeripath
Copy link
Contributor Author

It would be better if I could come up with some tests but it seems it would be very difficult. Certainly it appears it would require us to store opaque git repositories which is not really very helpful.

There are quite a few little functions here that could lend themselves to a simple test case or two if someone were willing but I only think such testcases could hope to prevent regressions rather than actually spot errors. This particular problem was an assumption that stage 1 was always present so any testcase would have had the same assumption.

@Gusted
Copy link
Contributor

Gusted commented Jul 28, 2022

This whole part of the codebase is quite untested and I remember we had a attempt at adding test for this when another bug arises(the "empty" PR one IIRC) from the conflict checking, but it's just too complicated to not end up hunderds of LOCs for one test-case.

@GiteaBot GiteaBot removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jul 28, 2022
@GiteaBot GiteaBot added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Jul 28, 2022
@6543
Copy link
Member

6543 commented Jul 28, 2022

🚀

@6543 6543 merged commit 7a428fa into go-gitea:main Jul 28, 2022
6543 pushed a commit to 6543-forks/gitea that referenced this pull request Jul 29, 2022
…gitea#20528)

There is a subtle bug in the code relating to collating the results of
`git ls-files -u -z` in `unmergedFiles()`. The code here makes the
mistake of assuming that every unmerged file will always have a stage 1
conflict, and this results in conflicts that occur in stage 3 only being
dropped.

This PR simply adjusts this code to ensure that any empty unmergedFile
will always be passed down the channel.

The PR also adds a lot of Trace commands to attempt to help find future
bugs in this code.

Fix go-gitea#19527

Signed-off-by: Andrew Thornton <art27@cantab.net>
@6543 6543 added the backport/done All backports for this PR have been created label Jul 29, 2022
@6543
Copy link
Member

6543 commented Jul 29, 2022

-> #20536

@zeripath zeripath deleted the fix-19527-prevent-missed-unmerged branch July 29, 2022 13:02
6543 added a commit that referenced this pull request Jul 29, 2022
) (#20536)

There is a subtle bug in the code relating to collating the results of
`git ls-files -u -z` in `unmergedFiles()`. The code here makes the
mistake of assuming that every unmerged file will always have a stage 1
conflict, and this results in conflicts that occur in stage 3 only being
dropped.

This PR simply adjusts this code to ensure that any empty unmergedFile
will always be passed down the channel.

The PR also adds a lot of Trace commands to attempt to help find future
bugs in this code.

Fix #19527

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: zeripath <art27@cantab.net>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 1, 2022
* giteaofficial/main: (29 commits)
  [skip ci] Updated translations via Crowdin
  Support localized README (go-gitea#20508)
  Clean up and fix clone button script (go-gitea#20415)
  Add disable download source configuration (go-gitea#20548)
  Fix default merge style (go-gitea#20564)
  Update login methods in package docs (go-gitea#20561)
  Add missing Tabs on organisation/package view (Frontport go-gitea#20539) (go-gitea#20540)
  [skip ci] Updated licenses and gitignores
  Add setting `SQLITE_JOURNAL_MODE` to enable WAL (go-gitea#20535)
  Rework file highlight rendering and fix yaml copy-paste (go-gitea#19967)
  Add new API endpoints for push mirrors management (go-gitea#19841)
  WebAuthn CredentialID field needs to be increased in size (go-gitea#20530)
  Add latest commit's SHA to content response (go-gitea#20398)
  Improve token and secret key generation docs (go-gitea#20387)
  [skip ci] Updated translations via Crowdin
  Rework raw file http header logic (go-gitea#20484)
  Update lunny/levelqueue to prevent NPE when reads are performed after close (go-gitea#20534)
  Added guidance on file to choose to download (go-gitea#20474)
  [skip ci] Updated translations via Crowdin
  Ensure that all unmerged files are merged when conflict checking (go-gitea#20528)
  ...
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 10, 2022
…gitea#20528)

There is a subtle bug in the code relating to collating the results of
`git ls-files -u -z` in `unmergedFiles()`. The code here makes the
mistake of assuming that every unmerged file will always have a stage 1
conflict, and this results in conflicts that occur in stage 3 only being
dropped.

This PR simply adjusts this code to ensure that any empty unmergedFile
will always be passed down the channel.

The PR also adds a lot of Trace commands to attempt to help find future
bugs in this code.

Fix go-gitea#19527

Signed-off-by: Andrew Thornton <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CompareAndPullRequestPost() causes internal server error
4 participants