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

feat(record)!: add default commit message for --stash #1462

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

claytonrcarter
Copy link
Collaborator

This adds a default commit message to use when record --stash is called without any --message arguments. This is a breaking change and needs some discussion.

I find that – when stashing – I usually just want to save something for later and move on, without worrying about what to call it, and this is closer to the behavior of git's built-in stash.

Currently, calling record --stash without any --message args will open $EDITOR to supply a commit/stash message, so this would break that behavior for users used to that UX. It maintains the current behavior of using the --message args, if any are supplied.

Alternate options

  1. Put this behind a config flag.
  2. Put the actual "default stash message" into a config option.
  3. I can just give up and rely on shell completion or git aliases to fill in my default message. 😄

@claytonrcarter claytonrcarter force-pushed the record-stash-optional-message branch from ecace4a to 6ff4506 Compare December 8, 2024 04:04
@claytonrcarter
Copy link
Collaborator Author

cc @YakoYakoYokuYoku for comment since you initially added the --stash flag

@claytonrcarter claytonrcarter force-pushed the record-stash-optional-message branch 2 times, most recently from f2e9397 to a7360db Compare December 10, 2024 16:11
@YakoYakoYokuYoku
Copy link
Contributor

In my opinion I'd put this behind a config and I'd also do that to the default commit message.

@claytonrcarter claytonrcarter force-pushed the record-stash-optional-message branch from a7360db to 403f9ec Compare December 14, 2024 23:03
@arxanas
Copy link
Owner

arxanas commented Dec 15, 2024

I haven't really looked at the code yet. Re design: I actually originally envisioned that --stash should let you skip the message and provide a default one:

  • I agree that your workflow probably represents the more common workflow, and also matches the git stash behavior.
  • That is, if you're stashing work, there's a decent chance that you very much don't want to spend time figuring out the commit message, and want to defer that to later, considering that the rest of the work in the stash is also not complete.

My only remaining design consideration for stashes is that I think that it should be easy to differentiate multiple stashes in your smartlog in some way:

  • The ideal way would probably be to enhance the smartlog to be able to preview something like the size of the diffs, or the changed files, etc.
    • This should give you a reasonable idea about what the stash contains, as well as commits generally.
    • Implementing this would require non-trivial design and implementation work, as computing diffs for all the smartlog commits has performance/scalability implications.
  • Assuming we're not changing the smartlog capabilities, then I would advise that you change the default stash message to include some kind of dynamic data like that. Some simple options:
    • The current date and/or time (corresponding to the author timestamp, noting that the smartlog shows commit timestamps rather than author timestamps by default, IIRC).
    • The number of changed files. (You could even include the first N paths or filenames, if you wanted to be fancy, the main consideration being an overly-long commit message.)

For the default stash message:

  • If we include dynamic data, then maybe we can write stash: changes to 3 files (etc.), rather than temp as the prefix?
  • If we don't use stash: as a prefix, then I'd recommend wip: as a slightly more standard prefix (I recently converted my own workflows from temp: to wip: 👀).
  • In principle: We could also just use empty commit messages, jj-style, but I think it might confuse or surprise some users, especially if the smartlog doesn't show any auxiliary contextual data.

I would just not put this behind a config flag, and simply leave it as a breaking change for --stash users. For non-stash users:

  • For now, I don't think we should alter the behavior of the non-stash workflow.
  • In the long term, I think that all invocations of git record without --interactive should not open the commit message editor, and instead provide a default commit message too.
    • The git-branchless design principle here is to not have any interactive operations unless the user explicitly opted in.
    • If you provide --interactive today, then you can immediately jump into the message editor by pressing e, so that would be the ultimate intended workflow.
    • Tangentially: This would also bring the design closer in design to jj, where jj new doesn't prompt for a commit message.

I don't think we need a "default" stash message configuration option, as I suspect it might not carry its weight in terms of discoverability and maintainability, but we can keep it if you feel strongly and are willing to test and document it.

@claytonrcarter claytonrcarter force-pushed the record-stash-optional-message branch from 403f9ec to e1cada1 Compare December 16, 2024 01:39
@claytonrcarter claytonrcarter marked this pull request as draft December 16, 2024 01:40
@claytonrcarter claytonrcarter force-pushed the record-stash-optional-message branch from e1cada1 to 7da4d76 Compare December 16, 2024 01:43
@claytonrcarter
Copy link
Collaborator Author

Thank you both for your input. @YakoYakoYokuYoku, I had recently pushed a version w/ config flags, but I have discarded that change based on the the latest suggestions by @arxanas. I can share or re-push a version with config options in place if you'd like to try them or see them. Just let me know.

  • prefix: I've changed to stash: as it conveys both what the changes are (in git's idiomatic terminology) and also how they were saved/created/commited, but I'm not married to it. Happy to change to wip: or draft: as desired. I seem so recall hearing that "WIP" may be a idiom that is not as familiar to non-english speakers, but my only context on that is from back when Gitlab changed from using WIP: to Draft:; I really don't have a claim to stake on either of those terms.
  • dynamic data: For the sake of discussion, I have used the diff "short stats" for the dynamic commit message (eg stash: 1 file changed, 1 insertion(+), 1 deletion(-). I did this by plumbing git2::Diff::stats() through to the branchless Diff via a new short_stats() method.
  • I had considered putting the date into the message originally, but opted no to because that data is already part of the commit. I will reconsider based on your input, but I haven't done so yet here. I'm thinking it would be something like stash(2024-12-15 08:47PM): 1 file changed, 1 insertion(+), 1 deletion(-). My concern is that the default message may feel cluttered, but I will experiment and report back.
  • This change will actually be helpful for feat: add split command #1464, because that feature is also creating new, temporary commits where it's relevant to summarize the contents. In the current implementation of that, I was just using the repo relative path of the extracted file if only 1 file was split out, or "N files" if >1 file was extracted. The issue, as you noted, was the commit message getting too long w/ multiple files and/or long relative paths. I could also just use the filename w/o path, but that could have problems if (eg) >1 lib.rs files were changed in different dirs in the same commit. Then again, maybe stash: lib.rs lib.rs lib.rs Cargo.toml Cargo.toml would be enough to identify a temporary commit.

claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this pull request Dec 18, 2024
But only when no message is explicitly supplied.

This is a breaking change. Previously, the user would be prompted for a message
if none was supplied.
claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this pull request Dec 18, 2024
@claytonrcarter claytonrcarter force-pushed the record-stash-optional-message branch from 7da4d76 to 22d2698 Compare December 18, 2024 01:44
claytonrcarter added a commit to claytonrcarter/git-branchless that referenced this pull request Dec 20, 2024
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.

3 participants