git_ui: Move git_graph into git_ui - #57503
Conversation
|
@Anthony-Eid Looks like after merging a new main branch the tests are broken 🤔 I will take a look and update the branch update: |
|
These refactoring PRs need some more explanation if you want people to be able to review them. For example, in this PR you removed the GPL license, there are sections of code that were removed, as well as function calls that were changed to other signatures and calls. It isn't clear to me that functions that were abstracted here do the same thing as the original code, such as This PR description doesn't include any context for why the refactoring is being done, and I had to go chat with Anthony to find out. In the future when executing any refactorings like this, please take it one piece at a time in PRs and don't include the addition of logical refactors with large non-logic-related changes like moving files around. |
|
Also the build failing earlier makes me wonder if you ran the build locally before submitting the PR, which makes me even more suspect of whether some of the logical changes and new functions were adequately checked before the PR was submitted. |
Yes, I did run it locally but the PR was opened ~1 week ago and after merging a new main it failed (as before merging a new main there were merge conflicts)
The whole
Yeah, that was a part of the moving things into
Noted. I can put the description and which things were refactored code-wise in this PR as well, wdyt? It will make the review easier I guess but after some thinking: does any "non Zed team member" review really "counts" 😅 ? |
Yes, it is expected that a PR passes tests after merging main. We can not review or merge it otherwise.
Yes, and this should be mentioned in the PR description, along with an explanation of why the code that was previously covered under the removed license is compatible with where it was moved to.
Not sure what you mean by this, please expand. |
|
I updated the PR description, so it should now have an explanation "why" this PR exists.
I am not sure that I follow this. The deleted license file was a symlink to the actual license, so the actual license is still there and
So, how I see this whole situation (Please bear in mind that I might look at this from the wrong angle!):
|
Co-authored-by: Mikhail Pertsev <arthasmontano@mail.ru>
cc @Anthony-Eid ## Why This is the first step in moving the Git Graph work into the Git UI crate before continuing with follow-up refactors and feature work. The goal is for Git UI components and shared Git UI helpers to live in one crate, so future changes to the Git Graph can reuse existing `git_ui` code instead of duplicating it. This PR is not only a filesystem move. While moving `git_graph` into `git_ui`, a few small dependency and helper boundaries had to change: - `git_graph` and `git_ui` both needed the same remote parsing and commit tooltip construction behavior, so those pieces are now shared from `git_ui`. - `git_graph` previously depended on `project_panel` to resolve file-history actions from the project panel selection. After moving `git_graph` into `git_ui`, keeping that dependency would create an undesirable `git_ui` -> `project_panel` relationship. The project-panel-specific action forwarding now lives in `project_panel`, and calls into exported `git_ui::git_graph` helpers instead. - `git_graph` initialization now happens through `git_ui::init`, so downstream crates only need to initialize `git_ui`. This prepares the codebase for the next planned PRs: splitting the large `git_graph.rs` implementation into smaller pieces, then adding Git Graph features such as keeping the main branch lane at index `0`. ## License removal The removed `crates/git_graph/LICENSE-GPL` file was a symlink to the repository root `LICENSE-GPL`. The moved code is now inside `git_ui`, which is also licensed as `GPL-3.0-or-later` and has its own `LICENSE-GPL` symlink to the same root license file. The code did not move to a differently licensed crate; it remains covered by the same GPL license. Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - N/A --------- Co-authored-by: Anthony Eid <anthony@zed.dev>
Resolve git_graph move into git_ui (zed-industries#57503); keep custom knip::init. Preserve fork feature flags as enabled_for_all.
cc @Anthony-Eid ## Why This is the first step in moving the Git Graph work into the Git UI crate before continuing with follow-up refactors and feature work. The goal is for Git UI components and shared Git UI helpers to live in one crate, so future changes to the Git Graph can reuse existing `git_ui` code instead of duplicating it. This PR is not only a filesystem move. While moving `git_graph` into `git_ui`, a few small dependency and helper boundaries had to change: - `git_graph` and `git_ui` both needed the same remote parsing and commit tooltip construction behavior, so those pieces are now shared from `git_ui`. - `git_graph` previously depended on `project_panel` to resolve file-history actions from the project panel selection. After moving `git_graph` into `git_ui`, keeping that dependency would create an undesirable `git_ui` -> `project_panel` relationship. The project-panel-specific action forwarding now lives in `project_panel`, and calls into exported `git_ui::git_graph` helpers instead. - `git_graph` initialization now happens through `git_ui::init`, so downstream crates only need to initialize `git_ui`. This prepares the codebase for the next planned PRs: splitting the large `git_graph.rs` implementation into smaller pieces, then adding Git Graph features such as keeping the main branch lane at index `0`. ## License removal The removed `crates/git_graph/LICENSE-GPL` file was a symlink to the repository root `LICENSE-GPL`. The moved code is now inside `git_ui`, which is also licensed as `GPL-3.0-or-later` and has its own `LICENSE-GPL` symlink to the same root license file. The code did not move to a differently licensed crate; it remains covered by the same GPL license. Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - N/A --------- Co-authored-by: Anthony Eid <anthony@zed.dev>
cc @Anthony-Eid ## Why This is the first step in moving the Git Graph work into the Git UI crate before continuing with follow-up refactors and feature work. The goal is for Git UI components and shared Git UI helpers to live in one crate, so future changes to the Git Graph can reuse existing `git_ui` code instead of duplicating it. This PR is not only a filesystem move. While moving `git_graph` into `git_ui`, a few small dependency and helper boundaries had to change: - `git_graph` and `git_ui` both needed the same remote parsing and commit tooltip construction behavior, so those pieces are now shared from `git_ui`. - `git_graph` previously depended on `project_panel` to resolve file-history actions from the project panel selection. After moving `git_graph` into `git_ui`, keeping that dependency would create an undesirable `git_ui` -> `project_panel` relationship. The project-panel-specific action forwarding now lives in `project_panel`, and calls into exported `git_ui::git_graph` helpers instead. - `git_graph` initialization now happens through `git_ui::init`, so downstream crates only need to initialize `git_ui`. This prepares the codebase for the next planned PRs: splitting the large `git_graph.rs` implementation into smaller pieces, then adding Git Graph features such as keeping the main branch lane at index `0`. ## License removal The removed `crates/git_graph/LICENSE-GPL` file was a symlink to the repository root `LICENSE-GPL`. The moved code is now inside `git_ui`, which is also licensed as `GPL-3.0-or-later` and has its own `LICENSE-GPL` symlink to the same root license file. The code did not move to a differently licensed crate; it remains covered by the same GPL license. Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - N/A --------- Co-authored-by: Anthony Eid <anthony@zed.dev>
cc @Anthony-Eid ## Why This is the first step in moving the Git Graph work into the Git UI crate before continuing with follow-up refactors and feature work. The goal is for Git UI components and shared Git UI helpers to live in one crate, so future changes to the Git Graph can reuse existing `git_ui` code instead of duplicating it. This PR is not only a filesystem move. While moving `git_graph` into `git_ui`, a few small dependency and helper boundaries had to change: - `git_graph` and `git_ui` both needed the same remote parsing and commit tooltip construction behavior, so those pieces are now shared from `git_ui`. - `git_graph` previously depended on `project_panel` to resolve file-history actions from the project panel selection. After moving `git_graph` into `git_ui`, keeping that dependency would create an undesirable `git_ui` -> `project_panel` relationship. The project-panel-specific action forwarding now lives in `project_panel`, and calls into exported `git_ui::git_graph` helpers instead. - `git_graph` initialization now happens through `git_ui::init`, so downstream crates only need to initialize `git_ui`. This prepares the codebase for the next planned PRs: splitting the large `git_graph.rs` implementation into smaller pieces, then adding Git Graph features such as keeping the main branch lane at index `0`. ## License removal The removed `crates/git_graph/LICENSE-GPL` file was a symlink to the repository root `LICENSE-GPL`. The moved code is now inside `git_ui`, which is also licensed as `GPL-3.0-or-later` and has its own `LICENSE-GPL` symlink to the same root license file. The code did not move to a differently licensed crate; it remains covered by the same GPL license. Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - N/A --------- Co-authored-by: Anthony Eid <anthony@zed.dev>
cc @Anthony-Eid
Why
This is the first step in moving the Git Graph work into the Git UI crate before continuing with follow-up refactors and feature work. The goal is for Git UI components and shared Git UI helpers to live in one crate, so future changes to the Git Graph can reuse existing
git_uicode instead of duplicating it.This PR is not only a filesystem move. While moving
git_graphintogit_ui, a few small dependency and helper boundaries had to change:git_graphandgit_uiboth needed the same remote parsing and commit tooltip construction behavior, so those pieces are now shared fromgit_ui.git_graphpreviously depended onproject_panelto resolve file-history actions from the project panel selection. After movinggit_graphintogit_ui, keeping that dependency would create an undesirablegit_ui->project_panelrelationship. The project-panel-specific action forwarding now lives inproject_panel, and calls into exportedgit_ui::git_graphhelpers instead.git_graphinitialization now happens throughgit_ui::init, so downstream crates only need to initializegit_ui.This prepares the codebase for the next planned PRs: splitting the large
git_graph.rsimplementation into smaller pieces, then adding Git Graph features such as keeping the main branch lane at index0.License removal
The removed
crates/git_graph/LICENSE-GPLfile was a symlink to the repository rootLICENSE-GPL. The moved code is now insidegit_ui, which is also licensed asGPL-3.0-or-laterand has its ownLICENSE-GPLsymlink to the same root license file. The code did not move to a differently licensed crate; it remains covered by the same GPL license.Self-Review Checklist:
Release Notes: