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: workspace update-stale: set description on recovery commit #4738

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

martinvonz
Copy link
Owner

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

The recover commit we create in some cases (when an operation has been
lost) doesn't currently have a description. That makes it easy to miss
that it's special.
Copy link
Collaborator

@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.

Thank you!

I think I see many of these commits, and I didn't know what they are about. Now I know, and I will know which commits this applies to.

If all the commits I think are related to this actually are caused by this, this seems to happen more often than I'd expect with concurrency. I'm not sure why so many operations would fail to be recorded.

@@ -125,6 +125,16 @@ fn create_and_check_out_recovery_commit(
vec![commit_id.clone()],
commit.tree_id().clone(),
)
.set_description(
"recovery commit from `jj workspace update-stale`
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Optional)

The first line is a bit inconspicuous and doesn't make it clear that the commit is machine-generated. Perhaps add JJ AUTO-RECOVERY: before it? Then, people will know that something strange happened, and what to search for and ask about.

We could also try it as is and see how many of these commits get generated and how in conspicuous they are.

BTW, I'm not sure everyone agrees, but I generally like the idea of having jj use JJ SOMETHING: as a marker for some text that is special to jj, extending the JJ: syntax we use in description editor (I don't mean to do this religiously, but it makes sense to me here).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have a strong feeling, but it would be nice if the description looked unusual. User might push the recovery commit by mistake if it looked like a normal commit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should prefix it with JJ: and use that private commits feature to ensure that commits starting with JJ: don’t get pushed by default.

@@ -125,6 +125,16 @@ fn create_and_check_out_recovery_commit(
vec![commit_id.clone()],
commit.tree_id().clone(),
)
.set_description(
"recovery commit from `jj workspace update-stale`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have a strong feeling, but it would be nice if the description looked unusual. User might push the recovery commit by mistake if it looked like a normal commit.

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.

4 participants