-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Adds stashes into Commit view #3797
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 13 changed files in this pull request and generated 1 suggestion.
Files not reviewed (8)
- package.json: Language not supported
- src/views/branchesView.ts: Evaluated as low risk
- src/views/commitsView.ts: Evaluated as low risk
- src/views/nodes/branchNode.ts: Evaluated as low risk
- src/views/nodes/repositoryNode.ts: Evaluated as low risk
- src/git/gitProviderService.ts: Evaluated as low risk
- src/views/nodes/remoteNode.ts: Evaluated as low risk
- src/views/nodes/branchesNode.ts: Evaluated as low risk
const data = await this.git.log( | ||
repoPath, | ||
{ configs: gitLogDefaultConfigsWithFiles, ref: options?.ref, stdin: options?.stdin }, | ||
{ configs: gitLogDefaultConfigsWithFiles, ref: ref, stdin: stdin }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the 'stdin' variable is only included when it has valid content to avoid unexpected behavior. Suggestion: { configs: gitLogDefaultConfigsWithFiles, ref: ref, ...(stdin ? { stdin } : {}) }
{ configs: gitLogDefaultConfigsWithFiles, ref: ref, stdin: stdin }, | |
{ configs: gitLogDefaultConfigsWithFiles, ref: ref, ...(stdin ? { stdin } : {}) } |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
34cd987
to
103d555
Compare
103d555
to
ec140cb
Compare
The settings are all off by default, because there could be perf implications for large repos. So the risks here are super low. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok. Tiny nit before merging
ec140cb
to
0a95e73
Compare
I think this is ready for review/testing, as it works for any views that use "branch nodes", e.g. Commits, Branches, Worktrees, Repositories views.
We will need to figure out how to get it to work with the Commit Graph without having to refresh all the commits (and stashes) based on the selected "Branches Visibility"@axosoft-ramint maybe instead of filtering the stash commits, we just also return a set of "reachable" commits, and then be able to pass both sets into the Graph or something.We will need to handle the Commit Graph differently.