git: Unify commit popups#38749
Conversation
|
@ConradIrwin I added more context to this PR, please check it if you find the time |
cole-miller
left a comment
There was a problem hiding this comment.
Thanks, I like the idea of reducing the duplication here!
This is a bit difficult to review right now because it seems some stray bytes were introduced into commit.rs, causing git to mark that file as binary and preventing it from showing a diff. Could you fix that? Happy to do a more detailed review afterward.
123f0e8 to
c0dbcee
Compare
cole-miller
left a comment
There was a problem hiding this comment.
Thanks and sorry for the delay in returning to this! I left some feedback about the code style and organization here, just trying to keep the change focused.
crates/git/src/commit.rs
Outdated
| impl Default for CommitDetails { | ||
| fn default() -> Self { | ||
| Self { | ||
| sha: Default::default(), | ||
| author_name: Default::default(), | ||
| author_email: Default::default(), | ||
| commit_time: OffsetDateTime::now_utc(), | ||
| message: Default::default(), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl PartialEq for CommitDetails { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| self.sha == other.sha | ||
| } | ||
| } | ||
|
|
||
| impl Eq for CommitDetails {} |
There was a problem hiding this comment.
Ditto the PartialEq and Eq impls here, and I'd rather not have the Default impl at all because calling now_utc in it feels like a footgun
|
I don't know the rule about closing comments in Zed, so I'm leaving them open but everything should be done @cole-miller |
|
Hey @LoricAndre, sorry for the slow turnaround here. I realized when looking over this again that we can solve the original issue with a simpler approach that's a subset of what you've done here: just move |
024af91 to
2fe3b62
Compare
|
Hi @cole-miller, I was about to ask for some input, so the timing is pretty perfect (I have my hands quite full with my other projects, don't worry about taking some time to review). |
|
I resolved the previous comments as the changes made them obsolete at this point, feel free to reopen anything that you still deem relevant |
|
Thanks! I just pushed some small tweaks, this is ready to go now. |
|
Thank you, I had missed that dead code |
Closes #26424 Supersedes #35328 Originally, `git::blame` uses its own `ParsedCommitMessage` as the source for the commit information, including the PR section. This changes unifies this with `git::repository` and `git_ui::git_panel` by moving this and some other commit-related structs to `git::commit` instead, and making both `git_ui::blame_ui` and `git_ui::git_panel` pull their information from these structs. Release notes : - (Let's Git Together) Fixed the commit tooltip in the git panel not showing information like avatars. --------- Co-authored-by: Cole Miller <cole@zed.dev> Co-authored-by: Zed Zippy <234243425+zed-zippy[bot]@users.noreply.github.com>
Closes zed-industries#26424 Supersedes zed-industries#35328 Originally, `git::blame` uses its own `ParsedCommitMessage` as the source for the commit information, including the PR section. This changes unifies this with `git::repository` and `git_ui::git_panel` by moving this and some other commit-related structs to `git::commit` instead, and making both `git_ui::blame_ui` and `git_ui::git_panel` pull their information from these structs. Release notes : - (Let's Git Together) Fixed the commit tooltip in the git panel not showing information like avatars. --------- Co-authored-by: Cole Miller <cole@zed.dev> Co-authored-by: Zed Zippy <234243425+zed-zippy[bot]@users.noreply.github.com>
Closes zed-industries#26424 Supersedes zed-industries#35328 Originally, `git::blame` uses its own `ParsedCommitMessage` as the source for the commit information, including the PR section. This changes unifies this with `git::repository` and `git_ui::git_panel` by moving this and some other commit-related structs to `git::commit` instead, and making both `git_ui::blame_ui` and `git_ui::git_panel` pull their information from these structs. Release notes : - (Let's Git Together) Fixed the commit tooltip in the git panel not showing information like avatars. --------- Co-authored-by: Cole Miller <cole@zed.dev> Co-authored-by: Zed Zippy <234243425+zed-zippy[bot]@users.noreply.github.com>
Closes zed-industries#26424 Supersedes zed-industries#35328 Originally, `git::blame` uses its own `ParsedCommitMessage` as the source for the commit information, including the PR section. This changes unifies this with `git::repository` and `git_ui::git_panel` by moving this and some other commit-related structs to `git::commit` instead, and making both `git_ui::blame_ui` and `git_ui::git_panel` pull their information from these structs. Release notes : - (Let's Git Together) Fixed the commit tooltip in the git panel not showing information like avatars. --------- Co-authored-by: Cole Miller <cole@zed.dev> Co-authored-by: Zed Zippy <234243425+zed-zippy[bot]@users.noreply.github.com>
Closes #26424
Supersedes #35328
Originally,
git::blameuses its ownParsedCommitMessageas the source for the commit information, including the PR section. This changes unifies this withgit::repositoryandgit_ui::git_panelby moving this and some other commit-related structs togit::commitinstead, and making bothgit_ui::blame_uiandgit_ui::git_panelpull their information from these structs.Release notes :