resolve: try to resolve all conflicted files in fileset #5111

scott2000 · 2024-12-16T18:26:35Z

If many files are conflicted, it would be nice to be able to resolve all conflicts at once without having to run jj resolve multiple times. This is especially nice for merge tools which try to automatically resolve conflicts without user input, but it can also be used for regular merge editors like VS Code.

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

arxanas · 2024-12-16T18:45:20Z

I recently learned that some commands support filesets. Is jj resolve one of those? Could we support this use-case by extending the fileset language, rather than adding a flag specifically to jj resolve? (Does jj resolve 'all()' work today?)

martinvonz · 2024-12-16T18:57:01Z

We actually already support filsets in this command. It's just that the command would always stop after the first conflict before this PR (so even jj resolve 'all()' would just pick the first conflict it runs into).

scott2000 · 2024-12-16T19:01:48Z

I recently learned that some commands support filesets. Is jj resolve one of those? Could we support this use-case by extending the fileset language, rather than adding a flag specifically to jj resolve? (Does jj resolve 'all()' work today?)

Yes, it already does support filesets, and the default is all() currently already. The issue is that it always picks a single file from the fileset, rather than using the whole fileset. The new --all flag just tells it to use the whole fileset, rather than picking a single file from the fileset. We could maybe make it so that if the user explicitly passes a fileset, it uses the whole fileset? I'm not sure if anyone relies on the existing behavior though.

Edit: nevermind, looks like @martinvonz already responded!

arxanas · 2024-12-16T19:09:59Z

We actually already support filsets in this command. It's just that the command would always stop after the first conflict before this PR (so even jj resolve 'all()' would just pick the first conflict it runs into).

Is this because existing merge editor work on a single file at a time, so we have no choice but to resolve conflicts individually? (Is there a warning in the case that subsequent conflicts are ignored?)

For uniformity, should we have an all: modifier (like for revsets) for filesets?

Assuming that it's a limitation of the merge editor interface, and you don't inadvertently want to resolve multiple files at once, perhaps it makes sense to warn before trying to resolve conflicts for multiple files, in the same way that we warn for certain multiple-revision operations?
I could be convinced either way, but by default, I would be inclined to keep the DSLs similar.

scott2000 · 2024-12-16T19:23:31Z

If we want the default behavior of picking a single file to be expressable with filesets, then we'd probably want a first(fileset) function to pick a single file from a fileset. I'm not sure if that would make the implementation of filesets more complicated though.

TBH, I'm not sure I've ever actually wanted the default behavior of only resolving a single, seemingly-random file though. Usually, I want to resolve every conflict, or I want to resolve a conflict in a specific file I specify. So maybe we could just change jj resolve to just resolve every file in the fileset and require the user to name a specific file if they only want to resolve a single file?

arxanas · 2024-12-16T20:29:16Z

If we want the default behavior of picking a single file to be expressable with filesets, then we'd probably want a first(fileset) function to pick a single file from a fileset. I'm not sure if that would make the implementation of filesets more complicated though.

Makes sense to me, although implementing that is its whole own matter.

TBH, I'm not sure I've ever actually wanted the default behavior of only resolving a single, seemingly-random file though. Usually, I want to resolve every conflict, or I want to resolve a conflict in a specific file I specify. So maybe we could just change jj resolve to just resolve every file in the fileset and require the user to name a specific file if they only want to resolve a single file?

In both your workflow and my workflow, it sounds like resolving a single arbitrary file is probably not desirable:

In my own workflow, I typically get scared and confused when I try to finish up and I wasn't expecting the tool to launch more editors.
- It used to be particularly difficult to resolve in cases where I was using a CLI editor to resolve conflicts, so there was no way to just go back to my terminal and type ctrl-c to cancel the whole process.
- These days, I know how to make Vim exit with an error code, but there may be other users might share my old workflow of trying to trying to quit and then press ctrl-c really fast before the caller can launch another editor 👀.
Thererfore: I would prefer an error in the case that jj resolve would operate on multiple files, while you would prefer to just start resolving multiple files, but neither of us would want the current behavior.
But, in fairness: I don't use jj resolve that much vs just opening the conflicted files in my editor and letting them get auto-resolved, so I'll defer to anyone who specifically uses jj resolve.

scott2000 · 2024-12-16T20:45:59Z

These days, I know how to make Vim exit with an error code, but there may be other users might share my old workflow of trying to trying to quit and then press ctrl-c really fast before the caller can launch another editor 👀.

I think in practice, this shouldn't be an issue because we treat an unmodified or blank output file as an error, so it would stop launching more editors if someone closes the editor without making any changes. That being said, it does feel a bit strange to launch the editor multiple times without the user requesting it explicitly, so it might be nice to not launch multiple editors by default anyway.

At some point, I'm thinking that we probably will want to support merge tools which can merge whole directories at once instead of being invoked with each file individually.

martinvonz · 2024-12-16T23:19:06Z

So maybe we could just change jj resolve to just resolve every file in the fileset and require the user to name a specific file if they only want to resolve a single file?

I'm also for that.

martinvonz · 2024-12-16T23:20:24Z

Oh, @ilyagr might also have thoughts. He wrote most of the current implementation.

martinvonz · 2024-12-19T19:44:36Z

cli/src/merge_tools/external.rs

+                    "Stopping due to error after resolving {i} conflicts"
+                )?;
+                print_error_sources(ui, Some(&err))?;
+                break;


Perhaps we should still return the error here to make it more predictable for scripts? Otherwise the exit code is a bit hard to interpret. Alternatively, we could make it not an error even after no conflicts were resolved. What do you think? It might be unlikely that scripts call jj resolve (without --list), so maybe I'm worrying about nothing.

Yeah, I was considering that as well. I mainly chose to make it a warning instead of an error because I didn't want any previous successful resolutions to be discarded (so returning an error immediately isn't an option), and I thought it looked strange having the error message be at the end of the command after the transaction commits successfully instead of being before the transaction commits. Maybe I could make it so that it prints the error immediately, but doesn't actually exit with the error code until after the transaction commits?

Maybe I could make it so that it prints the error immediately, but doesn't actually exit with the error code until after the transaction commits?

SGTM, thanks.

Hmm it ended up being more difficult to print the error immediately than I thought. It seems necessary to return a CommandError from the command, because calling std::process::exit would skip finalizing the pager, but all the current CommandError variants print a message at the end of the command. I'm not sure that printing the error immediately would be worth the complexity, so I currently changed it to just return the error like normal, but just wait until after committing the transaction.

I think it might be clear enough that some conflicts were successfully resolved, since I added an error message saying Stopped due to error after resolving <count> conflicts. What do you think?

cli/src/merge_tools/mod.rs

cli/src/commands/resolve.rs

yuja · 2024-12-20T06:40:08Z

cli/src/merge_tools/mod.rs

    pub fn edit_file(
        &self,
+        ui: &Ui,
+        path_converter: &RepoPathUiConverter,
        tree: &MergedTree,
-        repo_path: &RepoPath,
+        repo_paths: &[&RepoPath],


Just a comment. Perhaps, this will be edit(ui, tree, matcher) at some point, but more refactoring will be needed.

I'm going to change the merge tools to accept multiple files, and this will make it easier to pass all the required data about each file.

If many files are conflicted, it would be nice to be able to resolve all conflicts at once without having to run `jj resolve` multiple times. This is especially nice for merge tools which try to automatically resolve conflicts without user input, but it is also good for regular merge editors like VS Code. This change makes the behavior of `jj resolve` more consistent with other commands which accept filesets since it will use the entire fileset instead of picking an arbitrary file from the fileset. Since we don't support passing directories to merge tools yet, the current implementation just calls the merge tool repeatedly in a loop until every file is resolved, or until an error occurs. If an error occurs after successfully resolving at least one file, it is downgraded to a warning instead. This means the user can just close the editor at any point to cancel resolution on all remaining files.

scott2000 force-pushed the resolve-all branch from 29cfe83 to 11fc840 Compare December 16, 2024 19:30

scott2000 marked this pull request as draft December 17, 2024 03:17

scott2000 force-pushed the resolve-all branch from 11fc840 to 69c8240 Compare December 17, 2024 17:07

scott2000 changed the title ~~resolve: add --all option~~ resolve: try to resolve all conflicted files in fileset Dec 17, 2024

scott2000 marked this pull request as ready for review December 17, 2024 18:08

scott2000 force-pushed the resolve-all branch 2 times, most recently from dbbe3df to ecf54fa Compare December 18, 2024 20:40

martinvonz reviewed Dec 20, 2024

View reviewed changes

cli/src/merge_tools/mod.rs Outdated Show resolved Hide resolved

yuja reviewed Dec 20, 2024

View reviewed changes

scott2000 force-pushed the resolve-all branch 3 times, most recently from 0b8147c to fdb52c6 Compare December 21, 2024 18:49

scott2000 added 2 commits December 23, 2024 08:43

merge_tools: extract MergeToolFile struct

22fd444

I'm going to change the merge tools to accept multiple files, and this will make it easier to pass all the required data about each file.

scott2000 force-pushed the resolve-all branch from fdb52c6 to 7c4f747 Compare December 23, 2024 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resolve: try to resolve all conflicted files in fileset #5111

resolve: try to resolve all conflicted files in fileset #5111

scott2000 commented Dec 16, 2024

arxanas commented Dec 16, 2024

martinvonz commented Dec 16, 2024

scott2000 commented Dec 16, 2024 •

edited

Loading

arxanas commented Dec 16, 2024

scott2000 commented Dec 16, 2024 •

edited

Loading

arxanas commented Dec 16, 2024

scott2000 commented Dec 16, 2024

martinvonz commented Dec 16, 2024

martinvonz commented Dec 16, 2024

martinvonz Dec 19, 2024 •

edited

Loading

scott2000 Dec 20, 2024

martinvonz Dec 20, 2024

scott2000 Dec 21, 2024

yuja Dec 20, 2024

resolve: try to resolve all conflicted files in fileset #5111

Are you sure you want to change the base?

resolve: try to resolve all conflicted files in fileset #5111

Conversation

scott2000 commented Dec 16, 2024

Checklist

arxanas commented Dec 16, 2024

martinvonz commented Dec 16, 2024

scott2000 commented Dec 16, 2024 • edited Loading

arxanas commented Dec 16, 2024

scott2000 commented Dec 16, 2024 • edited Loading

arxanas commented Dec 16, 2024

scott2000 commented Dec 16, 2024

martinvonz commented Dec 16, 2024

martinvonz commented Dec 16, 2024

martinvonz Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

scott2000 Dec 20, 2024

Choose a reason for hiding this comment

martinvonz Dec 20, 2024

Choose a reason for hiding this comment

scott2000 Dec 21, 2024

Choose a reason for hiding this comment

yuja Dec 20, 2024

Choose a reason for hiding this comment

scott2000 commented Dec 16, 2024 •

edited

Loading

scott2000 commented Dec 16, 2024 •

edited

Loading

martinvonz Dec 19, 2024 •

edited

Loading