Skip to content
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

Updated dugite-extra and find-git-exec. #6602

Merged
merged 1 commit into from
Nov 22, 2019
Merged

Conversation

kittaakos
Copy link
Contributor

The updated dependencies contain fix for:

  • The termination of the spawned process. The bogus process termination resulted in an empty string from the stdout as the exec-path. The empty string "path" was incorrectly normalized to .. (See: use close not exit process event #6587)
  • [win32]: Enhanced git discovery. The new logic falls back to which.
  • [win32]: The unstaging command. -u was added to ensure that git's index reflects a consistent view of the working tree after the operation has been performed.

Signed-off-by: Akos Kitta kittaakos@typefox.io

What it does

See above 👆

How to test

Review checklist

Reminder for reviewers

@marcdumais-work, I would like to request you for a review becasue of the dependencies update. We maintain both dependencies, both have MIT license. find-git-exec was inspired by VS Code, but it was always the case. Please check if we are good with the PR. Thank you!

The updated dependencies contain fix for:
 - The termination of the spawned process. The bogus process termination resulted in an empty string from the `stdout` as the `exec-path`. The empty string "path" was incorrectly normalized to `.`. (See: #6587)
 - [win32]: Enhanced `git` discovery. The new logic falls back to `which`.
 - [win32]: The unstaging command. `-u` was added to ensure that git's index reflects a consistent view of the working tree after the operation has been performed.

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
@kittaakos kittaakos added the dependencies pull requests that update a dependency file label Nov 21, 2019
Copy link
Contributor

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Tested with docker/linux environment in Gitpod. Works nicely! Thank you very much for taking care of this fixes! 🙏
(Would be nice, if some else verify this on Windows.)

@kittaakos
Copy link
Contributor Author

if some else verify this on Windows

I will.

@kittaakos
Copy link
Contributor Author

I have verified the unstaging on Windows. It worked. The not on PATH defect was never an issue with my Windows env. I am merging it.

@kittaakos kittaakos merged commit b9969ba into master Nov 22, 2019
@kittaakos kittaakos deleted the fix-git-not-on-path branch November 22, 2019 12:02
@marcdumais-work
Copy link
Contributor

marcdumais-work commented Nov 25, 2019

@marcdumais-work, I would like to request you for a review becasue of the dependencies update. We maintain both dependencies, both have MIT license. find-git-exec was inspired by VS Code, but it was always the case. Please check if we are good with the PR. Thank you!

Checking now, thanks for your patience :)

update: I see the PR is merged, but I'll check anyway

@marcdumais-work
Copy link
Contributor

@kittaakos Everything looks good:

  • I did the 3rd party license check using clearlydefined on master and all dependencies are still good
  • about the two dependencies maintained under the typefox org: from what I can see you handled the license correctly for the copied code in find-git-exec and the process above covers any new 3rd party dependency that might be newly pulled from these two.

@kittaakos
Copy link
Contributor Author

Everything looks good:

Thank you for your help, @marcdumais-work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants