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.

Sign up for GitHub

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

Jump to bottom

Surface / clarify / better expose modify/delete conflict #896

Closed
xmo-odoo opened this issue Jun 21, 2024 · 2 comments
Closed

Surface / clarify / better expose modify/delete conflict #896

xmo-odoo opened this issue Jun 21, 2024 · 2 comments
Labels
mergebot

Comments

@xmo-odoo
Copy link
Collaborator

xmo-odoo commented Jun 21, 2024 •
edited
Loading

This is a broad issue if mostly hit by the JS team I would think:

  • in branch A, have a file f1
  • in branch B (descendant of A), delete f1
  • update f1 in a PR of A
  • when PR-A is forward-ported, it's going to "resurrect" f1, with a modify/delete conflict but no conflict marker

Example: odoo/odoo#167615 where the conflict was notified but odoo/odoo@c237341 clearly shows that addons/web/static/tests/legacy/views/kanban/kanban_view_tests.js is just added

Because there are no conflict markers it won't be blocking any sort of CI or anything, the PR will be detached and there will be an error message from the forwardbot, but it will still be easy to miss that the file was reintroduced, and a dev can easily fix other conflicts, not notice the new file, and merge that, reintroducing a weird dead file.

That's not great.

Various possible options (which may be combined?):

  • try to find if git can surface modfy/delete conflicts better couldn't find anything, and SO didn't surface anything either
    • manually insert conflict marker in the lazarus file if no?
  • parse git's conflict output to make a note of modify/delete conflicts
  • compare the total diffstats, it should be possible for a cherrypick to have (a lot) less lines than the original, but is there any way it has more?
  • have the mergebot keep track of conflicting files (mapping files to blobs), and block approval / progress until they've all been replaced or removed?
    • that might actually be a pretty good one: on SYNC trigger a cron to retrieve the HEAD commit and see if the CONFLICT blobs are still present, remove those which are not from the list of conflicts; on r+ check that the head is sync'd and verify that the CONFLICT list is empty.
  • have the mergebot check files added and if they match between source and target (show --name-status, or show --diff-filter=A --name-only lists only added files): I don't see a scenario where a file can be added by a merge / cherrypick without that exact same file being added by the source
@github-project-automation github-project-automation bot moved this to ideas in Mergebot Jun 21, 2024
@xmo-odoo xmo-odoo moved this from ideas to accepted in Mergebot Jun 21, 2024
@xmo-odoo xmo-odoo moved this from accepted to done in Mergebot Sep 27, 2024
xmo-odoo added a commit that referenced this issue Sep 27, 2024
@xmo-odoo
[IMP] forwardport: surfacing of modify/delete conflicts
8f27773 
Given branch A, and branch B forked from it. If B removes a file which
a PR to A later modifies, on forward port Git generates a
modify/delete conflict (as in one side modifies a file which the
other deleted).

So far so good, except while it does notify the caller of the issue
the modified file is just dumped as-is into the working copy (or
commit), which essentially resurrects it.

This is an issue, *especially* as the file is already part of a
commit (rather tan just a U local file), if the developer fixes the
conflict "in place" rather than re-doing the forward-port from scratch
it's easy to miss the reincarnated file (and possibly the changes that
were part of it too), which at best leaves parasitic dead code in the
working copy. There is also no easy way for the runbot to check it as
adding unimported standalone files while rare is not unknown
e.g. utility scripts (to say nothing of JS where we can't really track
the usages / imports at all).

To resolve this issue, during conflict generation post-process
modify/delete to insert artificial conflict markers, the file should
be syntactically invalid so linters / checkers should complain, and
the minimal check has a step looking for conflict markers which should
fire and prevent merging the PR.

Fixes #896
@xmo-odoo
Copy link
Collaborator Author

@aab-odoo should be deployed, hopefully the next such conflict will be easier to notice.

@aab-odoo
Copy link

Thanks @xmo-odoo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
mergebot
Projects
Mergebot
Status: done
Milestone
No milestone
Development

No branches or pull requests
2 participants
@xmo-odoo @aab-odoo