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.

Sign up for GitHub

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

Jump to bottom

test,fs: add fs.rm() tests for .git directories #42410

Merged
merged 4 commits into from
Mar 28, 2022
Merged

test,fs: add fs.rm() tests for .git directories #42410

merged 4 commits into from
Mar 28, 2022

Conversation

RaisinTen
Copy link
Contributor

Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: #38810 (comment)
Signed-off-by: Darshan Sen raisinten@gmail.com

@RaisinTen
test,fs: add fs.rm() tests for .git directories
7edad00 
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: nodejs#38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Mar 20, 2022
@RaisinTen RaisinTen mentioned this pull request Mar 20, 2022
Closed
bcoe
bcoe approved these changes Mar 20, 2022
View reviewed changes
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

LGTM once @richardlau's comment is addressed.

@RaisinTen
test: check if git is present before running the tests
b76ef18 
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Mar 21, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 21, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 21, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@RaisinTen
test: run cd dir && git init && cd - instead of git init -C dir
a2da073 
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen
Copy link
Contributor Author

RaisinTen commented Mar 26, 2022 •
edited
Loading

Git in centos7-arm64-gcc8 doesn't support the -C option, so the test was failing with:

Unknown option: -C
usage: git [--version] [--help] [-c name=value]
           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
           [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
           <command> [<args>]

I have updated the test to run git init inside the directory that it tries to turn into a git directory instead, PTAL.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@RaisinTen RaisinTen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 26, 2022
@RaisinTen
fixup! test: run cd dir && git init && cd - instead of `git init -C…
bc950fa 
… dir`

Signed-off-by: Darshan Sen <raisinten@gmail.com>
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Mar 27, 2022
Open
26 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/43196/

@github-actions github-actions bot mentioned this pull request Mar 28, 2022
Open
24 tasks
@RaisinTen RaisinTen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 28, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 28, 2022
@nodejs-github-bot nodejs-github-bot merged commit 5a138b5 into nodejs:master Mar 28, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 5a138b5

@RaisinTen RaisinTen deleted the test,fs/add-fs.rm-tests-for-git-directories branch March 28, 2022 03:58
This was referenced Mar 29, 2022
Open
Open
Open
Open
Open
Open
juanarbol pushed a commit that referenced this pull request Apr 4, 2022
@RaisinTen @juanarbol
test,fs: add fs.rm() tests for .git directories
3f5b004 
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: #38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit to juanarbol/node that referenced this pull request Apr 5, 2022
@RaisinTen @juanarbol
test,fs: add fs.rm() tests for .git directories
470e911 
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: nodejs#38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs#42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Apr 5, 2022
Closed
Merged
juanarbol pushed a commit that referenced this pull request Apr 6, 2022
@RaisinTen @juanarbol
test,fs: add fs.rm() tests for .git directories
19c933c 
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: #38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
@RaisinTen @xtx1130
test,fs: add fs.rm() tests for .git directories
e4c9ac2 
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: nodejs#38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs#42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this pull request May 31, 2022
@RaisinTen @juanarbol
test,fs: add fs.rm() tests for .git directories
a195b4f 
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: #38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@juanarbol juanarbol mentioned this pull request May 31, 2022
Closed
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
@RaisinTen @danielleadams
test,fs: add fs.rm() tests for .git directories
5e897c1 
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: #38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 11, 2022
@RaisinTen @targos
test,fs: add fs.rm() tests for .git directories
40b9148 
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: #38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 11, 2022
@RaisinTen @targos
test,fs: add fs.rm() tests for .git directories
92e829e 
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: #38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@RaisinTen @targos
test,fs: add fs.rm() tests for .git directories
1641ff5 
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: #38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@RaisinTen @guangwong
test,fs: add fs.rm() tests for .git directories
dd287cc 
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: nodejs/node#38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs/node#42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@aduh95 aduh95 Awaiting requested review from aduh95

@bcoe bcoe Awaiting requested review from bcoe

@jasnell jasnell Awaiting requested review from jasnell

@richardlau richardlau Awaiting requested review from richardlau

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@RaisinTen @nodejs-github-bot @bcoe @jasnell @lpinca @richardlau @aduh95