feat(vcs): add read-only jj repository support#31
Conversation
mabd-dev
left a comment
mabd-dev
left a comment
There was a problem hiding this comment.
thank you for the contribution. Overall the code look clean and well made
| VCSType string `json:"vcsType"` | ||
| Branch string `json:"branch"` | ||
| UncommitedFiles []string `json:"uncommitedFiles"` | ||
| OutgoingCommits []string `json:"outgoingCommits"` |
There was a problem hiding this comment.
why this exists here? i don't like the idea that only support jj. same thing if we have a variable here that is only supported by git.
Maybe this struct can be remodeled to handle both jj and git repo. For now, it's this is fine. Will look into it later
There was a problem hiding this comment.
In jj, a commit is the unit of work so more than looking at uncommitted files , being able to see the number of outgoing commits makes more sense.
I can either remove OutgoingCommits entirely for now and use RemoteStatus.Ahead which should give us the count of number of commits which are ahead. OutgoingCommits would have actually shown what those commits are.
Another approach is to add GIT support for Outgoing Commits. This way there will be no variables that are vcs specific.
Let me know which approach you think is best here.
There was a problem hiding this comment.
It might more sense to add OutgoingCommits to RemoteStatus instead of Repostate as conceptually it belongs near sync state.
There was a problem hiding this comment.
Making git support outgoing commits make sense. And yes, I do agree that OutgoingCommits should be in RemoteStatus not RepoState
| } | ||
|
|
||
| func (p *Provider) CheckRepoState(path string) (report.RepoState, []string) { | ||
| state, warnings := gitx.CheckRepoState(path) |
There was a problem hiding this comment.
hmm, should we move all gitx package files to vcs/git package?
I guess this is the right thing to do, since they will only be used in here
There was a problem hiding this comment.
Makes sense I will move the gitx package files to internal/vcs/git ..
I will keep the internal/render/tui as is for now as we have no jj actions.
Let me know if that's okay.
If not I will create interfaces similar to internal/vcs/provider.go for the various actions.
There was a problem hiding this comment.
yes, keep internal/render/tui as is for now
frittlechasm
force-pushed
the
feat/jjsupport
branch
from
0294143 to
863fb4e
Compare
Introduce a VCS provider layer and generalize repo discovery so reposcan can detect and scan both git and jj repositories.
frittlechasm
force-pushed
the
feat/jjsupport
branch
from
0cf06cf to
420736b
Compare
|
updated the PR with the relevant changes |
mabd-dev
left a comment
mabd-dev
left a comment
There was a problem hiding this comment.
LGTM. Thank you for you work @frittlechasm
2 participants
Adds read-only support for scanning jj repositories alongside Git repositories.
Changes include: