gita drive root error on Windows

[bcdiaconu]

from this point on gita group executes successfully while gita ll will throw the same exception

OS: Windows 11
Gita ver: 0.16.1.2
Installed with pip

[bcdiaconu]

The issue lays here where it sees parent var definition as a relative path rather than a drive letter

[nosarthur]

I just tried it on my MBP and there was no traceback. Maybe it's specific to windows.

Does the auto context ever work on your machine?

[bcdiaconu]

I just tried it on my MBP and there was no traceback. Maybe it's specific to windows.

Does the auto context ever work on your machine?

yes, auto context works on my machine. its just that i have done gita add -a . while cwd was i:

[bcdiaconu]

I have changed the parent value from "I:" to "I:\\" and it does not raise error anymore. At least not this one.

I'm thinking of adding "\\" if not exists and afterwards remove the trailing one. I am less experienced with Python so I would really like your opinion on it.

Later edit:
it will not work because next time the function is called the parent definition is still malformed (eg: "I:CPP" instead of "I:\\CPP")

[nosarthur]

Did you add \\ in ~/.config/gita/groups.csv? What is the top-level group in that file after running gita add -a . at I:?

Does it work if you just add \, i.e., I:\?

I am not familiar with windows. For *nix machines, a / is guaranteed. Maybe we can add os.sep if it doesn't exist in the group path, when saving the group path?

[nosarthur]

What is the line that triggers the traceback? I tried this on my MBP and it doesn't error out

In [2]: os.path.commonpath(('I:', 'I:'))
Out[2]: 'I:'

In [4]: os.path.commonpath(('I:\\', 'I:'))
Out[4]: ''

[bcdiaconu]

I have replace every line in groups.csv similar to the following:
was:
-MOBILE:openScale:"I:MOBILE"
replaced with:
-MOBILE:openScale:"I:\MOBILE"

with this modification when I am inside a directory and do a gita ll will execute successfully
but if I run the same command in I: it will throw another exception

[bcdiaconu]

os.path.commonpath(('I:\', 'I:'))

on windows side:

[bcdiaconu]

tried again by deleting the two csv files and doing the same operation from within an directory instead of drive root and the context works properly:

[bcdiaconu]

looking at the image above it looks like using the Path var type instead of str is the way forward as it solves a few problems. Some of the more interesting ones being solved for this issue are:

• the file/dir separation char, namely slashes or backslashes
• the malformed D:dir1 to the correct D:\dir1

Important: aways use Path.resolve() before converting to str

[nosarthur]

This seems to be the problem:
https://stackoverflow.com/questions/66907726/why-is-the-absolute-path-of-a-drive-letter-equal-to-the-working-directory

Currently I always strip off the trailing os separator, see __main__.py::_path_name. Maybe we just need to pick the other convention: always keep the trailing os separtor. I think both would work, I probably picked the current one because # Note that os.path.commonpath has no trailing /

Another (easier) solution could be to use relative_to, or is_relative_to in the pathlib. Maybe they handle the windows drive correctly

[bcdiaconu]

Maybe we just need to pick the other convention: always keep the trailing os separtor.

Unfortunately I do may think that is enough. I saw that all paths are malformed eg: d:projects instead of either d:/projects or true Windows style d:\\projects. That is why I think this approach will only solve some usecases and this kind of problems that will be solved on one platform and create problems on another will come over and over to byte in the future. And will be like a continuous and exhausting patching process.

I still believe that the only way to move forward in a stable way is to use what seems pretty reliable and tested Path class. It will require a bit more RAM (but not that much) with a great benefit in terms of stability in the long term.

[bcdiaconu]

Another (easier) solution could be to use relative_to, or is_relative_to in the pathlib. Maybe they handle the windows drive correctly

This looks like it could be a true solution, to use PurePath. If you have an idea on what statements to test, I will gladly do so on Windows side to make sure it is or not the way forward. I will also take a better look at them when I get a bit more time at hand.

[bcdiaconu]

I took a look at PurePath and it contains cross-platform functions that manipulate platform specific paths.

To have a single code-base targeting all platforms, it is required to have cross-platform functions that manipulate cross-platform paths.

So my conclusion is that Path is the way to go. Path also have a relative functionality, namely os.path.relpath

Please let me know if you find it differently.

[nosarthur]

just curious, does it work if you simply replace os.path.commonpath by os.path.commonprefix as in the TODO comments?

[bcdiaconu]

it started really promising. but ruined in the end.

[nosarthur]

what about this one?

def get_relative_path(kid: str, parent: str) -> Union[List[str], None]:
    """
    Return the relative path depth if relative, otherwise MAX_INT.

    Both the `kid` and `parent` should be absolute paths
    """
    if parent == "":
        return None

    p_kid = Path(kid)
    # p_kid = Path(kid).resolve()
    try:
        p_rel = p_kid.relative_to(parent)
    except ValueError:
        return None
    rel = str(p_rel).split(os.sep)
    if rel == ["."]:
        rel = []
    return rel

[bcdiaconu]

exactly, expected results were right. but coming back with another try

[nosarthur]

these are too fake, can you gita clear, and then do that gita add -a at I:?

[bcdiaconu]

def get_relative_path_w_resolve(kid: str, parent: str) -> Union[List[str], None]:
    """
    Return the relative path depth if relative, otherwise MAX_INT.

    Both the `kid` and `parent` should be absolute paths
    """
    if parent == "" or kid == "":
        return None

    p_kid = Path(kid).resolve()
    p_parent = Path(parent).resolve()
    try:
        p_rel = p_kid.relative_to(p_parent)
    except ValueError:
        return None
    rel = str(p_rel).split(os.sep)
    if rel == ["."]:
        rel = []
    return rel

this however will bring expected results, if works as expected also on linux:

[nosarthur]

I think your test cases are a bit too unreal / strict. In reality they won't be of those variations. The absolute paths with os.sep are saved

Does my version solves the very first problem you reported? I am not sure if resolve() is needed (I suspect it's not needed since the kid and parent are already absolute with os.sep)

[bcdiaconu]

I suspect it's not needed since the kid and parent are already absolute with os.sep

Well, tests made on Path have shown differently (see Path tests ). And the csv file contains some unnatural Windows path formats

I think your test cases are a bit too unreal / strict.

Indeed the should be unreal. However, the real world showed they are quite real (see csv contents). And the strictness in testcases bring a working solution in a large set of real-world situation (which should indeed be unreal)

Does my version solves the very first problem you reported?

I will try. I need to get back to job now. I'll get back to you later

[bcdiaconu]

I will try. I need to get back to job now. I'll get back to you later

A PR with how you want your function to be integrated would be good, which I shall test on Windows side.

[nosarthur]

please try v0.16.2.1, it corresponds to PR #219

[bcdiaconu]

a big step forward but still some issues with passing the path as an cli arg. different behavior in passing d: and d:\

[nosarthur]

what does os.path.abspath return for d: and d:\ on your machine?

[nosarthur]

also, what does the top-level group look like in ~/.config/gita/group.csv for d: and d:\?

[bcdiaconu]

I see you merged the PR so it seems it makes no sense to give further feedback

[nosarthur]

I merged it since it's already makes it better. We can still keep improving

According to this stackoverflow thread, d: means cwd on the d drive. Thus we are getting the expected result

https://stackoverflow.com/questions/66907726/why-is-the-absolute-path-of-a-drive-letter-equal-to-the-working-directory

[bcdiaconu]

According to this stackoverflow thread, d: means cwd on the d drive. Thus we are getting the expected result

Indeed. Testing in cmd/powershell it turns out it is the work-dir on the d drive. at the same time it seems that each drive has a working-directory and when switching the drive you switch the current working directory to the working-directory of that drive. so indeed it is behaving correctly from that perspective.

but still there is an issue as seen below:

[nosarthur]

Could you save the ~/.config/gita/groups.csv in these two cases and see which 2 groups are missing?

Or we could stop here since d: is a somewhat problematic usage. Or I can match input x: and add \ to them

[bcdiaconu]

the contents of the files are quite different. one more thing that might make a bit of light is that some groups have at least 1 entry like - -. or in other words, a repo named - or a list inside a list

[nosarthur]

hmm, I am leaning towards leave it as is, or add \ to x:. do you have any preference?

[bcdiaconu]

It is fine as is. The\ will not solve this.

The problem seems to be another issue altogether. I created the 220 issue for that