Best to discuss together with @Bastien Montagne (mont29) he proposed the signature of the callbacks. including the unused flags parameter. We should get some consensus.

I have no plans of creating a python api for this. The API should be done in 2 ways. One the add-on is interested in where files are stored (BAT) or want to modify (asset browser). In the other hands an add-on would also want to provide their own external files (fluid caches in flip fluids for example).
I do think it can be good for addons but would lead me further away towards the target we want to achieve. In external files for add-ons any operation on the path should be abstracted away what is a lot of work and not really my focus point. Perhaps when discussing Asset Libraries API we could decide otherwise.

I agree that test cases is very useful for this part as it is well hidden to actual users and the stability of the API depends on the internal design of the specific ID types. I doubt if it is easy to add as it requires a lot of data to work on. For example the BLI_path_frame_range_calc has many edge cases so we should design a few data sets where we can test multiple cases.

Test cases for some ID types can be added. But then we should discuss timing. In the meeting last week it was discussed that this was the an important thing to add before moving to bcon3. We are now a week further and we see that the reviewing of the mechanisms that are required still takes a lot of time.

Personally I don't mind this to move to blender 3.1 as the mechanisms aren't at all tested yet. But that should be agreed by the project and project owners.

Some clarifications @Jeroen Bakker (jbakker) and I just worked on:

I'll move the discussion thread from the inline comments to the main comments.

@Bastien Montagne (mont29) said:

We currently have two cases for the given external_file_path pointer:

• It is a fixed array of chars:
  • callback needs to know its size to be able to safely modify it, and is not expected to modify that pointer of pointer.
  • callback should only consider external_file_path_size parameter, and fully ignore is_mem_alloc one.
• It is an allocated buffer:
  • callback does not care about its size, if it needs to modify it it should re-allocate it and update accordingly the pointer it points to.
  • callback should only consider is_mem_alloc parameter, and fully ignore external_file_path_size one.

Aha, so there are certain freedoms that the callback has in certain situations, and not in others. This is not always the case when passing dynamically allocated memory, so I would propose to model the API so that it communicates what can be done with the pointer. The way the memory is allocated doesn't matter. Whether or not the callback is allowed to reallocate the memory and update the pointer, that is what (IMO) is important.

I also don't quite agree with the strict "modification of dynamically allocated memory requires reallocation"; if it can be done by just overwriting the existing data, why bother reallocating?

Could we avoid the is_mem_alloc parameter? Yes. Should we do it? Imho, no, since this would:

• Add some extra 'implied' meaning to external_file_path_size, mixing two different info into the same parameter.
• Reduce overall clarity, in a case where things are already not really simple due to the need to handle two very different cases in a same, abstract way.

This is what's currently happening here in the patch; I agree that we should either use one or the other.

• Less important, but reduce general future-proofness, since we may actually want to pass in the size of an allocated string at some point, if that becomes needed/relevant.

I'd do this right now, instead of "lying" that a zero-size path was passed. Parameters should have one meaning, and any exceptions make it harder to work with.

My proposal:

• Always pass the path size to external_file_path_size.
• Rename is_mem_alloc to something like may_reallocate or may_move_pointer. This expresses what the callback can do with the information it gets, rather than communicating implementation details from the caller.

In D12818#361404, @Sybren A. Stüvel (sybren) wrote:

We currently have two cases for the given external_file_path pointer:

• It is a fixed array of chars:
  • callback needs to know its size to be able to safely modify it, and is not expected to modify that pointer of pointer.
  • callback should only consider external_file_path_size parameter, and fully ignore is_mem_alloc one.
• It is an allocated buffer:
  • callback does not care about its size, if it needs to modify it it should re-allocate it and update accordingly the pointer it points to.
  • callback should only consider is_mem_alloc parameter, and fully ignore external_file_path_size one.

Aha, so there are certain freedoms that the callback has in certain situations, and not in others. This is not always the case when passing dynamically allocated memory, so I would propose to model the API so that it communicates what can be done with the pointer. The way the memory is allocated doesn't matter. Whether or not the callback is allowed to reallocate the memory and update the pointer, that is what (IMO) is important.

I also don't quite agree with the strict "modification of dynamically allocated memory requires reallocation"; if it can be done by just overwriting the existing data, why bother reallocating?

I fully disagree with this analysis. There are no "certain freedoms that the callback has in certain situations". There are two fully different ways of handling storage of those paths, and the callbacks have to deal with both in they want to modify those paths.

Not saying this is an ideal case, but this is what we have to work with.

