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

"scalar unregister" cannot work against a registered but non-existing enlistment with its absolute path. #4200

Closed
1 task done
elpiekay opened this issue Dec 27, 2022 · 4 comments · Fixed by #4253
Closed
1 task done
Assignees
Milestone

Comments

@elpiekay
Copy link

  • I was not able to find an open or closed issue matching what I'm seeing

Setup

  • Which version of Git for Windows are you using? Is it 32-bit or 64-bit?
$ git --version --build-options

git version 2.39.0.windows.2
cpu: x86_64
built from commit: e7d4c504802f93f5914ed48944ced4e593bcaf6c
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
  • Which version of Windows are you running? Vista, 7, 8, 10? Is it 32-bit or 64-bit?
$ cmd.exe /c ver

Microsoft Windows [Version 10.0.19042.1165]
  • What options did you set as part of the installation? Or did you choose the
    defaults?
# One of the following:
> type "C:\Program Files\Git\etc\install-options.txt"
> type "C:\Program Files (x86)\Git\etc\install-options.txt"
> type "%USERPROFILE%\AppData\Local\Programs\Git\etc\install-options.txt"
> type "$env:USERPROFILE\AppData\Local\Programs\Git\etc\install-options.txt"
$ cat /etc/install-options.txt

Editor Option: VIM
Custom Editor Path:
Default Branch Option:
Path Option: Cmd
SSH Option: OpenSSH
Tortoise Option: false
CURL Option: OpenSSL
CRLF Option: LFOnly
Bash Terminal Option: MinTTY
Git Pull Behavior Option: Merge
Use Credential Manager: Enabled
Performance Tweaks FSCache: Enabled
Enable Symlinks: Disabled
Enable Pseudo Console Support: Enabled
Enable FSMonitor: Enabled
  • Any other interesting things about your environment that might be related
    to the issue you're seeing?
No.

Details

  • Which terminal/shell are you running Git from? e.g Bash/CMD/PowerShell/other
Bash
# Case 1
cd /c/
rm -rf foo
git init foo
scalar register foo
scalar list
# We can see the enlistment "C:/foo"
scalar unregister /c/foo
scalar list
# The enlistment "C:/foo" is gone from the list.

# Case 2
cd /c/
rm -rf foo
git init foo
scalar register foo
scalar list
# We can see the enlistment "C:/foo"
scalar unregister foo
scalar list
# The enlistment "C:/foo" is gone from the list.

# Case 3
cd /c/
rm -rf foo
git init foo
scalar register foo
scalar list
# We can see the enlistment "C:/foo"
rm -rf foo
scalar unregister foo
scalar list
# The enlistment "C:/foo" is gone from the list.

# Case 4
cd /c/
rm -rf foo
git init foo
scalar register foo
scalar list
# We can see the enlistment "C:/foo"
rm -rf foo
scalar unregister /c/foo
scalar list
# The enlistment "C:/foo" is still in the list.
  • What did you expect to occur after running these commands?

In Case 1, we can unregister an enlistment with its absolute path if the path exists.
In Case 2, we can unregister an enlistment with its relative path if the path exists.
In Case 3, we can unregister an enlistment with its relative path even if the path has been removed by rm -rf.
In Case 4, we cannot unregister an enlistment with its absolute path if the path has been removed by rm -rf. The enlistment is expected to be unregistered. So far as I've tried, in order to unregister it, we have to re-create the path first, by mkdir -p /c/foo or git init /c/foo. As in Case 3, we can also use its relative path.

  • What actually happened instead?

The command scalar unregister /c/foo does not unregister the enlistment C:/foo which has been removed by rm -rf.

  • If the problem was occurring with a specific repository, can you provide the
    URL to that repository to help us with testing?

It's not occurring with a specific repository.

@dscho
Copy link
Member

dscho commented Jan 27, 2023

Case 4

cd /c/
rm -rf foo
git init foo
scalar register foo
scalar list

We can see the enlistment "C:/foo"

rm -rf foo
scalar unregister /c/foo
scalar list

The enlistment "C:/foo" is still in the list.

I can reproduce this, but I cannot reproduce it in Git's test suite with this diff:

diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
index 25f500cf682a..5697d350d3f4 100755
--- a/t/t9210-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -119,7 +119,17 @@ test_expect_success 'scalar unregister' '
 	! grep -F "$(pwd)/vanish/src" scalar.repos &&
 
 	# scalar unregister should be idempotent
-	scalar unregister vanish
+	scalar unregister vanish &&
+
+	# should also work with an absolute path
+	git init vanish/src &&
+	scalar register vanish/src &&
+	scalar list >scalar.repos &&
+	grep -F "$(pwd)/vanish/src" scalar.repos &&
+	rm -rf vanish &&
+	scalar unregister "$PWD/vanish" &&
+	scalar list >scalar.repos &&
+	! grep -F "$(pwd)/vanish/src" scalar.repos
 '
 
 test_expect_success 'set up repository to clone' '

I suspect it might have something to do with C:/foo being at the drive root.

dscho added a commit to dscho/git that referenced this issue Jan 27, 2023
Let's be careful to make this function work near the drive root: to
resolve the drive root path itself, we _need_ a trailing backslash:
if a file handle to the path `C:` is created, it does not actually refer
to the drive root. Instead, it refers to this very Windows-only concept
of a "per-drive current directory".

This also requires the code that wants to re-append the last component
to be more careful and only append a slash _if necessary_.

This commit fixes the problem with `scalar unregister C:/foo` that was
reported at git-for-windows#4200.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho
Copy link
Member

dscho commented Jan 27, 2023

I suspect it might have something to do with C:/foo being at the drive root.

And that's exactly what it was. #4253 has a fix for this, even if the description and the fix sound as if they are completely unrelated to Scalar. The root cause simply has nothing to do with Scalar 😁

@dscho dscho self-assigned this Jan 27, 2023
dscho added a commit that referenced this issue Jan 28, 2023
…root (#4253)

This PR fixes a pretty fundamental problem where
`win32_strbuf_realpath()` mishandled non-existing paths that are close
to the drive root, e.g. `C:/foo`.

This fixes #4200
@dscho
Copy link
Member

dscho commented Jan 28, 2023

/add relnote bug Some commands mishandled absolute paths near the drive root (e.g. scalar unregister C:/foo), which has been fixed.

The workflow run was started

github-actions bot pushed a commit to git-for-windows/build-extra that referenced this issue Jan 28, 2023
Some commands mishandled absolute paths near the drive root (e.g.

C:/foo`](git-for-windows/git#4200)), which has
Signed-off-by: gitforwindowshelper[bot] <gitforwindowshelper-bot@users.noreply.github.com>
@dscho dscho added this to the Next release milestone Feb 1, 2023
@dscho
Copy link
Member

dscho commented Feb 8, 2023

/add relnote blurb As announced previously, Git for Windows will drop support for Windows 7 and for Windows 8 in one of the next versions, following Cygwin's and MSYS2's lead (Git for Windows relies on MSYS2 for components such as Bash and Perl).

The workflow run was started

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants