Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Webdav]: Native Exists() and Stat() functions #7073

Merged
merged 1 commit into from
Jul 7, 2015

Conversation

arnova
Copy link
Member

@arnova arnova commented May 6, 2015

Previously we used plain http methods for Webdav file Exists/Stat, and although it works (most of the time), it is wrong and may not work properly with all clients. This PR implements the proper native functions.

@arnova
Copy link
Member Author

arnova commented May 6, 2015

@mkortstiege : Please comment/review. Note that I needed to change CCurlFile a bit to avoid logspam.

@arnova
Copy link
Member Author

arnova commented May 6, 2015

jenkins build this please

@arnova arnova force-pushed the webdav_propfind branch 2 times, most recently from 584b9c6 to 0366fa7 Compare May 6, 2015 10:36
@arnova
Copy link
Member Author

arnova commented May 6, 2015

jenkins build this please

@arnova
Copy link
Member Author

arnova commented May 6, 2015

jenkins build this please

@arnova
Copy link
Member Author

arnova commented May 6, 2015

jenkins build this please

@kib
Copy link
Member

kib commented May 6, 2015

jenkins build this please because github was down earlier today

@arnova arnova force-pushed the webdav_propfind branch 16 times, most recently from ecf19ff to 7a0346a Compare May 10, 2015 16:26
@arnova
Copy link
Member Author

arnova commented May 13, 2015

ping @mkortstiege . After some more reading it seems we might as well use the HEAD method for exists/stat, also seems a bit more efficient since less data is transferred. I think we do still want the improvements in CCurlFile.

@mkortstiege
Copy link
Member

Looks and sounds good to me. Would like to get another +1 so we can shove this in for next beta to get some more testers on it.

@Paxxi
Copy link
Member

Paxxi commented May 13, 2015

I like the changes but it will be a change in behaviour so I don't think it should go in during Isengaard.

@arnova
Copy link
Member Author

arnova commented May 13, 2015

@Paxxi: The Curl changes are pretty safe and currently we hammer (retry) on 404 which is not nice. I'd propose the keep this PR on hold and in the meantime I'll open a new one which only has the Curl changes.

@Paxxi
Copy link
Member

Paxxi commented May 14, 2015

I agree @arnova you have my +1 for merging those :)

@MartijnKaijser
Copy link
Member

jenkins build and merge

@MartijnKaijser MartijnKaijser added this to the Isengard 16.0-alpha1 milestone Jul 7, 2015
@jenkins4kodi jenkins4kodi merged commit 922e234 into xbmc:master Jul 7, 2015
@arnova
Copy link
Member Author

arnova commented Jul 11, 2015

@MartijnKaijser : You probably misread the discussion in this PR. But this shouldn't have been merged (yet). And apparently it's causing crashes, so I think for now it would be best to revert until we've fixed the crash and we're sure there's an actual benefit from this.

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.

None yet

6 participants