Please create the future diffs with git diff -U100 so that unchanged lines can be seen too.
Better yet would be arcanist. https://wiki.blender.org/wiki/Tools/CodeReview#Use_Arcanist

In D11466#293429, @Ankit Meel (ankitm) wrote:

Please create the future diffs with git diff -U100 so that unchanged lines can be seen too.
Better yet would be arcanist. https://wiki.blender.org/wiki/Tools/CodeReview#Use_Arcanist

Oops, forgot the -U1000. OK, I will check Arcanist out. And process your remarks.

Added some remarks to the code review. Managed to install Arcanist. Will try to use it for my next diff push.

Processed most, if not all, code review remarks. I'm still a bit unsure and unsatified about the listxattr() error handling. It should work, but it seems convoluted.

I also kept the malloc call, because I didn't see a way to replace that with std::string. I didn't print anything to console in case the malloc fails, though. Should I? Also, I noticed Blender has some things like MEM_mallocN(). Should I've used those?

ankitm - What are the values of is_readable and is_writable in the case of a placeholder file ?

@lzandman- I wondered what to do with those. They are false for placeholder files, as no corresponding attribute is found in the collection (because I didn't query for them). The result is the file is marked FILE_ATTR_SYSTEM which does seem kind of weird, but it seems to work out. I think currently the only consequence is that the file is displayed using ICON_SYSTEM

Files in this state should have FILE_ATTR_OFFLINE, but not FILE_ATTR_SYSTEM as they are not "protected" and not FILE_ATTR_READONLY as they are writable (with a delay). Could you just set both is_readable and is_writable to true when fileIsPlaceholder to avoid the setting of FILE_ATTR_SYSTEM?

One does not simple switch to std::string ;-) @Ankit Meel (ankitm)

Searching Google or Github for com.microsoft.OneDrive.RecallOnOpen gives me no matches of this being used in any code. Which makes me wonder why that is, if OneDrive on macOS is rare enough that no one else bothered yet, or if there is some other mechanism used in other apps.

In D11466#294884, @Brecht Van Lommel (brecht) wrote:

Searching Google or Github for com.microsoft.OneDrive.RecallOnOpen gives me no matches of this being used in any code. Which makes me wonder why that is, if OneDrive on macOS is rare enough that no one else bothered yet, or if there is some other mechanism used in other apps.

It indeed seems undocumented. But there's no other official way. It seems Microsoft implemented its own system for these on-demand files. Apple has since introduced an API for iCloud Library files and an extension model that apparently also can be used by third-party developers. But OneDrive doesn't seem to be using this API.

Forgot to mention, I did find an official method to check some of the OneDrive file pinning state. But it involves executing the OneDrive app itself. Doesn't seem like a viable way to run from Blender:
https://docs.microsoft.com/en-us/onedrive/files-on-demand-mac

Please let's not overcomplicate this implementation by trying to use C++ style at all costs, it's too hard to understand. This can simply be:

static const char *ONEDRIVE_RECALLONOPEN_ATTRIBUTE = "com.microsoft.OneDrive.RecallOnOpen";

static bool find_attribute(const std::string &attributes, const char *search_attribute)
{
  /* Attributes is a list of consecutive null-terminated strings. */
  const char *end = attributes.data() + attributes.size();
  for (const char *item = attributes.data(); item < end; item += strlen(item) + 1) {
    if (STREQ(item, search_attribute)) {
      return true;
    }
  } 

  return false;
}

In D11466#294981, @Brecht Van Lommel (brecht) wrote:

Please let's not overcomplicate this implementation by trying to use C++ style at all costs, it's too hard to understand. This can simply be:

Oh boy, now we've almost done a full circle :-) This somewhat resembles my original code. I've updated the diff with your implementation.

When reading the following keep in mind that I don't use a Mac (and so could say dumb things) and my opinion should be treated with less authority than others wading in here (so can be ignored).

But I think you should reconsider some word usages and function names to make this more generic for later work. I realize that this is about OneDrive in particular, but you are really just testing for a type of file state that will also be used by other services.

Whether a user is using OneDrive, iCloud, Google Drive, or a host of other services, there will be a time when files are listed but aren't immediately available without a delay. The files in this state will have FILE_ATTR_OFFLINE to help notify the user of this, then once they are made available (brought online, transferred to a faster medium, unarchived, uncompressed, or downloaded) the file will be like any other file and will not have the FILE_ATTR_OFFLINE attribute.

So first I would consider the use of "is_placeholder" and "checkIfFileIsPlaceholder". Microsoft might use this term in its implementation to refer to the (almost empty) files that are temporarily in place of the target files, but we are less interested in what is there now, and more in the current state of the target file itself. So we aren't checking if "when the file is a placeholder" but if the target file is temporarily in this state. So I would use "is_offline" in place of "is_placeholder" and rename "checkIfFileIsPlaceholder" to "checkIfFileIsOffline". That way it will be pretty obvious on what to do for anyone adding support for other similar services.

In D11466#295068, @Harley Acheson (harley) wrote:

But I think you should reconsider some word usages and function names to make this more generic for later work. I realize that this is about OneDrive in particular, but you are really just testing for a type of file state that will also be used by other services.

I tried to keep it generic. That's why I added two functions, checkIfFileIsPlaceholder() and oneDrive_checkIfFileIsPlaceholder(), whereby the first is the generic one that currently simply calls the seconds one. But in the future could also contain additional logic for other services/scenarios.

So first I would consider the use of "is_placeholder" and "checkIfFileIsPlaceholder". So I would use "is_offline" in place of "is_placeholder" and rename "checkIfFileIsPlaceholder" to "checkIfFileIsOffline". That way it will be pretty obvious on what to do for anyone adding support for other similar services.

Good point! I do think placeholder is a OneDrive-specific term. iCloud Drive calls them "ubiquitous" files. I'll update the code.

Microsoft just announced they will improve the OneDrive experience on macOS later this year. It seems they will replace their custom files on-demand implementation and use the official APIs:

Later this year we will be revamping the OneDrive sync experience on the latest version of macOS based on Apple’s new File Provider platform to improve the Finder experience for OneDrive.

https://techcommunity.microsoft.com/t5/microsoft-onedrive-blog/microsoft-works-to-ensure-a-great-onedrive-experience-on-apple/ba-p/2400803

Good to hear! Should make OneDrive and iCloud Library implementations more similar.

So what about this? I think it's ready for a final review. I've been using it privately for some time now and it seems to work just fine. Would be great to have this in an official build.

@Leon Zandman (lzandman) - So what about this? I think it's ready for a final review. I've been using it privately for some time now and it seems to work just fine. Would be great to have this in an official build.

Don't worry yet that this might be forgotten. Release target would be 3.0 and that will be getting much more development time (will get an extension of a 2-3 months announced fairly soon).