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

merge_tools: add merge-tool-conflict-exit-codes option #4981

Merged

Conversation

scott2000
Copy link
Collaborator

@scott2000 scott2000 commented Nov 26, 2024

Some Git merge drivers can partially resolve conflicts, leaving some conflict markers in the output file. In that case, they exit with a code between 1 and 127 to indicate that there was a conflict remaining in the file (since Git uses a shell to run the merge drivers, and shells often use exit codes above 127 for special meanings such as exiting due to a signal):

https://git-scm.com/docs/gitattributes#_defining_a_custom_merge_driver

We should support this convention as well, since it will allow many Git merge drivers to be used with Jujutsu, but we don't run our merge tools through a shell, so there is no reason to treat exit codes 1 through 127 specially. Instead, we let the user specify which exact exit codes should indicate conflicts. This is also better for cross-platform support, since Windows may use different exit codes than Linux for instance.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@scott2000 scott2000 force-pushed the merge-tool-error-status-on-conflict branch from 5bc7b4d to a042e13 Compare November 26, 2024 21:02
@scott2000 scott2000 marked this pull request as ready for review November 26, 2024 21:04
@scott2000 scott2000 force-pushed the merge-tool-error-status-on-conflict branch from a042e13 to 1b3e5cf Compare November 26, 2024 22:32
@scott2000 scott2000 force-pushed the merge-tool-error-status-on-conflict branch from 1b3e5cf to cc19790 Compare December 3, 2024 01:25
@scott2000 scott2000 changed the title merge_tools: add merge-tool-error-status-on-conflict config option merge_tools: add merge-tool-conflict-exit-codes option Dec 3, 2024
Copy link
Collaborator

@yuja yuja left a comment

Choose a reason for hiding this comment

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

Thanks!

cli/src/merge_tools/external.rs Outdated Show resolved Hide resolved
cli/src/merge_tools/external.rs Show resolved Hide resolved
@scott2000 scott2000 force-pushed the merge-tool-error-status-on-conflict branch from cc19790 to f461cac Compare December 4, 2024 01:39
Some Git merge drivers can partially resolve conflicts, leaving some
conflict markers in the output file. In that case, they exit with a code
between 1 and 127 to indicate that there was a conflict remaining in the
file (since Git uses a shell to run the merge drivers, and shells often
use exit codes above 127 for special meanings such as exiting due to a
signal):

https://git-scm.com/docs/gitattributes#_defining_a_custom_merge_driver

We should support this convention as well, since it will allow many Git
merge drivers to be used with Jujutsu, but we don't run our merge tools
through a shell, so there is no reason to treat exit codes 1 through 127
specially. Instead, we let the user specify which exact exit codes
should indicate conflicts. This is also better for cross-platform
support, since Windows may use different exit codes than Linux for
instance.
@scott2000 scott2000 force-pushed the merge-tool-error-status-on-conflict branch from f461cac to a86d940 Compare December 4, 2024 01:56
@scott2000 scott2000 merged commit fa7d9c0 into martinvonz:main Dec 4, 2024
18 checks passed
@scott2000 scott2000 deleted the merge-tool-error-status-on-conflict branch December 4, 2024 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants