rewrite: add reset-author-timestamp revset

[scott2000]

This is my first time contributing, but I thought this might be a useful feature. This PR resolves #2000 by adding a revset to configure which commits have their author timestamps updated when they are rewritten. This gives a lot of flexibility for configuration, but it's more complex than having a single hard-coded rule. Another alternative might be to have a configuration option with a few fixed enum variants instead of a revset. I would appreciate any comments and feedback!

One thing which I was unsure about is whether all of the existing tests should be updated (it caused a large number of changes since many commit IDs are different due to the different timestamps), or if this feature should be disabled by default in tests except for a few new tests created to test this feature.

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

[yuja]

According to #2000, options are:

0. on creation (current behavior): incomplete() = none()
1. on first edit: incomplete() = description(exact:"") & empty()
2. on every edit but rebase: incomplete() = all()
3. on commit with certain command or option: incomplete() = none() and use --reset-author?

Do we have any other practical conditions to update author timestamp?

I'm asking because incomplete() = none()|all() sounds odd, and we'll probably need to find a better name if this option is implemented as a revset alias.

[scott2000]

Do we have any other practical conditions to update author timestamp?

I imagine it could be useful to use description("wip:") to update WIP commits or remote_branches().. to only update commits which haven't been pushed to a remote, but I'm not sure whether those are common enough to be necessary to support.

I'm asking because incomplete() = none()|all() sounds odd, and we'll probably need to find a better name if this option is implemented as a revset alias.

Yes, it does sound odd for those cases. Maybe revsets.update-author-timestamp would be better than revset-aliases.incomplete()?

[yuja]

I imagine it could be useful to use description("wip:") to update WIP commits or remote_branches().. to only update commits which haven't been pushed to a remote,

Yeah, that might be useful, but I'm also not sure.

I'm asking because incomplete() = none()|all() sounds odd, and we'll probably need to find a better name if this option is implemented as a revset alias.

Yes, it does sound odd for those cases. Maybe revsets.update-author-timestamp would be better than revset-aliases.incomplete()?

Yes. revsets.update-author-timestamp (or maybe reset-author-timestamp?) sounds better. It's unlikely that we'll use incomplete() in -r incomplete()... form.

@ilyagr any comments to this PR in general?

[joyously]

Do we have any other practical conditions to update author timestamp?

Does your list include setting a historical timestamp (like putting an old project that is in zip files finally into version control)?

[yuja]

Does your list include setting a historical timestamp (like putting an old project that is in zip files finally into version control)?

No, it doesn't. It's the list when user would expect author timestamp of new commit is settled.

[martinvonz]

This PR affects many more call sites than I had expected. Do you think we could get good enough behavior by adding something similar to the option to abandon empty or emptied commits? That is, would another reset_author_if_empty: bool field on that struct be sufficient?

[martinvonz]

This PR affects many more call sites than I had expected. Do you think we could get good enough behavior by adding something similar to the option to abandon empty or emptied commits? That is, would another reset_author_if_empty: bool field on that struct be sufficient?

Sorry, that made no sense. We never want to reset the author timestamp when rebasing. We only want to reset it when creating new commits for reasons other than rebasing.

[yuja]

Perhaps, tx.mut_repo().rewrite_commit() can be proxied through WorkspaceCommandTransaction and timestamp can be updated there? It's not nice that the logic has to be moved to CLI, but I can't find a good place to host this functionality.

Btw, if we don't need revset support, error handling will be much simpler.

[ilyagr]

Some relatively minor thoughts, assuming we go with this design (which got nicer with the last update, thank you!):

• the second commit would benefit from a few more specific tests
• if we support revsets, then perhaps the restriction on the author being the same could be incorporated into the revset? This might mean having to rename the revset again, perhaps reset-author-and-timestamp-on-edit?
• I'm wondering whether there's a better name for the higher-level rewrite-commit function, to make it a little clearer how it's different from the library's rewrite-commit function. Maybe edited-commit? That's not perfect, but it's what came to mind so far.

I'll also ping @arxanas, @chriskrycho and @necauqua in case they have any thoughts.

This feels quite reasonable to me. However, I am not actually bothered by the lack of this feature, so it's hard for me to have a clear feeling about whether this is sufficiently flexible (on one hand) and whether the complexity of allowing a customizable revset aliases is justified (on the other hand).

[necauqua]

This feels quite reasonable to me. However, I am not actually bothered by the lack of this feature

I feel exactly the same :)
Seems good, but I never really felt the lack of it

[scott2000]

Thanks for the feedback!

• if we support revsets, then perhaps the restriction on the author being the same could be incorporated into the revset? This might mean having to rename the revset again, perhaps reset-author-and-timestamp-on-edit?

Yeah, that would make things a bit simpler in CommitBuilder. In that case, we could make it behave the same as jj describe --reset-author does, so maybe revsets.reset-author-on-edit would be clear enough?

• I'm wondering whether there's a better name for the higher-level rewrite-commit function, to make it a little clearer how it's different from the library's rewrite-commit function. Maybe edited-commit? That's not perfect, but it's what came to mind so far.

I was thinking about that too. Maybe rewrite_edited_commit would work?

This feels quite reasonable to me. However, I am not actually bothered by the lack of this feature, so it's hard for me to have a clear feeling about whether this is sufficiently flexible (on one hand) and whether the complexity of allowing a customizable revset aliases is justified (on the other hand).

To me, it seems like it would be good to support at least some level of customization here, since there seems to be at least 4 reasonable behaviors. We could potentially use an enum with these values instead:

Enum Value	Corresponding revset	Author time represents	Similar to
never	none()	Commit created	jj currently
discardable	description(exact:"") & empty()	Work started
no-description	description(exact:"")	First draft completed	git
always	all()	Latest revision completed	git-branchless

(no-description only works if you follow a Git-like workflow where you describe a commit when you finish the first draft)

To me, the main reason to use revsets is to give additional flexibility to users (e.g. resetting for WIP commits) and to make the configuration more consistent (since many config options already use revsets). On the other hand, it seems like using an enum value would allow the logic to be moved into the library code more easily, and it might make it easier to consume for users of the library.

If this option will be rarely used (perhaps most people don't even know about the distinction between author and committer in Git?), then it might not be worth the complexity from revsets.

[ilyagr]

Yeah, that would make things a bit simpler in CommitBuilder. In that case, we could make it behave the same as jj describe --reset-author does, so maybe revsets.reset-author-on-edit would be clear enough?

I was thinking about that too. Maybe rewrite_edited_commit would work?

These sound fine to me.

---

I'd only prefer an enum to a revset if it really makes the implementation much simpler (I haven't thought much about that). From the UI perspective, I think a revset is great, unless we'd rather go with a boolean option or with nothing.

Also, my sense is that there are people who'd appreciate this feature, so I'd lean towards trying some version of it.