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

cli: New --changes-in argument to jj restore #1298

Merged
merged 2 commits into from
Aug 1, 2023
Merged

Conversation

ilyagr
Copy link
Contributor

@ilyagr ilyagr commented Feb 20, 2023

This argument can restore a merge commit to the merge of its parents. This allows one to apply the default behavior of jj restore to another commit. In other words, jj restore without arguments is equivalent to jj restore -c @.

Original description (before switching -r => --rp)

The title was "cmd: Add -r argument to `jj restore

The interface is now similar to jj diffedit, jj diff, etc.

Checklist

If applicable:

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

@ilyagr ilyagr marked this pull request as ready for review February 20, 2023 21:36
@ilyagr ilyagr force-pushed the restore-r branch 9 times, most recently from 84f2d0e to ff003ba Compare February 21, 2023 04:24
src/commands/mod.rs Outdated Show resolved Hide resolved
src/commands/mod.rs Show resolved Hide resolved
tests/test_global_opts.rs Show resolved Hide resolved
src/commands/mod.rs Outdated Show resolved Hide resolved
@ilyagr ilyagr force-pushed the restore-r branch 2 times, most recently from 99513af to 142b183 Compare February 26, 2023 21:58
@ilyagr ilyagr marked this pull request as draft March 5, 2023 05:42
@ilyagr ilyagr force-pushed the restore-r branch 2 times, most recently from 7c33eaa to eceb1aa Compare March 15, 2023 03:41
@ilyagr ilyagr force-pushed the restore-r branch 4 times, most recently from f86eea3 to 5a1aa56 Compare April 5, 2023 01:42
@ilyagr ilyagr force-pushed the restore-r branch 3 times, most recently from c7c37a4 to 558e3de Compare May 24, 2023 05:52
@ilyagr
Copy link
Contributor Author

ilyagr commented Jun 7, 2023

I thought about this again since it's related to recovery and the message to be printed in #1679.

How about something like jj restore --revision-from-parents? Perhaps with a shorter alias like --rp. jj restore -r would print a hint and exit.

@ilyagr ilyagr force-pushed the restore-r branch 3 times, most recently from 1a4ab3d to 379caf2 Compare July 11, 2023 04:30
@ilyagr ilyagr force-pushed the restore-r branch 6 times, most recently from f622eb8 to e70ad14 Compare July 29, 2023 04:13
@ilyagr ilyagr changed the title cmd: Add -r argument to jj restore cli: New --revision-from-parents AKA --rp argument to jj restore Jul 30, 2023
@ilyagr ilyagr marked this pull request as ready for review July 30, 2023 04:07
@ilyagr
Copy link
Contributor Author

ilyagr commented Jul 30, 2023

It took me a while to update all the docstrings, hope the result is clear enough.

@martinvonz
Copy link
Member

restore is not interactive and does not use an external diff tool. It doesn't show the diff at all.

By the way, I've been confused by this a lot of times. In my mind, I want to copy some contents over from one revision to another, so I try jj restore --from X -i but that flag doesn't exist. We should probably roll back f6d226e. With the change in this PR, it seems like diffedit and restore have the same functionality except that one is interactive and one is non-interactive.

@ilyagr
Copy link
Contributor Author

ilyagr commented Jul 30, 2023

We should probably roll back f6d226e

jj rebase -i is confusing to me. On one hand, I'd expect it to default to restoring everything, which would mean having the "from" side on the right and the "to" side on the left. On the other hand, that seems confusing, especially if jj diffedit also exists with the "from" side on the left.

Some options that come to mind:

  • Print a hint to use diffedit when the user does jj rebase -i.
  • Ignore the problem I mentioned and explicitly say that jj rebase -i is a synonym for jj diffedit (except the --rp option is named weirdly different than -r, though I suppose we could accept -i -r if we really wanted to).

In any case, I think that's outside the scope of this PR.

@ilyagr
Copy link
Contributor Author

ilyagr commented Jul 30, 2023

BTW, to substitute for Github autolinking, @avamsi opened a related discussion at #1929.

@martinvonz
Copy link
Member

We should probably roll back f6d226e

jj rebase -i is confusing to me. On one hand, I'd expect it to default to restoring everything, which would mean having the "from" side on the right and the "to" side on the left. On the other hand, that seems confusing, especially if jj diffedit also exists with the "from" side on the left.

Some options that come to mind:

  • Print a hint to use diffedit when the user does jj rebase -i.
  • Ignore the problem I mentioned and explicitly say that jj rebase -i is a synonym for jj diffedit (except the --rp option is named weirdly different than -r, though I suppose we could accept -i -r if we really wanted to).

I really like the idea of using a 3-pane diff we talked about in #1905 (comment). One of the advantages of that is we can have the initial state be either the left or the right side. For jj diffedit, we would presumably default to the right side, but for jj restore -i, we'd default to the left side. What do you think?

In any case, I think that's outside the scope of this PR.

Agreed.

src/commands/mod.rs Outdated Show resolved Hide resolved
@ilyagr ilyagr changed the title cli: New --revision-from-parents AKA --rp argument to jj restore cli: New --changes-in argument to jj restore Aug 1, 2023
Also updates docstrings for `diffedit`, `abandon` in related ways. The changes
are a bit too intertwined to comfortable split into a separate commit.
src/commands/mod.rs Outdated Show resolved Hide resolved
@ilyagr ilyagr enabled auto-merge (rebase) August 1, 2023 22:19
@ilyagr ilyagr merged commit 8c9fc98 into jj-vcs:main Aug 1, 2023
@ilyagr ilyagr deleted the restore-r branch August 1, 2023 22:29
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.

5 participants