Skip to content

cli: rename jj touch to jj metaedit #7365

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
Merged
merged 5 commits into from
Sep 3, 2025
Merged

cli: rename jj touch to jj metaedit #7365

merged 5 commits into from
Sep 3, 2025

Conversation

martinvonz
Copy link
Member

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/updated tests to cover my changes

@martinvonz martinvonz requested a review from a team as a code owner August 27, 2025 15:50
@martinvonz
Copy link
Member Author

@glehmann FYI

@glehmann
Copy link
Contributor

You might want to also rename test_touch_command.rs.

@martinvonz
Copy link
Member Author

You might want to also rename test_touch_command.rs.

Done.

ilyagr
ilyagr previously approved these changes Aug 27, 2025
View reviewed changes
Copy link
Contributor

@ilyagr ilyagr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

SGTM, thank you and everyone involved! Let's wait a day for comments before merging.

@ilyagr ilyagr added the 🕚 waiting to give others chance to review We want to give people a chance to comment before merging label Aug 28, 2025
@yuja
Copy link
Contributor

yuja commented Aug 28, 2025

If it's called metaedit, maybe we should leave unmodified commits as diffedit would do?

I think touch had implication that it would always update committer timestamp, so it was okay to rewrite target commits unconditionally. OTOH, metaedit sounds like a command to edit <thing> explicitly.

@martinvonz
Copy link
Member Author

If it's called metaedit, maybe we should leave unmodified commits as diffedit would do?

Yes, I think that would be good. We could add an explicit flag to make it bump the timestamp (i.e. to consider all commits changed). I think it's fine to consider it a limitation that it currently rewrites unchanged commits. What do you think?

@martinvonz
Copy link
Member Author

Actually, I can probably quite easily make it not change the commits by default. Maybe I'll also add that flag to bump the committer timestamp.

@ilyagr
Copy link
Contributor

ilyagr commented Aug 28, 2025 •
edited
Loading

Actually, I can probably quite easily make it not change the commits by default. Maybe I'll also add that flag to bump the committer timestamp.

IMO, this could be a separate PR. But up to you two.

For now (if we are making that a separate PR), we could mention this as a limitation in the docs (optionally, as far as I'm concerned) It might also make sense to mention it in the commit description. I thought it was mentioned, but I think that it was actually only mentioned on Discord somewhere.

@martinvonz
cli: rename jj touch to jj metaedit
b7bf899 
There were some concerns that `touch` only makes sense if you're
familiar with the Unix `touch` command. I think that's a fair
concern. I did not hear any objections to renaming the command. If
we're going to do it, we should do it now, so it doesn't ever get
released under the old name.
@martinvonz
Copy link
Member Author

This PR now makes jj metaedit without arguments a no-op (not sure if we should make that an error) and adds a --update-committer-timestamp.

@ilyagr ilyagr dismissed their stale review August 28, 2025 05:11

New commits, Yuya probably plans to take a look either way.

@ilyagr ilyagr removed the 🕚 waiting to give others chance to review We want to give people a chance to comment before merging label Aug 28, 2025
yuja
yuja reviewed Aug 28, 2025
View reviewed changes
@martinvonz
metaedit: remove redundant check for error code
b9fcb16 
The status code is already tested as part of the `CommandOutput`.

Also rename variable of type `CommandOutput` to more standard
`output`.
@martinvonz
metaedit: move rewriter.reparent() call inside if/else arms
03fff23 
This is just a tiny refactoring to prepare for making the command not
rewrite commits if nothing changed.
@martinvonz
metaedit: don't rewrite commits if nothing changed
4da9181
@martinvonz
metaedit: add option to update committer timestamp
37c620e
@glehmann
Copy link
Contributor

glehmann commented Sep 3, 2025

IIRC, we are very close to a release, so this PR should be merged quickly!

ilyagr
ilyagr approved these changes Sep 3, 2025
View reviewed changes
Copy link
Contributor

@ilyagr ilyagr left a comment

Choose a reason for hiding this comment

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

I think there may have been some miscommunication. It looks OK to me and I don't see any signs that Yuya wanted further changes.

Thanks Gaetan!

@martinvonz martinvonz added this pull request to the merge queue Sep 3, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 3, 2025
@martinvonz martinvonz added this pull request to the merge queue Sep 3, 2025
Merged via the queue into main with commit 24f7c76 Sep 3, 2025
29 checks passed
@martinvonz martinvonz deleted the mz/xuyokqnkqlrr branch September 3, 2025 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuja yuja yuja left review comments

@ilyagr ilyagr ilyagr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@martinvonz @glehmann @yuja @ilyagr