[nodefs] Don't try to resolve absolute symlinks outside of the VFS #21805

mho22 · 2024-04-23T13:15:04Z

This PR reverts the behaviour that was added in #3277 and suggested in #3222.

…path

sbc100 · 2024-04-24T15:13:58Z

src/library_fs.js

@@ -904,7 +904,8 @@ FS.staticInit();` +
      if (!link.node_ops.readlink) {
        throw new FS.ErrnoError({{{ cDefs.EINVAL }}});
      }
-      return PATH_FS.resolve(FS.getPath(link.parent), link.node_ops.readlink(link));
+      var parent = PATH_FS.resolve(FS.getPath(link.parent), link.node_ops.readlink(link)) === path ? link.parent.parent : link.parent;


Can you construct a test case that shows why this is needed?

Perhaps modify test/unistd/links.c.

Also can you explain in more detail why this is needed? I'm strugglying the understanding why sometimes we resolve relative to one path and sometimes relative to another

I'll try my best to explain my use case. Based on the scheme I gave above, with the current behavior, I can't access the directory store outside of the directory public where the symlink is. The symlink is "blocked" in the public directory.

As it is my first emscripten contribution, and as I use emscripten from within another tool, I don't know if I can reproduce this in test/unistd/links.c, but if you have some steps or suggestions, I would be glad to make things work.

This suggestion is based on this issue

In the above scheme what is value of the symlink in public/store? I.e. where is it pointing? i.e. Is it pointing to ../store ? i.e. what call did you use to create the link?

Thank you for the context, WordPress/wordpress-playground#1283 looks like it has more info

@sbc100 The value of the public/store symlink when I run ls -la in the public directory returns an absolute path /Users/.../test/store while my test folder name is test.

I used the php method symlink() to create the symlink.

I hope this helps.

I used this script :

test/script.php

<?php symlink( __DIR__.'/store', __DIR__.'/public/store' );

and ran the script in the test folder : php script.php

I see.. and presumably both of those are absolute paths?

That means that target of the symbol is absolute (/full/path/to/public/store) and not relative (../public/store)? Is that what you want? Does the problem go away if you use a relative symlink?

You're right! It works perfectly with relative paths! But unfortunately, I'm dealing with absolute paths in my current situation. A situation where I am not the one who created the symlinks. Do you think it's still worth pursuing?

Yes, this is worth fixing, I'm was just trying to figure out the set of circumstances in which this occurs.

I should try different use cases then. My suggestion probably works if the symlink path leads to a file in parent directories. But what if the symlink path leads to children directories within a parent directory. I should investigate.

mho22 · 2024-04-25T10:00:17Z

@sbc100 I ended up removing the suggestion I made. Because it didn't work for other use cases. But I dug into code to find out where it could be problematic and tried to correct these lines with multiple use cases in mind.

Here are the use cases I based my PR on :

// USE CASE 0 RELATIVE PATH : a parent's neighbour directory in a child directory
symlink( '../relative', './public/relative' );

// USE CASE 1 RELATIVE PATH : A child directory in a parent directory
symlink( './directory/subdirectory', './public/subsubrelative' );

// USE CASE 2 RELATIVE PATH : a parent's child directory in a child's subdirectory
symlink( '../../relative/subrelative', './public/directory/subrelative' );

// USE CASE 3 ABSOLUTE PATH : a parent's neighbour directory in a child directory
symlink( __DIR__ . '/absolute', __DIR__ . '/public/absolute' );

// USE CASE 4 ABSOLUTE PATH : A child directory in a parent directory
symlink( __DIR__ . '/public/directory/subdirectory', __DIR__ . '/public/subsubabsolute' );

// USE CASE 5 ABSOLUTE PATH : a parent's child directory in a child's subdirectory
symlink( __DIR__ . '/absolute/subabsolute', __DIR__ . '/public/directory/subabsolute' );

My use cases are using emscripten directly within @php-wasm/node so those lines are php. I have no clue how to implement this in a test for emscripten.

sbc100 · 2024-04-25T15:15:08Z

@sbc100 I ended up removing the suggestion I made. Because it didn't work for other use cases. But I dug into code to find out where it could be problematic and tried to correct these lines with multiple use cases in mind.

Here are the use cases I based my PR on :
// USE CASE 0 RELATIVE PATH : a parent's neighbour directory in a child directory
symlink( '../relative', './public/relative' );

// USE CASE 1 RELATIVE PATH : A child directory in a parent directory
symlink( './directory/subdirectory', './public/subsubrelative' );

// USE CASE 2 RELATIVE PATH : a parent's child directory in a child's subdirectory
symlink( '../../relative/subrelative', './public/directory/subrelative' );

// USE CASE 3 ABSOLUTE PATH : a parent's neighbour directory in a child directory
symlink( __DIR__ . '/absolute', __DIR__ . '/public/absolute' );

// USE CASE 4 ABSOLUTE PATH : A child directory in a parent directory
symlink( __DIR__ . '/public/directory/subdirectory', __DIR__ . '/public/subsubabsolute' );

// USE CASE 5 ABSOLUTE PATH : a parent's child directory in a child's subdirectory
symlink( __DIR__ . '/absolute/subabsolute', __DIR__ . '/public/directory/subabsolute' );
My use cases are using emscripten directly within @php-wasm/node so those lines are php. I have no clue how to implement this in a test for emscripten.

Do you know which of these 6 cases fail currently with emscripten and which succeed?

mho22 · 2024-04-25T15:48:51Z

@sbc100 without the modifications I made in this PR, the three first succeed and the three last fail, because these 3 last use absolute paths. But with the modifications I added with my last commit, all of the uses cases are working.

mho22 · 2024-04-27T11:17:27Z

@sbc100 I reduced the code modification to one simple line in src/library_nodefs.js on line 238:

- path = nodePath.relative(nodePath.resolve(node.mount.opts.root), path);
+ path = nodePath.relative( PATH.isAbs(path) ? NODEFS.realPath(node.parent) : nodePath.resolve(node.mount.opts.root), path );

This will check if the symlink path is absolute or relative. If it is relative, it runs the already working code, if it is absolute, it takes the parent path instead of the root path and returns its new relative path.

Here is an example of this behavior with a relative path based on the structure I gave above [ Assuming this is in a test directory at the root of the machine ] :

- test
    - public
        - directory
        - store ~symlink~
    - store

If store symlink is a relative path, it should be : ../store

So the line above will be :

path = nodePath.relative( '/test', '../store' ); returning ../store. This is actually working fine.

If store symlink is an absolute path, it should be : /test/store

This is the current behavior :

path = nodePath.relative( '/test', '/test/store' ); returning store.

This is not working. Because it then concatenates store with the parent path being /test/public resulting in /test/public/store and calling again readlink leading to an almost indefinite loop.

So the modification above will bring this :

path = nodePath.relative( '/test/public', '/test/store' ); returning ../store.

This transforms the absolute path in a relative path that we know works completely fine.

I believe this is correct, but for some reason, these tests are failing :

test_minimal_runtime_code_size_random_printf_wasm 
test_minimal_runtime_code_size_random_printf_wasm2js
test_externref

Feel free to share any insights if any.

mho22 · 2024-04-30T08:47:54Z

@sbc100 Now it seems only this test is failing :

test_embind_i64_val

Maybe linked with the issue #21653 you opened a month ago.

I remarked latest merges also had a test_embind_i64_val so I suppose this is a normal behavior.

mho22 · 2024-05-03T14:33:19Z

@sbc100 Hi ! I merged the 'main' branch into the PR and every test passed. What do you think I should do next ?

sbc100 · 2024-05-03T15:31:58Z

src/library_nodefs.js

@@ -235,7 +235,7 @@ addToLibrary({
        var path = NODEFS.realPath(node);
        try {
          path = fs.readlinkSync(path);
-          path = nodePath.relative(nodePath.resolve(node.mount.opts.root), path);
+          path = nodePath.relative( PATH.isAbs(path) ? NODEFS.realPath(node.parent) : nodePath.resolve(node.mount.opts.root), path );


I wonder if we should even be calling nodePath.relative at all? What happens if you just remove this line?

Without this line, the resolve method which works correctly with relative paths will return wrong paths when using absolute paths. The idea with this line is to return a relative path in any case.

But is that how readlink is supposed to work? If the link as absolute then shouldn't readlink return an absolute path?

@sbc100 You're correct. Removing this line resolved the issues I was experiencing.

It looks like issue #3222 was the reason for this line.

Hmm.. from my reading of #3222 and the fix that landed with the test_unistd_symlink_on_nodefs test, i don't think it is correct. See https://github.com/emscripten-core/emscripten/pull/21805#issuecomment-2094926239 below

sbc100 · 2024-05-03T15:33:14Z

Can you take the use cases you created above and add them to test/unistd/links.c. You can then run that test with ./test/runner.py other.test_unistd_links_nodefs

mho22 · 2024-05-05T11:22:54Z

@kleisauke and @sbc100 I pushed the code without that line and indeed the test_unistd_symlink_on_nodefs didn't pass.

The line 21 in file test/unistd/symlink_on_nodefs.c :

fs.symlinkSync(fs.realpathSync('./new-directory'), './symlink');

Inserts an absolute path in symlink. Therefore it couldn't be recognized except if you transform that absolute path in a relative path based on the mounted root in file src/library_nodefs.js:

path = nodePath.relative(nodePath.resolve(node.mount.opts.root), path);

The test/unistd/symlink_on_nodefs.c test pass if the line is replaced :

- fs.symlinkSync(fs.realpathSync('./new-directory'), './symlink');
+ fs.symlinkSync('./new-directory', './symlink');

Obviously.

So the absolute path issue still remains. Except if I replace :

- path = nodePath.relative(nodePath.resolve(node.mount.opts.root), path);
+ path = nodePath.relative( PATH.isAbs(path) ? NODEFS.realPath(node.parent) : nodePath.resolve(node.mount.opts.root), path );

this readlink method should return a relative path to make symlinks with absolute paths work. Maybe should we rename library_nodefs readlink function name to be more explicit ?

sbc100 · 2024-05-05T19:44:28Z

Having looked at test_unistd_symlink_on_nodefs I think the test is invalid. Absolute symlinks to locations outside of the VFS tree should not be supported and should not be magically translated into relative symlinks.

mho22 · 2024-05-06T08:32:22Z

@sbc100 Ok then, I'll modify the test test_unistd_symlink_on_nodefs like indicated above with an absolute path from the VFS tree and remove the path line in file src/library_nodefs.js . I think I should add the tests you asked earlier in the test_unistd_symlink_on_nodefs test file since it has a lot of similarities with it.

mho22 · 2024-05-06T11:03:21Z

@sbc100 I suggested a new test to verify a inside VFS tree and outside VFS tree symlinks. One will succeed opening the file and the other will fail. Besides these new tests, you'll have the relative and absolute paths tests asked earlier.

Anyways, only one test failed in test-browser-chrome-wasm64 :

======================================================================
ERROR [61.438s]: test_offset_converter (test_browser.browser64)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/project/test/common.py", line 2088, in run_browser
    self.assertContained(expected, output)
  File "/root/project/test/common.py", line 1418, in assertContained
    additional_info
  File "/usr/lib/python3.6/unittest/case.py", line 670, in fail
    raise self.failureException(msg)
AssertionError: Expected to find '/report_result?exit:1
' in '[no http server activity]
', diff:

--- expected
+++ actual
@@ -1 +1 @@
-/report_result?exit:1
+[no http server activity]

I suppose this is probably not linked with this PR.

sbc100 · 2024-05-06T16:27:39Z

test/unistd/links.c

  mkdir("working", 0777);
  chdir("working");
  symlink("../test/../there!", "link");
-  int fd = open("file", O_RDWR);
+  fd = open("file", O_RDWR);


Looks unrelated.. can this be reverted?

@sbc100 My bad. I'm on it.

sbc100 · 2024-05-06T16:29:12Z

test/unistd/symlink_on_nodefs.c

+    FS.mount(NODEFS, { root: './inside-symlink' }, 'direct-inside-link');
+
+    FS.mkdir('direct-outside-link');
+    FS.mount(NODEFS, { root: './outside-symlink' }, 'direct-outside-link');


Thanks for looking into this! I also put up a change to simplify this test a little: #21890

I will copy your changes.

sbc100 · 2024-05-06T16:36:22Z

test/unistd/links.c

@@ -31,10 +31,11 @@ void setup() {
    FS.mkdir('folder');


I think its still worth extending this test to include some absolute symlinks.

I don't know what I can do here that is not already done in the test/unistd/symlink_on_nodefs.c.

I would like to see some tests or absolute symlinks added to test/unistd/links.c.

test/unistd/links.c it where we run as many of the symlink tests as possible. symlink_on_nodefs I think should be reserved to tests that we can't run outside of nodefs (i.e. tests that involve using non-trivial mounting setups that require nodefs to pass).

One reason that we should use links.c where possible is that its actually possible to run links.c on native desktop systems to confirm they have the same behaviour as emscripten (i.e. to check emscripten is doing the right thing).

Perhaps symlink_on_nodefs.c should be renamed to something better.. but I can't think of anything. Basically its for tests that can't run outside of nodefs.

@sbc100 Ok, I understand. I should probably duplicate the tests :

test_relative_path_symlinks(); test_absolute_path_symlinks();

from symlink_on_nodefs.c based on the symlink system from links.c in setup() function.

Would you like me to refactor the code readability in links.c as well like you indicated below ? :

int main() { setup(); test_foo_bar(); test_baz(); return 0; }

I think so yes, but maybe wait for #21890 to land to avoid too many conflicts.

Actually I wonder if we even need symlink_on_nodefs.c at all anymore? Since it won't really be testing anything will it? Other than "symlinks to outside the VFS don't work" which is basically the same as "symlinks to non-existant files don't work"? Or is it will worth keeping it around do you think?

My understanding is that this use case and this test could probably be an indicator that this is not possible and that it shouldn't be possible. So we could imagine that the only test that the symlink_on_nodefs.c could have are :

test_inside_symlink(); test_outside_symlink(); test_mount_link();

The others will be moved to links.c ?

What I find mostly interesting here is the proof that this has been decided to work like this. With this use :

fs.symlinkSync(fs.realpathSync('directory'), 'outside-symlink');

sbc100 · 2024-05-06T16:41:25Z

test/unistd/symlink_on_nodefs.c

+      printf("buffer is %s\n", buffer);
+      fclose(fd);
+    }
+  }


Can you put each of these test "block" into their own function with a specific name? e.g.

int main() { setup(); test_foo_bar(); test_baz(); return 0; }

I updated it.

sbc100 · 2024-05-06T16:45:53Z

test/unistd/symlink_on_nodefs.c

-    FS.mkdir('direct-link');
-    FS.mount(NODEFS, { root: './symlink' }, 'direct-link');
+    FS.mkdir('direct-inside-link');
+    FS.mount(NODEFS, { root: './inside-symlink' }, 'direct-inside-link');


Is "direct" here short for directory "directory" if so maybe call "dir" so as not to be confused with that "direct" adjective?

Better still since these are mount points how about calling them "mount-inside-symlink" ?

I modified it.

mho22 · 2024-05-06T17:57:51Z

@sbc100 I made the corrections you asked, maybe did I miss something. Don't hesitate to give me other insights to improve the code readability.

mho22 · 2024-05-07T05:51:05Z

@sbc100 I merged and pushed the changes you requested and kept the symlink_on_nodefs.c test file with the remaining three tests :

test_inside_symlink
test_outside_symlink
test_mount_link

What do you think about this ?

sbc100

This is looking like a really awesome change now! Thanks for working on this.

sbc100 · 2024-05-07T20:05:36Z

test/unistd/links.c

+  }
+}
+
+void test_absolute_path_symlinks()


Can you put the opening curly braces on the same line as the function decl?

Sorry, on it.

sbc100 · 2024-05-07T20:09:33Z

test/unistd/symlink_on_nodefs.c

-  // mounted
-  fd = fopen("/direct-link/test", "r");
+void test_outside_symlink()
+{


Maybe add a comment here.

// outside-symlink is link to an absolute path which is not part of the emscripten VFS // and so we should be able to open it.

sbc100 · 2024-05-07T20:09:49Z

test/unistd/symlink_on_nodefs.c

-  fd = fopen("/direct-link/test", "r");
+void test_outside_symlink()
+{
+  FILE* fd = fopen("/working/outside-symlink/test", "r");
  assert(fd == NULL);


assert(errno == ENOENT)

sbc100 · 2024-05-07T20:13:09Z

test/unistd/links.c

@@ -31,25 +31,66 @@ void setup() {
  rtn = write(fd, "test", 5);
  assert(rtn == 5);
  close(fd);
-  rtn = mkdir("folder", 0777);
+  rtn = mkdir("directory", 0777);


Make we can use makedir(name) and makefile(name, content) helpers to make this setup more readable?

(also makelink so that we can add the assert(rtn == 0) for those symlink calls)

Should I replace every mkdir function for a makedir one, write function for a makefile and a makelink for any symlink function ?

Yes, that is what I was thinking.

Then it would read something like:

makedir("test"): makefile("test/file", "content"); makelink("file1", "file2");

sbc100

Could you also add an entry to to the ChangeLog, mentioning that this essentially reverts a previous change in behaviour?

mho22 · 2024-05-07T23:33:56Z

Could you also add an entry to to the ChangeLog, mentioning that this essentially reverts a previous change in behaviour?

@sbc100 Is this changelog ok for you ? :

3.1.61 - 05/08/24
-----------------
- Reverts the behaviour producing generic relative paths on symlinks.
  See (#3277) or (#3222).

sbc100 · 2024-05-08T07:03:25Z

Could you also add an entry to to the ChangeLog, mentioning that this essentially reverts a previous change in behaviour?

@sbc100 Is this changelog ok for you ? :
3.1.61 - 05/08/24
-----------------
- Reverts the behaviour producing generic relative paths on symlinks.
  See (#3277) or (#3222).

Can you mention that this is nodejs behaviour?

sbc100 · 2024-05-08T07:05:19Z

Could you also add an entry to to the ChangeLog, mentioning that this essentially reverts a previous change in behaviour?

@sbc100 Is this changelog ok for you ? :
3.1.61 - 05/08/24
-----------------
- Reverts the behaviour producing generic relative paths on symlinks.
  See (#3277) or (#3222).
Can you mention that this is nodejs behaviour?

How about:

- Under nodefs, symbolic links to files outside of mount locations no longer work.
  This reverts the previous behaviour added in #3277. (#21805)

mho22 · 2024-05-08T08:00:59Z

@sbc100 I added the changes you requested and added the changedir function to make the setup() method a little bit clearer. I hope this change meets your expectations.

sbc100 · 2024-05-08T18:01:22Z

ChangeLog.md

@@ -524,7 +529,7 @@ See docs/process.md for more on how version tagging works.
  (#18861)
 - The `emscripten_proxy_async_with_callback` API was replaced with a simpler
  `emscripten_proxy_callback` API that takes a second callback to be called if
-  the worker thread dies before completing the proxied work.  
+  the worker thread dies before completing the proxied work.


Can you revert all these unralated changes to the ChangeLog where newlines are removed?

BTW, the way I setup my editor is to (1) show trailing whitespace and (2) have a shortcut to strip it all, but not to have it do this automatically.. otherwise you end up with changes like this.

Thanks for the insights and sorry for the inconvenience.

I indeed had these three lines in my vscode/settings.json file enabled :

"files.insertFinalNewline": true, "files.trimFinalNewlines": true, "files.trimTrailingWhitespace": true,

I should be more careful.

sbc100

LGTM.. just two ultra minor nits.

sbc100 · 2024-05-08T18:02:43Z

test/unistd/links.c

-  int fd = open("file", O_RDWR | O_CREAT, 0777);
+}
+
+void makefile(char *file, char *content) {


Use const char * for both args here and args below.

mho22 · 2024-05-08T18:25:52Z

@sbc100 Minor changes pushed. This was a long conversation 🙂. Thank you for your time!

emscripten-core#21805 created a new entry, 3.1.61 (with release data 5/8), while we haven't released 3.1.60. Putting the entry back into 3.1.60.

#21805 created a new entry, 3.1.61 (with release data 5/8), while we haven't released 3.1.60. Putting the entry back into 3.1.60.

readlink method returns grand parent when parent path equals current …

a5fe8cd

…path

mho22 mentioned this pull request Apr 23, 2024

php-wasm : Symlink infinite loop WordPress/wordpress-playground#1283

Open

sbc100 reviewed Apr 24, 2024

View reviewed changes

mho22 and others added 2 commits April 25, 2024 10:27

Merge branch 'emscripten-core:main' into patch-1

7d672ac

Improved readlink behavior to accept absolute paths

5941467

Improved readlink behavior to accept absolute paths

ca48b26

Improved readlink behavior to understand absolute paths

a39210b

Merge branch 'main' into patch-1

521a95e

mho22 added 3 commits May 2, 2024 10:48

Merge branch 'main' into patch-1

122ff6a

Merge branch 'main' into patch-1

26a514a

Merge branch 'main' into patch-1

5300efc

sbc100 reviewed May 3, 2024

View reviewed changes

mho22 added 3 commits May 4, 2024 18:59

Cleared readlink relative path manipulation

38c0631

Merge branch 'patch-1' of github.com:mho22/emscripten into patch-1

5abe4b7

Improved readlink behavior to understand absolute paths

fdf4a4c

mho22 added 2 commits May 5, 2024 13:40

Improved readlink behavior to understand absolute paths

bd746f1

Improved readlink behavior to understand absolute paths

94b6d6d

Improved readlink behavior to understand absolute paths

8eefae8

sbc100 reviewed May 6, 2024

View reviewed changes

sbc100 mentioned this pull request May 6, 2024

NODEFS cannot read files from sparse disk image when mounted with absolute path under OSX #3222

Closed

mho22 changed the title ~~[nodefs] Don't try to resolve absolute symlinks are outside of the VFS~~ [nodefs] Don't try to resolve absolute symlinks outside of the VFS May 6, 2024

Revert resolving outside VFS absolute symlinks

d51fe58

Revert resolving outside VFS absolute symlinks

445b52a

Revert resolving outside VFS absolute symlinks

271c523

sbc100 reviewed May 7, 2024

View reviewed changes

Revert resolving outside VFS absolute symlinks

82298ce

sbc100 reviewed May 7, 2024

View reviewed changes

Revert resolving outside VFS absolute symlinks

a340985

mho22 and others added 2 commits May 8, 2024 12:03

Revert resolving outside VFS absolute symlinks

faaa6f7

Merge branch 'main' into patch-1

4a81b4b

sbc100 reviewed May 8, 2024

View reviewed changes

sbc100 approved these changes May 8, 2024

View reviewed changes

mho22 added 2 commits May 8, 2024 20:19

Revert resolving outside VFS absolute symlinks

0ef6e4f

Merge branch 'patch-1' of github.com:mho22/emscripten into patch-1

bf23b63

sbc100 enabled auto-merge (squash) May 8, 2024 19:05

sbc100 merged commit b632812 into emscripten-core:main May 8, 2024
29 checks passed

mho22 deleted the patch-1 branch May 8, 2024 20:19

aheejin added a commit to aheejin/emscripten that referenced this pull request May 8, 2024

Put VFS ChangeLog into 3.1.60

387a821

emscripten-core#21805 created a new entry, 3.1.61 (with release data 5/8), while we haven't released 3.1.60. Putting the entry back into 3.1.60.

aheejin mentioned this pull request May 8, 2024

Put VFS ChangeLog into 3.1.60 #21911

Merged

sbc100 pushed a commit that referenced this pull request May 8, 2024

Put VFS ChangeLog into 3.1.60 (#21911)

d7507fe

#21805 created a new entry, 3.1.61 (with release data 5/8), while we haven't released 3.1.60. Putting the entry back into 3.1.60.

brandonpayton mentioned this pull request May 20, 2024

Ensure plugin, theme, and WordPress updates succeed WordPress/wordpress-playground#1431

Draft

[nodefs] Don't try to resolve absolute symlinks outside of the VFS #21805

[nodefs] Don't try to resolve absolute symlinks outside of the VFS #21805

Conversation

mho22 commented Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mho22 Apr 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mho22 commented Apr 25, 2024 • edited

sbc100 commented Apr 25, 2024

mho22 commented Apr 25, 2024 • edited

mho22 commented Apr 27, 2024 • edited

mho22 commented Apr 30, 2024 • edited

mho22 commented May 3, 2024

Choose a reason for hiding this comment

mho22 May 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 commented May 3, 2024

mho22 commented May 5, 2024

sbc100 commented May 5, 2024

mho22 commented May 6, 2024 • edited

mho22 commented May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mho22 May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mho22 May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mho22 May 6, 2024 • edited

Choose a reason for hiding this comment

mho22 commented May 6, 2024 • edited

mho22 commented May 7, 2024 • edited

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 left a comment

Choose a reason for hiding this comment

mho22 commented May 7, 2024 • edited

sbc100 commented May 8, 2024

sbc100 commented May 8, 2024

mho22 commented May 8, 2024 • edited

Choose a reason for hiding this comment

mho22 May 8, 2024 • edited

Choose a reason for hiding this comment

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mho22 commented May 8, 2024 • edited

mho22 commented Apr 23, 2024 •

edited

mho22 Apr 24, 2024 •

edited

mho22 commented Apr 25, 2024 •

edited

mho22 commented Apr 25, 2024 •

edited

mho22 commented Apr 27, 2024 •

edited

mho22 commented Apr 30, 2024 •

edited

mho22 May 3, 2024 •

edited

mho22 commented May 6, 2024 •

edited

mho22 commented May 6, 2024 •

edited

mho22 May 6, 2024 •

edited

mho22 May 6, 2024 •

edited

mho22 May 6, 2024 •

edited

mho22 commented May 6, 2024 •

edited

mho22 commented May 7, 2024 •

edited

mho22 commented May 7, 2024 •

edited

mho22 commented May 8, 2024 •

edited

mho22 May 8, 2024 •

edited

mho22 commented May 8, 2024 •

edited