By the way, I read here that semicolon is not needed as PyObject_HEAD is a macro that already contains semicolon. And that the behavior is implementation dependent if the semicolon is added.

Why void*? Couldn't it be PyObject* since this is always going to be called from python code and therefore all objects are PyObject's?

When you do this, you are telling the PythonObj that it now own's this PyObject, and that it should call decrement when it goes out of scope. In this case, it would mean it decrements the store ref count when this function is over, which isn't what we want. I think if you change the _store to be a PythonObj from the beginning, then you never have to cast it to anything, and the ref count will just be handled for you.

PyIter_Next returns a new reference, which means we need to decrement the ref count when we're done with it. So I'd change tuples type to be PythonObj so the ref count is handled for you. You'll then have to convert it to PyObject* to pass to PyArg_ParseTuple below, but that's ok.

When using s or z without # it means it can't contain a null character. It looks like you are using:

s# for filename, filesize
z for node <--- node can contain null characters so this needs to be z#
s for deltabasefilename <--- the deltabasefilename can be a different length to the normal filename, so we need to store it's size as well
z for deltabasenode <--- can also contain null characters, so needs to be z#
z# for delta, delta size

If PyArg_ParseTuple fails, we need to raise the exception. PyArg_ParseTuple will set the python exception flag when it fails, so you just need to throw a pyexception (defined in our pythonutil.h), and then you can have a catch in the top level python-to-c interface function that catches pyexception and returns the appropriate error code to python. Search for pyexception in py-datapackstore.h to see how it's done elsewhere.

DeltaChainLink doesn't take ownership of the strings you pass in right? If these strings are temporarily created for the duration of the iterator, then when tuple goes out of scope and is freed, I think these pointers will become invalid? So we may need to either A) copy the strings when they are passed in to DeltaChainLink(), or B) attach all the returned tuple objects to PyDeltaChain as well, so they have the same lifetime as their associated DeltaChainLink objects. Option B would be more efficient

Same void* issue as above

The existence of this means we'll have python code constructing these. I think we want the PythonDataStore concept to be internal to the C++ implementation, and actual python code should never construct it.

Why isn't the implementation of getdeltachani, getdeltachainraw, and getmessing just put inside this class? Instead of calling out to a global function?

Why copy this?

If this is a python object, this should be of type PythonObj, so all the ref counting is handled for you.

I wanted to separate python specific from non python specific code because of 'Eden related stuff' as you told me

Because key is a reference to const Key. In which case Single has to accept pointer to const Key in cosntructor and also return pointer to const Key. And we cannot simply pass a const pointer to Single class because it extends KeyIterator. So the simples solution was to make a copy.

Yes, it should be PythonObj. However, I tried to separate python specific code from non python specific code and decided that PythonDataStore could be a wrapper which is not python specific (ironically) so that we dont iclude Python.h (bc it is needed only for PythonObj here)

Of course It could and should be but I tried not to include Python.h in PythonDataStore.

Ok

Ok

Yes, you are right. I was not sure what ParseTuple does and how it allocates memory

I think including python.h in PythonDataStore will be ok since PythonDataStore will only ever be included by a top level python-c file.

Since this is specific to the PythonDataStore, which will only ever be linked into Python specific implementations, I think it's ok to include python stuff here.

You could probably use const_cast or static_cast or something to cast it to non-const at compile time, since we know the Key isn't being modified. Alternatively, it might make sense to change KeyIterator to return (const key)*, but that's probably more code churn that necessary right now. I'd just do the cast, since copying Key's requires allocations that we'd rather avoid.

I think since this whole class is specific to python usage of the data store, it's ok for it to #include python.h. That won't prevent us from linking the other code into non-python implementations (like Eden) since they wouldn't be using pythondatastore.h anyway.

To be honest, I am not sure about option B, even though I like it more. As I am not completely sure what PyArg_ParseTuple does, but my guess is that it simply allocates memory for filename, node ... arrays and puts the contents of the tuple in there. Which means that we will need to copy these arrays because on the next iteration of the cycle PyArg_ParseTuple can (potentially) override existing arrays, because there is no memory management for the filename, node.. allocated variables.

Tbh, now I am a little bit confused about how we deal with memory management of PythonDataStores.

I essentially followed the pattern of py_datapackstore, because my previous implementation was ugly(in uniondatapackstore destructor I iterated over the vector of stores and explicitly freed the memory of pythondatastores only). Again, If I push_back pointers to PythonDataStore into stores vector then memory management has to be done by uniondatapackstore. Or if we make stores the vector of shared_ptr then the memory management of cdatapacks will be performed by uniondatapackstores which is not what we want. So I am a little bit confused about this part.

+ ERROR: testGetDeltaChainMultiPackPyStore (__main__.uniondatapackstoretests)
+
+ Traceback (most recent call last):
+ File "/data/users/ms2316/facebook-hg-rpms/fb-hgext/tests/cstore-uniondatapackstore.py", line 220, in testGetDeltaChainMultiPackPyStore
+ chain = unionstore.getdeltachain(revisions2[0][0], revisions2[0][1])
+ File "/data/users/ms2316/facebook-hg-rpms/fb-hgext/remotefilelog/datapack.py", line 319, in getdeltachain
+ raise KeyError((name, hex(node)))
+ KeyError: ('foo', '4fbae6af78196f9ce9f77add42ce8c03b866d3ab')
+ [1]

I dont think we can do that. Because in the first case uniondatapackstore accepts datapackstore(packdir), but when testing datapacks we want it to accepts multiple datapack(packpath) i.e. not packdir

Took me quite a while to figure out that we need to clear python exception, otherwise after entering once again we will exit immediately

I have been stuck with this thing for quite a while now. it->next() is giving me segmentation fault. Is this because variable 'it' has a pointer to KeyIterator but KeyIterator is an abstract class and doesnt have implementation of next? I.e. at runtime it is not known that it is actually a pointer to PythonKeyIterator, right?

for debugging purposes only

I'd expand this to the full 3 lines, with braces. Generally we've avoided single line if's like this because they are error prone during later refactors.

Also, why change away from the normal while loop pattern?

It'd be nice if we could verify that the current exception is a KeyError before we eat it.

If the store doesn't contain the key, we need to return an error code. UnionDeltaChainIterator.getNextChain expects it to return a chain->status() != GET_DELTA_CHAIN_OK if there's an error. Returning an empty chain just causes the union getNextChain to return the empty chain and stop searching other stores.

Was this a debug line?

We should probably rename this function to createPackStore instead of createPack, since it doesn't actually always return a pack.

chain->status() of the empty delta chain returns GET_DELTA_CHAIN_NOT_FOUND.

Yes. It is a debug line.

Eh, that seems pretty subtle. But ok for now I guess.

I'd find this easier to read as a switch block:

switch(is_cdatapack) {
    case 1:
        // Do things
        break;
    case 0:
        // Do things
        break;
    default:
        // Error
       return -1;
}