feat(Git Sync): UX improvements#8027
Conversation
gatzjames
force-pushed
the
feature/git-improvements-v10
branch
from
20ddf63 to
18b40a9
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for (const change of changes) { | ||
| console.log(`[git] Stage ${change.path} | ${change.status}`); | ||
| if (change.status[1] === 0) { | ||
| await git.remove({ ...this._baseOpts, filepath: convertToPosixSep(path.join('.', change.path)) }); |
Check warning
Code scanning / Semgrep OSS
Semgrep Finding: javascript.lang.security.audit.path-traversal.path-join-resolve-traversal.path-join-resolve-traversal
| if (change.status[1] === 0) { | ||
| await git.remove({ ...this._baseOpts, filepath: convertToPosixSep(path.join('.', change.path)) }); | ||
| } else { | ||
| await git.add({ ...this._baseOpts, filepath: convertToPosixSep(path.join('.', change.path)) }); |
Check warning
Code scanning / Semgrep OSS
Semgrep Finding: javascript.lang.security.audit.path-traversal.path-join-resolve-traversal.path-join-resolve-traversal
gatzjames
force-pushed
the
feature/git-improvements-v10
branch
from
735fc41 to
f4834ce
Compare
| // expect(await GitVCS.status(barTxt)).toBe('*added'); | ||
| // expect(await GitVCS.status(fooTxt)).toBe('*deleted'); | ||
| // await GitVCS.remove(fooTxt); | ||
| // expect(await GitVCS.status(barTxt)).toBe('*added'); | ||
| // expect(await GitVCS.status(fooTxt)).toBe('deleted'); | ||
| // await GitVCS.remove(fooTxt); | ||
| // expect(await GitVCS.status(barTxt)).toBe('*added'); | ||
| // expect(await GitVCS.status(fooTxt)).toBe('deleted'); |
| const diff = { | ||
| head: result[1], | ||
| workdir: result[2], | ||
| stage: result[3], | ||
| }; | ||
|
|
||
| return diff; |
| if ((headType === 'tree' || headType === 'special') && !isBlob) { | ||
| return; | ||
| } | ||
| if (headType === 'commit') { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
is there a significance of returning null or undefined here and below? may be worth a comment if so.
There was a problem hiding this comment.
Yes according to the map function docs null will not walk the tree anymore while undefined will try to walk it's children
| stageType !== 'blob' | ||
| ) { | ||
| // We don't actually NEED the sha. Any sha will do | ||
| // TODO: update this logic to handle N trees instead of just 3. |
| const entry = [undefined, headOid, workdirOid, stageOid]; | ||
| const result = entry.map(value => entry.indexOf(value)); | ||
| result.shift(); |
There was a problem hiding this comment.
can this use the same expression as above, where you name them out of the array
| const { changes, statusNames } = await getGitChanges(GitVCS, workspace); | ||
| const { changes, hasUncommittedChanges } = await getGitChanges(GitVCS); | ||
|
|
||
| console.log('changes', changes); |
| if (err instanceof Errors.HttpError) { | ||
| return { | ||
| errors: [`${err.message}, ${err.data.response}`], | ||
| }; | ||
| } | ||
| const errorMessage = err instanceof Error ? err.message : 'Unknown Error'; | ||
|
|
||
| return { errors: [errorMessage] }; | ||
| } |
There was a problem hiding this comment.
its not clear why these special httperror cases exist, comment or a more meaningful variable name would help
jackkav
left a comment
jackkav
left a comment
There was a problem hiding this comment.
looks good, we should do a mob test on this before GA 10.1 though, likely there is an edge we can't catch by reviewing the code
2 participants
Highlights:
Technical details:
Based on the architecture of isomorphic-git:
This change improves the performance of checking for changes by an order of magnitude on my tests especially on large repos with hundreds of changes.
To improve performance further:
Closes INS-4486
Closes #8020