Added tests for #556.

[dzhuang]

Testing #556 as mentioned in #767. The tests works without problems. The merge of this PR requires dzhuang/relate-sample@f43b734 being merged into sample repo.

P.S., We should mention in docs that path split should always use /.

@inducer

[dzhuang]

This need to be reverted when merging.

[inducer]

Thanks for doing this! I really don't understand why this works though. I tried whether dulwich does path parsing:

from dulwich.repo import Repo
r = Repo(".")
c = r[b"HEAD"]
t = r[c.tree]
f = t[b"pytools/__init__.py"]
print(f)

(run in a checkout of https://github.com/inducer/pytools/, because why not) It does not.

So if this code:

relate/course/content.py

Lines 651 to 653 in f4e8dee

	# https://github.com/inducer/relate/pull/556
	# FIXME: https://github.com/inducer/relate/issues/767
	name_parts = os.path.normpath(full_name).split(os.sep)

splits the path by os.sep (which i assume is \ in Windows, right?), how does it feed the right file names to dulwich? (I don't have a Windows system to check, otherwise I would)

[dzhuang]

OIC. I finally remembered the reason for proposing #556.

The .attribute file won't be parsed correctly in Windows. e.g., the file images/.attributes.yml will be images\.attributes.yml in Windows.

Now I can see this line (os.path.join)lead to that result:

relate/course/content.py

Line 803 in f4e8dee

attributes_path = os.path.join(os.path.dirname(path), ATTRIBUTES_FILENAME)

I didn't realized that when I try to fix this issue, and in #556 the workround went to a wrong direction. Although it actually fixed it, I must admit it's bad and sorry for causing your confusion.

So the better workround is to use "/".join([os.path.dirname(path), ATTRIBUTES_FILENAME]), am I right?

See something interesting in #784

[dzhuang]

FYI, os.path.normpath

>>> os.path.normpath("repo/root/sub")
'repo\\root\\sub'
>>> os.path.normpath(r"repo/root/sub\.attribute") 
'repo\\root\\sub\\.attribute'

normpath will replace all \ and / to os.sep, and then we can use os.sep to split the path to elements.

[inducer]

I propose that we standardize on / as a path separator in course content. I'm not a fan of allowing both (as we would if we used normpath). I agree that using os.path.join to construct repo paths (in the spot you found, but there are a few more) is incorrect, we should fix those instances.

How do you feel about this direction?

[dzhuang]

I agree. But my first attempt failed (see #784). Maybe there're more os.path.join need to be changed as you mentioned.