Could we avoid the is_mem_alloc parameter? Yes. Should we do it? Imho, no, since this would:

• Add some extra 'implied' meaning to external_file_path_size, mixing two different info into the same parameter.
• Reduce overall clarity, in a case where things are already not really simple due to the need to handle two very different cases in a same, abstract way.

This is what's currently happening here in the patch; I agree that we should either use one or the other.

No this is not. Current patch clearly convey which of the two cases we are dealing with. Passing 0to external_file_path_size in case of allocated buffer is simply a way to say 'ignore me', or 'unknown size' (we could add a #define UNKNOWN_BUFFER_SIZE 0 here if you want to be fully explicit about it).

• Less important, but reduce general future-proofness, since we may actually want to pass in the size of an allocated string at some point, if that becomes needed/relevant.

I'd do this right now, instead of "lying" that a zero-size path was passed. Parameters should have one meaning, and any exceptions make it harder to work with.

Once again, external_file_path_size has a single meaning, the size of of passed buffer. 0 just means 'unknown size'. There is no lying here, just unknown info.

My proposal:

• Always pass the path size to external_file_path_size.

This would add extra processing in allocated case, since we do not know that without querying MEM code.

• Rename is_mem_alloc to something like may_reallocate or may_move_pointer. This expresses what the callback can do with the information it gets, rather than communicating implementation details from the caller.

No, when allocated, code is not allowed to reuse the same buffer. Just always reallocate a new string if you need to modify its content, for at least those reasons:

• Current code always re-allocate.
• If a path is allocated, it was most likely done to try and save memory usage. If a path is shortened in the callback, then some memory is lost now.
• It would add additional complexity to the code (both generic looper, and callbacks):
  • Generic looper needs to find the currently allocated size.
  • Modifying callback needs to decide whether to re-allocate or modify in place (callbacks already have to deal with the allocated vs. fixed size buffer case, which is bad enough).
• It avoids inconsistent behaviors that may lead to hard-to-trackdown bugs (if callback sometimes re-uses same allocated buffer with different content, and sometimes re-allocate a new buffer, then code using that path pointer may introduce bugs in cases it would store the value of this pointer locally while using modifying callbacks e.g.).

In D12818#361881, @Bastien Montagne (mont29) wrote:

I fully disagree with this analysis. There are no "certain freedoms that the callback has in certain situations". There are two fully different ways of handling storage of those paths, and the callbacks have to deal with both in they want to modify those paths.

To me, communicating "this memory was dynamically allocated" is exposing what is basically an implementation detail of the caller. There are quite a few things implied here, like "you are allowed to reallocate and change the pointer", "you should not write to this memory", or "it is unknown how much memory is available". These are all different implications, and none of those strictly follow from "this is dynamically allocated". As a concrete example, just because malloc() was used does not always imply any function that gets that pointer is free to reallocate it.

No this is not. Current patch clearly convey which of the two cases we are dealing with. Passing 0to external_file_path_size in case of allocated buffer is simply a way to say 'ignore me', or 'unknown size' (we could add a #define UNKNOWN_BUFFER_SIZE 0 here if you want to be fully explicit about it).

I don't know how you can argue that this is clear in any way, when I get extra explanation and apparently still get it wrong. This is not clear.

The explanation you give here now is different from what is currently in the patch. The patch says "0 is passed if *external_file_path is dynamically allocated", you say "0 means unknown size". One does not automatically follow from the other, so to document one while actually meaning the other is confusing.

My entire point in this conversation is to cut down on implicit implications, to reduce strong coupling between different parts of the code, and to make things simpler. It is to avoid special values that are documented to mean one thing but actually mean another.
I don't understand why these points are dismissed so strongly. I'm not asking for big rewrites or redesigns. Just for some clarity in documentation & naming, and maybe the removal of one boolean parameter.

I do not see any implicit implication in current proposed code, and I do not understand how it is confusing to have two very clear cases, and the need to deal with both of them. To me it really feels like creating problems where there are none. Now if you think you can propose a better interface here, please feel free to do so, actual code might make things clearer than any endless confusing discussions and explanations.

But I really think we both have better ways to invest our time on real, actual issues, than passing by each other like that and being unable to understand each other. If this needs to be sorted out, this needs to be done in person at that point, since we can clearly not communicate our understanding of things through text on this topic.

Forgot to mention T92315: API Design for `IDType::foreach_external_file` in this task. It is a design task that would add a more descriptive API to the foreach_external_file that hides the complexity of how files are stored.