jj should be able to handle conflicts in executable scripts #1279
Comments
|
Definitely. I happened to be reminded of that while looking at that code just an hour or two ago. We can probably have this function here return whether the merged file should be executable: https://github.com/martinvonz/jj/blob/3dfedf581458f88982289f58db884bc65f1e635b/lib/src/conflicts.rs#L130-L144 Interested in attempting that? |
|
Oh, in the interactive case (which is what you actually asked about), the code is here: So we'd probably want |
Maybe... I've studied some rust in the past so it's not something I'm very familiar with. But I do want to contribute :) |
|
You'll also need to consider the case when one of the sides is executable and the other isn't. At first, I would suggest just aborting in that case, just like |
Yes, that's fine to do at first. It should be easy to add support for merging the executable bit in the common case of a regular 3-way merge, however. |
We discussed this on Discord, and it turns out that this situation is simpler than I thought. At least for 3-way merges, you know whether the base had the executable bit set, so it's a conflict between one side that didn't change it and another side that did change it. The latter can simply win. |
|
A related feature would be a Without such a command, tests for conflict resolution of executable bits would be harder to write and would have to be specific to POSIX systems. Update: This was done in #1693 |
|
I was looking at it a bit, but I'm not particularly confident it's as simple as the comments suggest. IIUC there's a few parts:
What is the end goal here? Do we want the user to look at the conflicted file, and be able to determine what the conflict is about? Or do we expect they may need to |
|
|
Thank you :) Good analysis. I'll answer them out of order.
I don't think we should embed that information in the file contents. And yes, I think
Regardless of the executable bit, we should write the conflict markers to the file. If there was a conflict in the executable bit, I guess it makes most sense to make the file not executable. Otherwise (no conflict in the executable bit), we should use that file mode.
When the conflicts in the file have been resolved, we need to figure out what to do with the executable bit. If the file is now executable on disk, it means that we had made it executable because there was no conflict, or the user had made it executable later. Either way, I suppose we should record the conflict as resolved with the executable bit set . Otherwise we need to look at the conflict from the tree again. If the executable bit can be automatically resolved, we should use that value on Windows or the value on disk on Unix. Otherwise (conflict in exec bit), we'll have to leave the whole conflict unresolved. And as Ilya said,
That's so you can partially resolve a conflict and continue to work on the resolution later. I don't think you need to worry about it in the context of this bug.
I suspect it's fine to ignore the executable bit in most of those cases, such as when displaying a diff. |
|
Changing the materialize code seemed simple enough, but I couldn't shake the feeling I'm missing something so was sporadically digging a bit into code instead of sending out PRs. I guess I'll take a look again once #2406 is in and maybe send something simple and let the reviewers worry instead. |
Sounds good. We're happy to help find any missing pieces during review. |
AFAICT, all callers of `Merge::to_file_merge()` are already well prepared for working with executable files. It's called from these places: * `local_working_copy.rs`: Materialized conflicts are correctly updated using `Merge::with_new_file_ids()`. * `merge_tools/`: Same as above. * `cmd_cat()`: We already ignore the executable bit when we print non-conflicted files, so it makes sense to also ignore it for conflicted files. * `git_diff_part()`: We print all conflicts with mode "100644" (the mode for regular files). Maybe it's best to use "100755" for conflicts that are unambiguously executable, or maybe it's better to use a fake mode like "000000" for all conflicts. Either way, the current behavior seems fine. * `diff_content()`: We use the diff content in various diff formats. We could add more detail about the executable bits in some of them, but I think the current output is fine. For example, instead of our current "Created conflict in my-file", we could say "Created conflict in executable file my-file" or "Created conflict in ambiguously executable file my-file". That's getting verbose, though. So, I think all we need to do is to make `Merge::to_file_merge()` not require its inputs to be non-executable. Closes #1279.
AFAICT, all callers of `Merge::to_file_merge()` are already well prepared for working with executable files. It's called from these places: * `local_working_copy.rs`: Materialized conflicts are correctly updated using `Merge::with_new_file_ids()`. * `merge_tools/`: Same as above. * `cmd_cat()`: We already ignore the executable bit when we print non-conflicted files, so it makes sense to also ignore it for conflicted files. * `git_diff_part()`: We print all conflicts with mode "100644" (the mode for regular files). Maybe it's best to use "100755" for conflicts that are unambiguously executable, or maybe it's better to use a fake mode like "000000" for all conflicts. Either way, the current behavior seems fine. * `diff_content()`: We use the diff content in various diff formats. We could add more detail about the executable bits in some of them, but I think the current output is fine. For example, instead of our current "Created conflict in my-file", we could say "Created conflict in executable file my-file" or "Created conflict in ambiguously executable file my-file". That's getting verbose, though. So, I think all we need to do is to make `Merge::to_file_merge()` not require its inputs to be non-executable. Closes #1279.
AFAICT, all callers of `Merge::to_file_merge()` are already well prepared for working with executable files. It's called from these places: * `local_working_copy.rs`: Materialized conflicts are correctly updated using `Merge::with_new_file_ids()`. * `merge_tools/`: Same as above. * `cmd_cat()`: We already ignore the executable bit when we print non-conflicted files, so it makes sense to also ignore it for conflicted files. * `git_diff_part()`: We print all conflicts with mode "100644" (the mode for regular files). Maybe it's best to use "100755" for conflicts that are unambiguously executable, or maybe it's better to use a fake mode like "000000" for all conflicts. Either way, the current behavior seems fine. * `diff_content()`: We use the diff content in various diff formats. We could add more detail about the executable bits in some of them, but I think the current output is fine. For example, instead of our current "Created conflict in my-file", we could say "Created conflict in executable file my-file" or "Created conflict in ambiguously executable file my-file". That's getting verbose, though. So, I think all we need to do is to make `Merge::to_file_merge()` not require its inputs to be non-executable. Closes #1279.
Description
Currently, if
jjnotices that a file that results in a conflict is an executable it won't bother helping the user resolve it, even if it's a script.Steps to Reproduce the Problem
.shfor example, marked withchmod +x)jj resolveExpected Behavior
As it's quite usual to edit such scripts,
jjshould allow for conflict resolution.Actual Behavior
jjerrors out of the conflict resolution.The text was updated successfully, but these errors were encountered: