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

Incorrect identification of the static part of the pattern for the disk root on Windows #63

Open
mrmlnc opened this issue Apr 19, 2023 · 4 comments · May be fixed by #64
Open

Incorrect identification of the static part of the pattern for the disk root on Windows #63

mrmlnc opened this issue Apr 19, 2023 · 4 comments · May be fixed by #64

Comments

@mrmlnc
Copy link
Contributor

mrmlnc commented Apr 19, 2023

What were you expecting to happen?

expect(gp('C:/', { flipBackslashes: false })).toEqual('C:/');
expect(gp('C:/.', { flipBackslashes: false })).toEqual('C:/');
expect(gp('C:/*', { flipBackslashes: false })).toEqual('C:/');
expect(gp('C:/./*', { flipBackslashes: false })).toEqual('C:/.');
expect(gp('C://', { flipBackslashes: false })).toEqual('C:/');
expect(gp('C://*', { flipBackslashes: false })).toEqual('C:/');

What actually happened?

expect(gp('C:/', { flipBackslashes: false })).toEqual('C:'); // 🔴 C: instead of C:/
expect(gp('C:/.', { flipBackslashes: false })).toEqual('C:'); // 🔴 C: instead of C:/
expect(gp('C:/*', { flipBackslashes: false })).toEqual('C:'); // 🔴 C: instead of C:/
expect(gp('C:/./*', { flipBackslashes: false })).toEqual('C:/.'); // 🟢 
expect(gp('C://', { flipBackslashes: false })).toEqual('C:/'); // 🟢 
expect(gp('C://*', { flipBackslashes: false })).toEqual('C:/'); // 🟢

Please give us a sample of your gulpfile

The examples above are tests for this repository.

Please provide the following information:

  • OS & version [e.g. MacOS Catalina 10.15.4]: Windows 11 PRO 22H2
  • node version (run node -v): v20.0.0
  • npm version (run npm -v): 9.6.4
  • gulp version (run gulp -v): nope

Additional information

The current result is not correct because its use leads to incorrect results in standard Node methods.like path.* or fs.*:

CWD: D:\\OpenSource\\glob-parent

const path = require('path');

path.win32.resolve('D:'); // CWD
path.win32.resolve('D:/'); // D:\\

const fs = require('fs');

fs.readdirSync('D:'); // list CWD
fs.readdirSync('D:/'); // list D:\\
@mrmlnc mrmlnc mentioned this issue Apr 19, 2023
Open
@mrmlnc
Copy link
Contributor Author

mrmlnc commented Apr 19, 2023 •
edited

Unfortunately, I don't know a better fix than the next one:

// https://github.com/gulpjs/glob-parent/blob/3a14ff5125d9fa9614dc214532327711fa6bbd93/index.js#L32

// remove path parts that are globby
do {
  if (isWin32 && !str.includes('\\')) {
    str = pathWindowsDirname(str); // path.win32.dirname
  } else {
    str = pathPosixDirname(str); // path.unix.dirname
  }
} while (isGlobby(str));

It is important for us to understand that C: is a drive on Windows.

The proposed fix also adds support for UNC paths like //?/C:/*.

@phated
Copy link
Member

phated commented Apr 22, 2023

This seems like a good change. I'll look into making it soon (been very busy recently).

@phated phated added this to To do in v5 via automation Apr 23, 2023
@phated phated moved this from To do to Nice to Have in v5 Apr 23, 2023
@phated
Copy link
Member

phated commented Jun 26, 2023

@sttk do you have time to look into this?

@sttk sttk linked a pull request Jul 1, 2023 that will close this issue
Open
@sttk
Copy link
Contributor

sttk commented Jul 1, 2023

@mrmlnc @phated
I've created the PR #64 to fix this issue. Please review it.

@phated phated removed this from Nice to Have in v5 Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
post-v5
Status: No status
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: support windows path with drive or unc volume sttk/glob-parent
3 participants
@phated @mrmlnc @sttk