Skip to content

Rename WorkingDirectoryBackingRootPath constants#1119

Merged
chrisd8088 merged 2 commits intomicrosoft:masterfrom
github:rename-workdir-backing-path
May 9, 2019
Merged

Rename WorkingDirectoryBackingRootPath constants#1119
chrisd8088 merged 2 commits intomicrosoft:masterfrom
github:rename-workdir-backing-path

Conversation

@chrisd8088
Copy link
Contributor

@chrisd8088 chrisd8088 commented May 8, 2019

This PR primarily is a followup to #1102 to accommodate the fact that on Linux, the relative location of the working directory backing store will be a short path (i.e., .gvfs/lower) and not a single filename like src. Hence we rename the WorkingDirectoryBackingRootName constants to WorkingDirectoryBackingRootPath, akin to how other short relative path constants are constructed in GVFS.Common.GVFSConstants.DotGVFS.

We also fix up the internal name of the workingDirectoryBackingRoot argument to the GVFS.Common.Enlistment class; it was possibly missed in #1102 because of the lowercase l in localStorageRoot.

And we add a TODO comment regarding the git.cmd script, which is presumably unused on non-Windows platforms and would be ignored even if renamed git as it's unlikely to be in the user's PATH.

These housekeeping changes are all in preparation, along with #1104 (DONE), #1123, and #1128 (still WIP) for #1125, which adds the GVFS.Platform.Linux classes and related build tooling.

/cc @jrbriggs, @kivikakk

Since on Linux the relative location of the working directory
backing store will be a short path (i.e., ".gvfs/lower") and
not a single filename, rename the WorkingDirectoryBackingRootPath
constants appropriately.

Also fixup the internal name of the workingDirectoryBackingRoot
argument to the common Enlistment class.
return new Result(true);
}

// TODO(Linux), TODO(Mac): either adjust to "git" or remove entirely
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call! An equivalent shell script could be:

#!/bin/sh

echo This repo was cloned using VFS for Git, and the git repo is in the 'src' directory
echo Switching you to the 'src' directory and rerunning your git command

cd src
git "$@"

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, there are some special characters in the git.cmd that probably do color stuff, but I'm too lazy to figure out how to do that in POSIX.

Copy link
Contributor Author

@chrisd8088 chrisd8088 May 8, 2019

Choose a reason for hiding this comment

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

Well, and the other consideration is that where Windows will (I believe; I'm no expert) search the current directory first for an executable, on Unix systems, unless someone happens to have put . in their PATH (which is generally frowned upon), that's unlikely.

So I'm not sure if there's really any value to this kind of "redirector" script on Mac or Linux, even if we named it git. Plus I feel like it would be confusing for folks who know that VFSForGit requires a custom git binary to work properly if we also have a script named the same thing but which isn't actually that binary.

Anyway, I just wanted to flag it because it was something I bumped into along the road to GVFS.Platform.Linux. :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point! Then, it's worth leaving as a TODO until we have a concrete decision about what to do with this step. Apparently, it was an important feature for users confused about where to go after a gvfs clone step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could also open an issue on this to track it for future work, if that's useful.

Copy link
Member

Choose a reason for hiding this comment

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

I could also open an issue on this to track it for future work, if that's useful.

I think it's worthwhile to have an issue tracking this, could you file one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done as #1138 -- thanks!

Add a TODO note regarding the "git.cmd" script which is ignored on
macOS (and also Linux) as it's not in $PATH and will not override
the "git" binary because the ".cmd" extension is not ignored as it
is on Windows.
@chrisd8088 chrisd8088 force-pushed the rename-workdir-backing-path branch from fa75ff7 to 8c366d0 Compare May 8, 2019 20:17
@chrisd8088 chrisd8088 changed the title Rename WorkingDirectoryBackingRootPath constants and fixup uid_t to uint Rename WorkingDirectoryBackingRootPath constants May 8, 2019
@chrisd8088 chrisd8088 marked this pull request as ready for review May 8, 2019 21:13
@chrisd8088 chrisd8088 merged commit 2258265 into microsoft:master May 9, 2019
@chrisd8088 chrisd8088 deleted the rename-workdir-backing-path branch May 9, 2019 21:29
@chrisd8088
Copy link
Contributor Author

Thanks very much @wilbaker!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants