surf: fix path serialization for tree and blob

[FintanH]

The http-api code expects the 'path' serialized field to be the full path for trees, tree entries, and blobs, i.e. the path where it is located concatenated with the name of the entry.

[xphoniex]

There are two issues:

1- doesn't work with subdirs, e.g. http://0.0.0.0:8080/api/v1/projects/rad:z34v8Axp4bEoa5iU9zZHgzRyDLg9H/blob/b4eb4ae3a0fcaa4ff952ca54ac5570abfb859eb0/dir1

error on server side:

2022-12-14T12:38:37.506825Z ERROR request{method=GET uri=/v1/projects/rad:z34v8Axp4bEoa5iU9zZHgzRyDLg9H/blob/b4eb4ae3a0fcaa4ff952ca54ac5570abfb859eb0/dir1}: radicle_httpd::api::error: Error: SurfObject(PathNotFound("dir1"))

I tried other variations such as ./dir1 and /dir1, neither works.

2- iirc, it used to show name of files, one directory deep. now it's only showing directory.

[FintanH]

1- doesn't work with subdirs, e.g. http://0.0.0.0:8080/api/v1/projects/rad:z34v8Axp4bEoa5iU9zZHgzRyDLg9H/blob/b4eb4ae3a0fcaa4ff952ca54ac5570abfb859eb0/dir1

This URL looks strange to me. Why is it blob/<sha>/dir1? Surely it should be tree?

2- iirc, it used to show name of files, one directory deep. now it's only showing directory.

Can you elaborate with an example?

[xphoniex]

This URL looks strange to me. Why is it blob/<sha>/dir1? Surely it should be tree?

Right, had to change ObjectType on interface as well, and use kind. That's why it was retrieving Blob.

[xphoniex]

Ok, tree is fixed. Where is the diff stats for commits?

http://0.0.0.0:8080/api/v1/projects/rad:z34v8Axp4bEoa5iU9zZHgzRyDLg9H/commits/b4eb4ae3a0fcaa4ff952ca54ac5570abfb859eb0

{
  "id": "b4eb4ae3a0fcaa4ff952ca54ac5570abfb859eb0",
  "author": {
    "name": "xphoniex",
    "email": "xphoniex@users.noreply.github.com",
    "time": 1671020342
  },
  "committer": {
    "name": "xphoniex",
    "email": "xphoniex@users.noreply.github.com",
    "time": 1671020342
  },
  "summary": "Initial",
  "message": "Initial\n",
  "description": "",
  "parents": []
}

[FintanH]

Ok, tree is fixed. Where is the diff stats for commits?

http://0.0.0.0:8080/api/v1/projects/rad:z34v8Axp4bEoa5iU9zZHgzRyDLg9H/commits/b4eb4ae3a0fcaa4ff952ca54ac5570abfb859eb0

{
  "id": "b4eb4ae3a0fcaa4ff952ca54ac5570abfb859eb0",
  "author": {
    "name": "xphoniex",
    "email": "xphoniex@users.noreply.github.com",
    "time": 1671020342
  },
  "committer": {
    "name": "xphoniex",
    "email": "xphoniex@users.noreply.github.com",
    "time": 1671020342
  },
  "summary": "Initial",
  "message": "Initial\n",
  "description": "",
  "parents": []
}

Are you using source::Commit? It seems like you may be using git::Commit instead.

Can you also check for any other potential errors upfront to make it quicker for me to get through them? :)

[xphoniex]

everything seems to be working, why did you change this:

{
  "header": {
    "sha1": "45d438ec1a249fc1a71ab8917a74463261b391fa",
    "author": {
      "name": "xphoniex",
      "email": "xphoniex@users.noreply.github.com"
    },
    "summary": "Second",
    "description": "",
    "committer": {
      "name": "xphoniex",
      "email": "xphoniex@users.noreply.github.com"
    },
    "committerTime": 1671022106
  },
  "diff": {
    "added": [
      {
        "path": "README",
        "diff": {
          "type": "plain",
          "hunks": [
            {
              "header": "@@ -0,0 +1 @@\n",
              "lines": [
                {
                  "line": "hello\n",
                  "lineNo": 1
                }
              ]
            }
          ]
        }
      },
      {
        "path": "dir_1/hello",
        "diff": {
          "type": "plain",
          "hunks": [
            {
              "header": "@@ -0,0 +1 @@\n",
              "lines": [
                {
                  "line": "world\n",
                  "lineNo": 1
                }
              ]
            }
          ]
        }
      }
    ],
    "deleted": [],
    "moved": [],
    "copied": [],
    "modified": [],
    "stats": {
      "filesChanged": 2,
      "insertions": 2,
      "deletions": 0
    }
  },
  "branches": [
    "refs/heads/master"
  ]
}

added is new, previously (in interface) it was created.

[FintanH]

Tbh I wasn’t a fan of the name so I reworked it a bit. Addition better mirrors the terminology used in git2

[xphoniex]

Ok. Seems like we also had a line.type in hunks which is now missing, and leads to this:

[FintanH]

The type is still there -- there was no type on Hunk. If you want to see the change set for the modifications I made to diff you can look here https://github.com/radicle-dev/radicle-git/commit/c316ce8d706e34e44d080e062792d2c8a29a6941

[xphoniex]

The type is still there -- there was no type on Hunk. If you want to see the change set for the modifications I made to diff you can look here c316ce8

I don't see a type here (needed on interface here)

[FintanH]

The type is still there -- there was no type on Hunk. If you want to see the change set for the modifications I made to diff you can look here c316ce8

I don't see a type here (needed on interface here)

Aye, that's because the type gets encoded by serde in the JSON output, as you can see here https://github.com/radicle-dev/radicle-git/commit/c316ce8d706e34e44d080e062792d2c8a29a6941#diff-86e0a918145f1ec86402eab0f6e086ac52cf4da3bfa39496ce210aaa99c7b1f0R204. So I'm not convinced that's the problem.

[FintanH]

The type field comes from this macro https://github.com/radicle-dev/radicle-git/commit/c316ce8d706e34e44d080e062792d2c8a29a6941#diff-bb08abcf297a2599496ce089c6e41e8d9c68196c334a1595b640c865fa863308R242

[FintanH]

I think I might have an idea of what it is. Bear with me

[FintanH]

Ya, so this is when the hunks are in "added" or "deleted". There's no need for a "type" in those cases because all the lines are added or deleted.

[xphoniex]

Ya, so this is when the hunks are in "added" or "deleted". There's no need for a "type" in those cases because all the lines are added or deleted.

So, if I add and remove two lines which were next to each other, I'll get two hunks diffs?

Also types are added, deleted, and plain?

[FintanH]

Ya, so this is when the hunks are in "added" or "deleted". There's no need for a "type" in those cases because all the lines are added or deleted.

So, if I add and remove two lines which were next to each other, I'll get two ~hunks~ diffs?

That would be a modification of a file. The "added" and "deleted" fields are for adding and deleting a file. So all lines will be an "addition" or "deletion" in those files. It's implied from them being added or deleted.

Also types are added, deleted, and plain?

No they're either "addition", "deletion", or "context" which correspond to the enum variants of "Modification". The FileDiff's type can be "binary" or "plain", which corresponds to its enum variants.

[xphoniex]

I'm not sure I understand where the type is supposed to be, can you take a look at this. It's a JSON response from API, and has no "addition", "deletion", or "context".

Or are you saying they only show up in modification type?

[FintanH]

I'm not sure I understand where the type is supposed to be, can you take a look at this. It's a JSON response from API, and has no "addition", "deletion", or "context".

Or are you saying they only show up in modification type?

Aye, because it has no hunks under the "modified" field. The only changes are under "added", which implies all the lines in those hunks were added because, y'know, the file was added.

[xphoniex]

This sounds like something that should be populated on the server side. It'd make things unnecessarily complicated on front-end cause now I have to pass line and diff just in-case it's an addition or deletion.

You can see how it works right now in interface, notice we're only passing line as argument.

Fix this please.

[FintanH]

This sounds like something that should be populated on the server side. It'd make things unnecessarily complicated on front-end cause now I have to pass line and diff just in-case it's an addition or deletion.

You can see how it works right now in interface, notice we're only passing line as argument.

Fix this please.

I don't understand why you can't change the interface code to better reflect the new structure as well. The code you linked was just mimicking what the Rust code was doing and so I feel like it can also be changed to mimic the new Rust code.

[xphoniex]

I don't understand why you can't change the interface code to better reflect the new structure as well. The code you linked was just mimicking what the Rust code was doing and so I feel like it can also be changed to mimic the new Rust code.

I had a similar discussion with @cloudhead re something similar (comments) and we concluded that we should let API be authority for such things and keep modifications on client side to bare minimum.

I prepared a fix, the way you suggested, regardless of which side we decide should determine the type.

[FintanH]

I don't understand why you can't change the interface code to better reflect the new structure as well. The code you linked was just mimicking what the Rust code was doing and so I feel like it can also be changed to mimic the new Rust code.

I had a similar discussion with @cloudhead re something similar (comments) and we concluded that we should let API be authority for such things and keep modifications on client side to bare minimum.

I prepared a fix, the way you suggested, regardless of which side we decide should determine the type.

I can agree for moving forward. I made it clear that there were going to be a lot of changes in radicle-surf until we released a stable version again. The PRs are also open for discussion.

[xphoniex]

lgtm, let's merge!

[cloudhead]

Yeah, so the reason I think the JSON output is not ideal (but it is acceptable), is that the type tag is mulitple levels up from the part it influences.

Given some tag called "type" that determines the type of some field "data" to be either of type x or y, usually you'll have {"type": "foo", "data": x | y ...}, but here we have effectively {"type": "foo", "stuff": { { { { { "data": x | y }}}}}.

It's ok though, we can work with it, perhaps worth revisiting when we specify the api fully in the future.

[FintanH]

Yeah, so the reason I think the JSON output is not ideal (but it is acceptable), is that the type tag is mulitple levels up from the part it influences.

Given some tag called "type" that determines the type of some field "data" to be either of type x or y, usually you'll have {"type": "foo", "data": x | y ...}, but here we have effectively {"type": "foo", "stuff": { { { { { "data": x | y }}}}}.

It's ok though, we can work with it, perhaps worth revisiting when we specify the api fully in the future.

Do we? Which "type" field are you referring to here? Because I'm not sure I follow.

The test output here might be the best place for showing me what you mean :) ~https://github.com/radicle-dev/radicle-git/blob/main/radicle-surf/t/src/git/diff.rs#L187-L222~

Sorry, this one is more clear: https://github.com/radicle-dev/radicle-git/blob/959f0b9a89d06e17a075a8a913df7494cd783196/radicle-surf/t/src/git/diff.rs#L193-L236

[cloudhead]

Do we? Which "type" field are you referring to here? Because I'm not sure I follow.

"added" vs. "modified" yields different types for what's inside "lines"

[FintanH]

Do we? Which "type" field are you referring to here? Because I'm not sure I follow.

"added" vs. "modified" yields different types for what's inside "lines"

But is that really unusual? They're distinctly different but happen to share some similar fields :) If I'm entirely off base, then I don't mind changing it back. I found it odd that an added file could have a type other than addition at all, the same for a deleted file.

[cloudhead]

But is that really unusual? They're distinctly different but happen to share some similar fields :) If I'm entirely off base, then I don't mind changing it back. I found it odd that an added file could have a type other than addition at all, the same for a deleted file.

What's unusual is really just that the field that determines the type of data is not right next to the data. So when interpreting the JSON, it's not immediately clear what determines the content of line, because line doesn't have its own type field, yet it appears to have different types :)

I don't feel super strongly about this, I think it can be solved with documentation of the API as well. If it's really easy to add though it might be worth it, but I think it requires a bunch of custom serde instances if I'm not mistaken.

[FintanH]

What's unusual is really just that the field that determines the type of data is not right next to the data. So when interpreting the JSON, it's not immediately clear what determines the content of line, because line doesn't have its own type field, yet it appears to have different types :)

So what I've been trying to say is that its type is determined by virtue of being in the "added" set of lines :smile: The type is redundant. Another way of thinking about it is that a line in added is a different type from the line in modified. Different data, different types. It just happens to be that modified's line also has a type of line that is an addition :)

I don't feel super strongly about this, I think it can be solved with documentation of the API as well. If it's really easy to add though it might be worth it, but I think it requires a bunch of custom serde instances if I'm not mistaken.

Ok, well I'll merge. If I'm being a nuisance here, what I mean is that I can change back all the lines, for added, deleted, and modified, to combined enum :bow:

[FintanH]

Well... I'll merge once I update to rust-1.66 :upside_down_face:

[xphoniex]

The type is redundant.

Talking about redundancy is premature optimization, but that's not the only issue. We want backend to be the ultimate authority and frontend, just a thin renderer of the json response without much processing.

Letting frontend populate certain fields, essentially creates another area where bugs could show up, and fixes need to be applied on the two front simultaneously, in some cases.

As an example, this is the API response from Github:

You can see how many times the url is repeated, there are 18 https://api.github.com/users/radicle-dev/ and 37 https://api.github.com/repos/radicle-dev/. By adding two base fields, this API can be reduced 2120 chars over 6896 chars, a 30% reduction.

However then you'd no longer be able to do this:

$ curl https://api.github.com/repos/radicle-dev/radicle-git | jq ".forks_url"
"https://api.github.com/repos/radicle-dev/radicle-git/forks"

[FintanH]

Sure, I was aiming to reduce the chances of bugs, since added files shouldn't have any other kind of type. If they had anything else it would have been a bug, so to speak.

Nonetheless, I just pushed a change to add the type field no matter what and confirmed in the serialization test that it works. Should be good to go now.

Thanks for the GitHub insight.