Conversation
7e0f973 to
238b8ca
Compare
|
Not to be the naming guy again, but would "squash" make more sense for this action? I have no idea what "fixup" means without reading a description, and "squash" already exists in jj concepts. |
Maybe. |
Name the action for the action it does, not the flags you give it. |
I agree and I've tried. I thought "squash, discarding message" was too long. It should ideally not use a lot of horizontal space. Any suggestions? |
Perhaps this is an indication to adjust the UI, by putting the "flags" as toggles, separate from the action name, instead of having separate actions. Or prompt for what to do with the description. |
That makes sense. Are you imagining something similar to
Throwing this out there, but I'm not sure it's a good idea either: "squash changes only" vs. "squash" (which would resolve the description somehow)?
This is a good point. I'm not sure if you had any thoughts on what this would look like in the future, i.e., if there would be a way to pass flags to equivalent |
Yes. (I don't have much opinion about the other questions yet.) |
|
As a non native speaker it took me some time to figure out that "fixup" is a git jargon and not an English word. If I didn't knew git, I would probably express the fixup concept with one of those words: fold, or absorb. I like fold the most, because absorb is already a jj concept and may cause confusion. |
|
Squish? |
I had the opposite problem.... I am a native English speaker, but don't use Git much, so "fixup" is simply a vague non-word that tells me nothing. |
|
Food for thought: assuming that the user is thinking first/primarily about the change they're squashing and the destination change is second/secondary in their mind, use option names that implicity refer to the src unless the dest is required. jj squash ... # msg editor
jj squash ... --edit # msg editor
jj squash ... --discard-msg # src's msg is discarded, add `--edit` to revise.
jj squash ... --overwrite-msg # src's msg replaces dest's, add `--edit` to revise.
jj squash ... --merge-msgs # Use an external tool to merge msgs (thinking an LLM, but user defined), then msg editor. add `--no-edit` (spelling?) to _not_ edit & YOLO the result. |
|
I currently really like the |
|
IIUC, we have some people who prefer calling the current "squash and discard description" just "squash" for now and then deciding what to do with "squash and combine descriptions" later (maybe a different name, maybe some other indication in the UI). So I'll go ahead and just replace "fixup" by "squash" now so this hopefully gets unblocked. |
238b8ca to
abf4fd6
Compare
|
I have renamed it to "squash". This is now ready for review |
abf4fd6 to
595b591
Compare
josephlou5
left a comment
There was a problem hiding this comment.
Also update the PR title.
I'm about to add support for a "squash" action (name might change). It will be allowed to mark a chain of commits as squashed, but it won'tx be allowed to mark merge commits as fixups. It would be easy to make pressing 'f' a no-op while on a merge commit, but then the user would still mark it as a fixup and later swap the commit with a merge commit. It gets complicated to check all interactions for each action, so this patch instead applies the state update to a clone of the state object, then validates the new state, and discards the new state if it's invalid. For now, the validation always passes.
This will reduce the risk of updating the old state instead of the new state.
It's easier to support squashing while discarding the message then asking the user for the combined message, so that's why I'm adding that first. For reference, `git rebase -i` calls this "fixup" while `hg histedit` calls it "roll". We'll have to decide later how to handle squashing while combining the message.
martinvonz
left a comment
There was a problem hiding this comment.
Thanks for reviewing
595b591 to
832054a
Compare
|
LGTM, but also update the PR title. |
jj arrangejj arrange
Done. Sorry, I misread your previous request a "Also updated the PR title.". I thought you had already updated it and I didn't even check :) |
| if let Some(new_tree) = new_tree { | ||
| let new_commit = rewriter.reparent().set_tree(new_tree).write().await?; | ||
| rewritten_commits.insert(id, new_commit); | ||
| } else if rewriter.parents_changed() { |
There was a problem hiding this comment.
Perhaps, we need to set predecessors somewhere in this function.
| if new_parent_ids != rewrite.new_parents { | ||
| let store = mut_repo.store().clone(); | ||
| let new_parents = try_join_all( | ||
| new_parent_ids | ||
| .iter() | ||
| .map(async |id| store.get_commit_async(id).await), | ||
| ) | ||
| .await?; | ||
| let new_base_tree = merge_commit_trees_no_resolve_without_repo( | ||
| mut_repo.store(), | ||
| mut_repo.index(), | ||
| &new_parents, | ||
| ) | ||
| .await?; | ||
| tree_terms.push(( | ||
| new_base_tree, | ||
| format!( | ||
| "{} (new parents of squashed commits)", | ||
| conflict_label_for_commits(&new_parents) | ||
| ), | ||
| )); | ||
| tree_terms.push(( | ||
| rewrite.old_commit.parent_tree(mut_repo).await?, | ||
| format!( | ||
| "{} (old parents of squashed commit)", | ||
| conflict_label_for_commits(&rewrite.old_commit.parents().await?) | ||
| ), | ||
| )); | ||
| } |
There was a problem hiding this comment.
Can you add test that would fail without these rebasing terms? It would be nicer if we can extract/reuse jj-lib function.
|
|
||
| fn is_valid(&self) -> bool { | ||
| for (id, commit_state) in &self.commits { | ||
| if self.external_children.contains(id) || self.external_parents.contains(id) { |
There was a problem hiding this comment.
nit: since external children are included in the rewrite plan, I think we should also validate them.
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)how it works, how it's organized), including any code drafted by AI
eye towards deleting anything that is irrelevant, clarifying anything that
is confusing, and adding details that are relevant. This includes, for
example, commit descriptions, PR descriptions, and code comments.