Severity	Location	Code	Message
Error	cstore/py-pythondatastore.h:1	S&RX	CheckCode
Error	cstore/pythondatastore.cpp:1	S&RX	CheckCode
Error	cstore/pythondatastore.h:1	S&RX	CheckCode

cstore/datastore.h
58	The methods will be changed later to cope with other types of DeltaChain (i.e. deltachain from python objects)

cstore/datapackstore.cpp
106–107	Instead of this pattern, you could have getDeltaChainRaw return a shared_ptr<DeltaChain>, then have the freedeltachain call happen in the DeltaChain destructor. That way it's automatically freed whenever it goes out of scope (i.e. here, or when the above DeltaChainIterator is destructed)
194	We should almost never use `new`. Instead use make_shared and return a shared pointer, so the memory ownership is not ambiguous. For instance, if we return a raw pointer like this, it's not clear to the caller if it is their responsibility to destruct the object or not.

cstore/datapackstore.h
40	Why change this to be a shared_ptr? I think the lifetime of this object is controlled by this class, so it's just handing a reference to the caller.
cstore/datastore.h
29	Maybe this can be something like: DeltaChainLink(delta_chain_link_t *ptr) : _filename1(link->filename), _node(link->node), ... So we could get rid of link completely, and we wouldn't need any `if (link) return link->... else return _foo` code?
56	Is isnull the normal name for this for a c++ iterator? From an iterator perspective, isfinished() or isdone() or something would read nicer in the for loops below.
75	Why the two constructors? This second one in particular seems funky. This constructor does a shallow copy, but I assume freedeltachain does a deep delete, which I could imagine could cause double-free's.
cstore/py-datapackstore.h
113	Why shared_ptr this? Key objects are pretty small and I don't think we're going to have derived classes form them, so I think we can pass them around by value.
251	What is the lifetime of this PythonDataStore? The `stores` vector gets passed to the UnionDatapackStore constructor, but I don't think that class assumes ownership. The non-python stores that are passed in have their lifetime maintained by their python wrapper (py_datapackstore), so when UnionDatapackStore is deallocated, it releases it's substore reference, which allows the py_datapackstore python wrappers to be deallocated, which then deallocates the native store. In your case, the store itself will be deallocated that way, but this PythonDataStore will leak I think.
cstore/py-pythondatastore.h
28	Why is this in the .h instead of in the c++ where it's used? Do we expect this to be used by other c++ files? Same for the other functions in this file.

cstore/datapackstore.h
40	Well. The idea behind this is the following: DatapackStoreKeyIterator and PythonDataStoreKeyIterator both inherit from KeyIterator. However, DatapackStoreKeyIterator and PythonDataStoreKeyIterator have different implementations. That is, DatapackStoreKeyIterator simply has a reference to a store and therefore can return only pointers to the keys, because memory management is actually done by DatapackStore. On the other hand PythonDataStoreKeyIterator is simply a wrapper around a vector of Keys and on next() method it removes the next key from the vector, puts that Key on heap(or we can in the first place put the keys on the heap) and returns the shared_ptr to that key so that the key is available while we have pointer to it and is destroyed automatically. Ahhh, after writing this message I actually noticed that PythonKeyIterator (which is already implemented) does approximately the same what I need(does memory management for current key) and therefore I will hopefully copy that design pattern.
cstore/datastore.h
29	Yeah. I will do that definitely. However, why are there two filenames passed in a 5tuple from python getdeltachain method? This is in remotefilelog/datapack.py:: datapack.getdeltachain()
56	Well, actually DeltaChainLink is not an iterator. DeltaChainIterator is the actual iterator, which returns DeltaChainLinks. And if there are not any links left it simply returns a nullLink. Therefore I have that isnull method to check if the link is null, i.e. last. I decided to name it that way but I can of course rename it.
75	Yes, I thought I am doing deep copy, bet since the delta_chain_t contains pointers to objects then probably not. Now this constructor doesn't break anything, because the only use of it is on lines 130 and 151 of datapackstore.cpp. There the freedeltachain is not called, therefore the memory is not freed and the ownership is implicitly passed to this DeltaChain object. But I guess I need to change it because it's not a good practice, right?
cstore/py-datapackstore.h
251	Yes, Agree.
cstore/py-pythondatastore.h
28	This .h file is included by pythondatastore.cpp. Shouldnt it be .h?

cstore/datapackstore.h
40	The only reason's I can think of for returning a shared_ptr are: if we need to return instances of a derived type of Key if we want the receiver to also manage the lifetime of the Key instance In this case, we aren't making derived versions of the Key class, and generally the receiver won't need to own this type, and if it does need to, a Key instance is pretty cheap to copy. So yea, the PythonKeyIterator pattern will be cheaper because you avoid all the allocations and ref counting.
cstore/datastore.h
29	In theory the deltabasenode could be a node from a completely different file. In practice it's always from the same file for now though. Maybe we should rename filename1 and filename2 to filename and deltabasefilename
75	Hmm. Maybe just add comments explaining that this class takes ownership of the chain, regardless of if you pass it by pointed or by value. The interaction between C code and C++ is always a bit funky like this.
cstore/py-pythondatastore.h
28	.h files are usually used to declare function and structure definitions that need to be reused between C++ files. In this case, since they're only used on the pythondatastore.cpp file, you could just declare the functions directly in there (even though they aren't part of the PythonDataStore class).

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	1194		Aug 23 2017, 9:18 AM	★	★
Diff 2	1199	Fixed bugs. Implemented abstraction around delta chains and delta chain links.	Aug 23 2017, 10:38 AM	★	★
Diff 3	1244	Changed native pointers to shared pointers, addressed memory management issues	Aug 24 2017, 9:31 AM	★	★
Diff 4	1261	This is not a working version. Added PythonDataStore to wrap around python…	Aug 24 2017, 5:42 PM	★	★
Diff 5	1262	Removed some redundant code	Aug 24 2017, 5:50 PM	★	★

Commit	Local	Parents	Author	Summary	Date
64700d25b31e	3953	765372cd7cf0	Mihails Smolins	cstore: gerenic uniondatapackstore (Show More…)	Aug 24 2017, 5:49 PM

Diff 1262

cstore/datapackstore.h

	// datapackstore.h - c++ declarations for a data pack store			// datapackstore.h - c++ declarations for a data pack store
	//			//
	// Copyright 2017 Facebook, Inc.			// Copyright 2017 Facebook, Inc.
	//			//
	// This software may be used and distributed according to the terms of the			// This software may be used and distributed according to the terms of the
	// GNU General Public License version 2 or any later version.			// GNU General Public License version 2 or any later version.
	//			//
	// no-check-code			// no-check-code

	#ifndef DATAPACKSTORE_H			#ifndef DATAPACKSTORE_H
	#define DATAPACKSTORE_H			#define DATAPACKSTORE_H

	extern "C" {			extern "C" {
	#include "cdatapack.h"			#include "cdatapack.h"
	}			}

	#include <string>
	#include <ctime>			#include <ctime>
				#include <memory>
				#include <string>
	#include <unordered_set>			#include <unordered_set>
	#include <vector>			#include <vector>

				#include "datastore.h"
	#include "key.h"			#include "key.h"
	#include "portability/portability.h"			#include "portability/portability.h"

	const clock_t PACK_REFRESH_RATE = 0.1 * CLOCKS_PER_SEC;			const clock_t PACK_REFRESH_RATE = 0.1 * CLOCKS_PER_SEC;

	class DeltaChainIterator {
	private:
	size_t _index;
	protected:
	std::vector<delta_chain_t> _chains;
	DeltaChainIterator() :
	_index(0) {}
	virtual delta_chain_t getNextChain(const Key &key) {
	return COMPOUND_LITERAL(delta_chain_t) { GET_DELTA_CHAIN_NOT_FOUND };
	}
	public:
	DeltaChainIterator(delta_chain_t chain) :
	_index(0) {
	_chains.push_back(chain);
	}

	virtual ~DeltaChainIterator() {
	for(std::vector<delta_chain_t>::iterator it = _chains.begin();
	it != _chains.end();
	it++) {
	freedeltachain(*it);
	}
	}

	delta_chain_link_t *next() {
	delta_chain_t *chain = &_chains.back();
	if (_index >= chain->links_count) {
	// If we're not at the end, and we have a callback to fetch more, do the
	// fetch.
	bool refreshed = false;
	if (chain->links_count > 0) {
	delta_chain_link_t *result = &chain->delta_chain_links[_index - 1];

	const uint8_t *deltabasenode = result->deltabase_node;
	if (memcmp(deltabasenode, NULLID, BIN_NODE_SIZE) != 0) {
	Key key(result->filename, result->filename_sz,
	(const char*)deltabasenode, BIN_NODE_SIZE);

	delta_chain_t newChain = this->getNextChain(key);
	if (newChain.code == GET_DELTA_CHAIN_OK) {
	// Do not free the old chain, since the iterator consumer may
	// still be holding references to it.
	_chains.push_back(newChain);
	chain = &_chains.back();
	_index = 0;
	refreshed = true;
	} else {
	freedeltachain(newChain);
	}
	}
	}

	if (!refreshed) {
	return NULL;
	}
	}

	delta_chain_link_t *result = &chain->delta_chain_links[_index];
	_index++;

	return result;
	}
	};

	class DatapackStore;			class DatapackStore;
	class DatapackStoreKeyIterator : public KeyIterator {			class DatapackStoreKeyIterator : public KeyIterator {
	private:			private:
	DatapackStore &_store;			DatapackStore &_store;
	KeyIterator &_missing;			KeyIterator &_missing;

	public:			public:
	DatapackStoreKeyIterator(DatapackStore &store, KeyIterator &missing) :			DatapackStoreKeyIterator(DatapackStore &store, KeyIterator &missing) :
	_store(store),			_store(store),
	_missing(missing) {}			_missing(missing) {}

	Key *next();			std::shared_ptr<Key> next();
				durhamUnsubmitted Not Done Why change this to be a shared_ptr? I think the lifetime of this object is controlled by this class, so it's just handing a reference to the caller. durham: Why change this to be a shared_ptr? I think the lifetime of this object is controlled by this…
				ms2316AuthorUnsubmitted Not Done Well. The idea behind this is the following: DatapackStoreKeyIterator and PythonDataStoreKeyIterator both inherit from KeyIterator. However, DatapackStoreKeyIterator and PythonDataStoreKeyIterator have different implementations. That is, DatapackStoreKeyIterator simply has a reference to a store and therefore can return only pointers to the keys, because memory management is actually done by DatapackStore. On the other hand PythonDataStoreKeyIterator is simply a wrapper around a vector of Keys and on next() method it removes the next key from the vector, puts that Key on heap(or we can in the first place put the keys on the heap) and returns the shared_ptr to that key so that the key is available while we have pointer to it and is destroyed automatically. Ahhh, after writing this message I actually noticed that PythonKeyIterator (which is already implemented) does approximately the same what I need(does memory management for current key) and therefore I will hopefully copy that design pattern. ms2316: Well. The idea behind this is the following: DatapackStoreKeyIterator and…
				durhamUnsubmitted Not Done The only reason's I can think of for returning a shared_ptr are: if we need to return instances of a derived type of Key if we want the receiver to also manage the lifetime of the Key instance In this case, we aren't making derived versions of the Key class, and generally the receiver won't need to own this type, and if it does need to, a Key instance is pretty cheap to copy. So yea, the PythonKeyIterator pattern will be cheaper because you avoid all the allocations and ref counting. durham: The only reason's I can think of for returning a shared_ptr are: - if we need to return…
	};			};

	/* Manages access to a directory of datapack files. */			/* Manages access to a directory of datapack files. */
	class DatapackStore {			class DatapackStore : public DataStore {
	private:			private:
	std::string _path;			std::string _path;
	clock_t _lastRefresh;			clock_t _lastRefresh;

	std::unordered_set<std::string> _packPaths;			std::unordered_set<std::string> _packPaths;

	datapack_handle_t *addPack(const std::string &path);			datapack_handle_t *addPack(const std::string &path);

	std::vector<datapack_handle_t*> refresh();			std::vector<datapack_handle_t*> refresh();
	public:			public:
	std::vector<datapack_handle_t*> _packs;			std::vector<datapack_handle_t*> _packs;

	DatapackStore(const std::string &path);			DatapackStore(const std::string &path);

	~DatapackStore();			~DatapackStore();

	DeltaChainIterator getDeltaChain(const Key &key);			DeltaChainIterator getDeltaChain(const Key &key);

	DatapackStoreKeyIterator getMissing(KeyIterator &missing);			std::shared_ptr<KeyIterator> getMissing(KeyIterator &missing);

	delta_chain_t getDeltaChainRaw(const Key &key);			std::shared_ptr<DeltaChain> getDeltaChainRaw(const Key &key);

	bool contains(const Key &key);			bool contains(const Key &key);

	void markForRefresh();			void markForRefresh();
	};			};

	#endif //DATAPACKSTORE_H			#endif //DATAPACKSTORE_H

cstore/datapackstore.cpp

	for(std::vector<datapack_handle_t*>::iterator it = _packs.begin();			for(std::vector<datapack_handle_t*>::iterator it = _packs.begin();
	it != _packs.end();			it != _packs.end();
	it++) {			it++) {
	close_datapack(*it);			close_datapack(*it);
	}			}
	}			}

	DeltaChainIterator DatapackStore::getDeltaChain(const Key &key) {			DeltaChainIterator DatapackStore::getDeltaChain(const Key &key) {
	delta_chain_t chain = this->getDeltaChainRaw(key);			std::shared_ptr<DeltaChain> chain = this->getDeltaChainRaw(key);
	if (chain.code == GET_DELTA_CHAIN_OK) {			if (chain->code() == GET_DELTA_CHAIN_OK) {
	return DeltaChainIterator(chain);			return DeltaChainIterator(chain);
	}			}

	freedeltachain(chain);

	throw MissingKeyError("unable to find delta chain");			throw MissingKeyError("unable to find delta chain");
				durhamUnsubmitted Done Instead of this pattern, you could have getDeltaChainRaw return a shared_ptr<DeltaChain>, then have the freedeltachain call happen in the DeltaChain destructor. That way it's automatically freed whenever it goes out of scope (i.e. here, or when the above DeltaChainIterator is destructed) durham: Instead of this pattern, you could have getDeltaChainRaw return a shared_ptr<DeltaChain>, then…
	}			}

	delta_chain_t DatapackStore::getDeltaChainRaw(const Key &key) {			std::shared_ptr<DeltaChain> DatapackStore::getDeltaChainRaw(const Key &key) {
	for(std::vector<datapack_handle_t*>::iterator it = _packs.begin();			for(std::vector<datapack_handle_t*>::iterator it = _packs.begin();
	it != _packs.end();			it != _packs.end();
	it++) {			it++) {
	datapack_handle_t pack = it;			datapack_handle_t pack = it;

	delta_chain_t chain = getdeltachain(pack, (const uint8_t *) key.node);			delta_chain_t chain = getdeltachain(pack, (const uint8_t *) key.node);
	if (chain.code == GET_DELTA_CHAIN_OOM) {			if (chain.code == GET_DELTA_CHAIN_OOM) {
	throw std::runtime_error("out of memory");			throw std::runtime_error("out of memory");
	} else if (chain.code == GET_DELTA_CHAIN_NOT_FOUND) {			} else if (chain.code == GET_DELTA_CHAIN_NOT_FOUND) {
	freedeltachain(chain);			freedeltachain(chain);
	continue;			continue;
	} else if (chain.code != GET_DELTA_CHAIN_OK) {			} else if (chain.code != GET_DELTA_CHAIN_OK) {
	freedeltachain(chain);			freedeltachain(chain);
	continue;			continue;
	}			}

	return chain;			return std::make_shared<DeltaChain>(chain);
	}			}

	// Check if there are new packs available			// Check if there are new packs available
	std::vector<datapack_handle_t*> refreshed = refresh();			std::vector<datapack_handle_t*> refreshed = refresh();
	for(std::vector<datapack_handle_t*>::iterator it = refreshed.begin();			for(std::vector<datapack_handle_t*>::iterator it = refreshed.begin();
	it != refreshed.end();			it != refreshed.end();
	it++) {			it++) {
	datapack_handle_t pack = it;			datapack_handle_t pack = it;

	delta_chain_t chain = getdeltachain(pack, (const uint8_t *) key.node);			delta_chain_t chain = getdeltachain(pack, (const uint8_t *) key.node);
	if (chain.code == GET_DELTA_CHAIN_OOM) {			if (chain.code == GET_DELTA_CHAIN_OOM) {
	throw std::runtime_error("out of memory");			throw std::runtime_error("out of memory");
	} else if (chain.code == GET_DELTA_CHAIN_NOT_FOUND) {			} else if (chain.code == GET_DELTA_CHAIN_NOT_FOUND) {
	freedeltachain(chain);			freedeltachain(chain);
	continue;			continue;
	} else if (chain.code != GET_DELTA_CHAIN_OK) {			} else if (chain.code != GET_DELTA_CHAIN_OK) {
	freedeltachain(chain);			freedeltachain(chain);
	continue;			continue;
	}			}

	return chain;			return std::make_shared<DeltaChain>(chain);
	}			}

	return COMPOUND_LITERAL(delta_chain_t) { GET_DELTA_CHAIN_NOT_FOUND };			return std::make_shared<DeltaChain>(GET_DELTA_CHAIN_NOT_FOUND);
	}			}

	Key *DatapackStoreKeyIterator::next() {			std::shared_ptr<Key> DatapackStoreKeyIterator::next() {
	Key *key;			std::shared_ptr<Key> key;
	while ((key = _missing.next()) != NULL) {			while ((key = _missing.next()) != NULL) {
	if (!_store.contains(*key)) {			if (!_store.contains(*key)) {
	return key;			return key;
	}			}
	}			}

	return NULL;			return NULL;
	}			}
	if (find(pack, (uint8_t*)key.node, &packindex)) {			if (find(pack, (uint8_t*)key.node, &packindex)) {
	return true;			return true;
	}			}
	}			}

	return false;			return false;
	}			}

	DatapackStoreKeyIterator DatapackStore::getMissing(KeyIterator &missing) {			std::shared_ptr<KeyIterator> DatapackStore::getMissing(KeyIterator &missing) {
	return DatapackStoreKeyIterator(*this, missing);			return std::make_shared<DatapackStoreKeyIterator>(*this, missing);
				durhamUnsubmitted Done We should almost never use `new`. Instead use make_shared and return a shared pointer, so the memory ownership is not ambiguous. For instance, if we return a raw pointer like this, it's not clear to the caller if it is their responsibility to destruct the object or not. durham: We should almost never use `new`. Instead use make_shared and return a shared pointer, so the…
	}			}

	std::vector<datapack_handle_t*> DatapackStore::refresh() {			std::vector<datapack_handle_t*> DatapackStore::refresh() {
	clock_t now = clock();			clock_t now = clock();

	std::vector<datapack_handle_t*> newPacks;			std::vector<datapack_handle_t*> newPacks;
	if (now - _lastRefresh > PACK_REFRESH_RATE) {			if (now - _lastRefresh > PACK_REFRESH_RATE) {
	std::vector<std::string> availablePacks = getAvailablePackFiles(_path);			std::vector<std::string> availablePacks = getAvailablePackFiles(_path);

cstore/datastore.h

This file was added.

				// datastore.h - c++ declarations for a data store
				//
				// copyright 2017 facebook, inc.
				//
				// this software may be used and distributed according to the terms of the
				// gnu general public license version 2 or any later version.
				//
				// no-check-code

				#ifndef DATASTORE_H
				#define DATASTORE_H

				extern "C" {
				#include "cdatapack.h"
				}

				#include <memory>
				#include <vector>

				#include "key.h"
				#include "portability/portability.h"

				class DeltaChainLink {
				public:
				const char _filename1, _filename2;
				char _node, _delta, *_deltabasenode;
				delta_chain_link_t *link;

				DeltaChainLink(delta_chain_link_t *ptr) : link(ptr) {}
				durhamUnsubmitted Not Done Maybe this can be something like: DeltaChainLink(delta_chain_link_t ptr) : _filename1(link->filename), _node(link->node), ... So we could get rid of link completely, and we wouldn't need any `if (link) return link->... else return _foo` code? durham:* Maybe this can be something like: ``` DeltaChainLink(delta_chain_link_t *ptr) : _filename1…
				ms2316AuthorUnsubmitted Not Done Yeah. I will do that definitely. However, why are there two filenames passed in a 5tuple from python getdeltachain method? This is in remotefilelog/datapack.py:: datapack.getdeltachain() ms2316: Yeah. I will do that definitely. However, why are there two filenames passed in a 5tuple from…
				durhamUnsubmitted Not Done In theory the deltabasenode could be a node from a completely different file. In practice it's always from the same file for now though. Maybe we should rename filename1 and filename2 to filename and deltabasefilename durham: In theory the deltabasenode could be a node from a completely different file. In practice it's…

				DeltaChainLink(const char filename1, const char filename2,
				char node, char deltabasenode, char *delta) :
				_filename1(filename1), _filename2(filename2), _node(node),
				_delta(delta), _deltabasenode(deltabasenode) {}

				const uint8_t* deltabase_node() {
				return link->deltabase_node;
				}

				const char* filename() {
				return link->filename;
				}

				uint16_t filename_sz() {
				return link->filename_sz;
				}

				const uint8_t* delta() {
				return link->delta;
				}

				data_offset_t delta_sz() {
				return link->delta_sz;
				}

				bool isnull() {
				durhamUnsubmitted Not Done Is isnull the normal name for this for a c++ iterator? From an iterator perspective, isfinished() or isdone() or something would read nicer in the for loops below. durham: Is isnull the normal name for this for a c++ iterator? From an iterator perspective…
				ms2316AuthorUnsubmitted Not Done Well, actually DeltaChainLink is not an iterator. DeltaChainIterator is the actual iterator, which returns DeltaChainLinks. And if there are not any links left it simply returns a nullLink. Therefore I have that isnull method to check if the link is null, i.e. last. I decided to name it that way but I can of course rename it. ms2316: Well, actually DeltaChainLink is not an iterator. DeltaChainIterator is the actual iterator…
				return (link == NULL);
				}
				ms2316AuthorUnsubmitted Not Done The methods will be changed later to cope with other types of DeltaChain (i.e. deltachain from python objects) ms2316: The methods will be changed later to cope with other types of DeltaChain (i.e. deltachain from…

				const uint8_t* node() {
				return link->node;
				}
				};

				class DeltaChain {
				public:
				//C DeltaChain
				delta_chain_t *chain;

				//Python DeltaChain
				std::shared_ptr< std::vector<DeltaChainLink> > _links;

				DeltaChain(delta_chain_t *ptr) : chain(ptr) {}

				DeltaChain(delta_chain_t orig) {
				durhamUnsubmitted Not Done Why the two constructors? This second one in particular seems funky. This constructor does a shallow copy, but I assume freedeltachain does a deep delete, which I could imagine could cause double-free's. durham: Why the two constructors? This second one in particular seems funky. This constructor does a…
				ms2316AuthorUnsubmitted Not Done Yes, I thought I am doing deep copy, bet since the delta_chain_t contains pointers to objects then probably not. Now this constructor doesn't break anything, because the only use of it is on lines 130 and 151 of datapackstore.cpp. There the freedeltachain is not called, therefore the memory is not freed and the ownership is implicitly passed to this DeltaChain object. But I guess I need to change it because it's not a good practice, right? ms2316: Yes, I thought I am doing deep copy, bet since the delta_chain_t contains pointers to objects…
				durhamUnsubmitted Not Done Hmm. Maybe just add comments explaining that this class takes ownership of the chain, regardless of if you pass it by pointed or by value. The interaction between C code and C++ is always a bit funky like this. durham: Hmm. Maybe just add comments explaining that this class takes ownership of the chain…
				chain = (delta_chain_t*)malloc(sizeof orig);
				if (chain != NULL) {
				memcpy(chain, &orig, sizeof orig);
				}
				}

				DeltaChain(get_delta_chain_code_t error) {
				chain = new COMPOUND_LITERAL(delta_chain_t) { GET_DELTA_CHAIN_NOT_FOUND };
				}

				DeltaChain(std::shared_ptr< std::vector<DeltaChainLink> > links) : _links(links) {}

				~DeltaChain() {
				freedeltachain(*chain);
				}

				const DeltaChainLink delta_chain_links(const size_t idx) {
				return DeltaChainLink(&(chain->delta_chain_links[idx]));
				}

				size_t links_count() {
				return chain->links_count;
				}

				get_delta_chain_code_t code() {
				return chain->code;
				}

				};

				class DeltaChainIterator {
				private:
				size_t _index;
				protected:
				std::vector< std::shared_ptr<DeltaChain> > _chains;
				DeltaChainIterator() :
				_index(0) {}
				virtual std::shared_ptr<DeltaChain> getNextChain(const Key &key) {
				return std::make_shared<DeltaChain>(GET_DELTA_CHAIN_NOT_FOUND);
				}
				public:
				DeltaChainIterator(std::shared_ptr<DeltaChain> chain) :
				_index(0) {
				_chains.push_back(chain);
				}

				DeltaChainLink next() {
				std::shared_ptr<DeltaChain> chain = _chains.back();

				if (_index >= chain->links_count()) {
				// If we're not at the end, and we have a callback to fetch more, do the
				// fetch.
				bool refreshed = false;
				if (chain->links_count() > 0) {
				//DeltaChainLink *result = &chain->delta_chain_links(_index - 1);
				DeltaChainLink result = chain->delta_chain_links(_index - 1);

				const uint8_t *deltabasenode = result.deltabase_node();
				if (memcmp(deltabasenode, NULLID, BIN_NODE_SIZE) != 0) {
				Key key(result.filename(), result.filename_sz(),
				(const char*)deltabasenode, BIN_NODE_SIZE);

				std::shared_ptr<DeltaChain> newChain = this->getNextChain(key);
				if (newChain->code() == GET_DELTA_CHAIN_OK) {
				// Do not free the old chain, since the iterator consumer may
				// still be holding references to it.
				_chains.push_back(newChain);
				chain = _chains.back();
				_index = 0;
				refreshed = true;
				}
				//newChain is freed here together with underlying delta_chain_t
				}
				}

				if (!refreshed) {
				return DeltaChainLink(NULL);
				}
				}

				DeltaChainLink result = chain->delta_chain_links(_index);
				_index++;

				return result;
				}
				};

				class DataStore {
				protected:
				DataStore() {}

				public:
				virtual ~DataStore() {}

				virtual DeltaChainIterator getDeltaChain(const Key &key) = 0;

				virtual std::shared_ptr<DeltaChain> getDeltaChainRaw(const Key &key) = 0;

				virtual std::shared_ptr<KeyIterator> getMissing(KeyIterator &missing) = 0;

				virtual bool contains(const Key &key) = 0;

				virtual void markForRefresh() = 0;
				};

				#endif //DATASTORE_H

cstore/key.h

	// key.h - c++ declarations for a key to pack data			// key.h - c++ declarations for a key to pack data
	//			//
	// Copyright 2017 Facebook, Inc.			// Copyright 2017 Facebook, Inc.
	//			//
	// This software may be used and distributed according to the terms of the			// This software may be used and distributed according to the terms of the
	// GNU General Public License version 2 or any later version.			// GNU General Public License version 2 or any later version.
	//			//
	// no-check-code			// no-check-code

	#ifndef KEY_H			#ifndef KEY_H
	#define KEY_H			#define KEY_H

	#include <cstring>			#include <cstring>
	#include <vector>			#include <vector>
	#include <stdexcept>			#include <stdexcept>
				#include <memory>

	#include "convert.h"			#include "convert.h"
	#include "convert.h"			#include "convert.h"

	/* Represents a key into the Mercurial store. Each key is a (name, node) pair,			/* Represents a key into the Mercurial store. Each key is a (name, node) pair,
	* though store implementations can choose to ignore the name in some cases. */			* though store implementations can choose to ignore the name in some cases. */
	struct Key {			struct Key {
	/* The filename portion of the key. */			/* The filename portion of the key. */
	std::string name;			std::string name;

	/* The byte node portion of the key. */			/* The byte node portion of the key. */
	char node[BIN_NODE_SIZE];			char node[BIN_NODE_SIZE];

	Key() :			Key() :
	node() {}			node() {}

				Key(const Key& other) {
				name = other.name;
				memcpy(node, other.node, BIN_NODE_SIZE);
				}

	Key(const char name, size_t namelen, const char node, size_t nodelen) :			Key(const char name, size_t namelen, const char node, size_t nodelen) :
	name(name, namelen) {			name(name, namelen) {
	if (nodelen != BIN_NODE_SIZE) {			if (nodelen != BIN_NODE_SIZE) {
	throw std::logic_error("invalid node length");			throw std::logic_error("invalid node length");
	}			}

	memcpy(this->node, node, BIN_NODE_SIZE);			memcpy(this->node, node, BIN_NODE_SIZE);
	}			}
	};			};

	class MissingKeyError : public std::runtime_error {			class MissingKeyError : public std::runtime_error {
	public:			public:
	MissingKeyError(const char *what_arg) :			MissingKeyError(const char *what_arg) :
	std::runtime_error(what_arg) {			std::runtime_error(what_arg) {
	}			}
	};			};

	class KeyIterator {			class KeyIterator {
	protected:			protected:
	KeyIterator() {}			KeyIterator() {}
	public:			public:
	virtual ~KeyIterator() = default;			virtual ~KeyIterator() = default;
	virtual Key *next() = 0;			virtual std::shared_ptr<Key> next() = 0;
	};			};

	#endif //KEY_H			#endif //KEY_H

cstore/py-datapackstore.h

	#include <memory>			#include <memory>
	#include <string>			#include <string>

	extern "C" {			extern "C" {
	#include "cdatapack.h"			#include "cdatapack.h"
	}			}

	#include "pythonutil.h"			#include "pythonutil.h"
				#include "pythondatastore.h"
				#include "datastore.h"
	#include "datapackstore.h"			#include "datapackstore.h"
	#include "key.h"			#include "key.h"
	#include "py-structs.h"			#include "py-structs.h"
	#include "uniondatapackstore.h"			#include "uniondatapackstore.h"

	// --------- DatapackStore Implementation ---------			// --------- DatapackStore Implementation ---------

	/*			/*
	}			}

	Key key(name, namelen, node, nodelen);			Key key(name, namelen, node, nodelen);

	DeltaChainIterator chain = self->datapackstore.getDeltaChain(key);			DeltaChainIterator chain = self->datapackstore.getDeltaChain(key);

	PythonObj resultChain = PyList_New(0);			PythonObj resultChain = PyList_New(0);

	delta_chain_link_t *link;
	size_t index = 0;			size_t index = 0;
	while ((link = chain.next()) != NULL) {			for (DeltaChainLink link = chain.next(); !link.isnull(); link = chain.next()) {
	PythonObj name = PyString_FromStringAndSize(link->filename, link->filename_sz);			PythonObj name = PyString_FromStringAndSize(link.filename(), link.filename_sz());
	PythonObj retnode = PyString_FromStringAndSize((const char *) link->node, NODE_SZ);			PythonObj retnode = PyString_FromStringAndSize((const char *) link.node(), NODE_SZ);
	PythonObj deltabasenode = PyString_FromStringAndSize(			PythonObj deltabasenode = PyString_FromStringAndSize(
	(const char *) link->deltabase_node, NODE_SZ);			(const char *) link.deltabase_node(), NODE_SZ);
	PythonObj delta = PyString_FromStringAndSize(			PythonObj delta = PyString_FromStringAndSize(
	(const char *) link->delta, (Py_ssize_t) link->delta_sz);			(const char *) link.delta(), (Py_ssize_t) link.delta_sz());

	PythonObj tuple = PyTuple_Pack(5, (PyObject)name, (PyObject)retnode,			PythonObj tuple = PyTuple_Pack(5, (PyObject)name, (PyObject)retnode,
	(PyObject)name, (PyObject)deltabasenode,			(PyObject)name, (PyObject)deltabasenode,
	(PyObject*)delta);			(PyObject*)delta);

	if (PyList_Append((PyObject*)resultChain, tuple.returnval())) {			if (PyList_Append((PyObject*)resultChain, tuple.returnval())) {
	return NULL;			return NULL;
	}			}
	PyErr_SetString(PyExc_RuntimeError, ex.what());			PyErr_SetString(PyExc_RuntimeError, ex.what());
	return NULL;			return NULL;
	}			}
	}			}

	class PythonKeyIterator : public KeyIterator {			class PythonKeyIterator : public KeyIterator {
	private:			private:
	PythonObj _input;			PythonObj _input;
	Key _current;			std::shared_ptr<Key> _current;
				durhamUnsubmitted Not Done Why shared_ptr this? Key objects are pretty small and I don't think we're going to have derived classes form them, so I think we can pass them around by value. durham: Why shared_ptr this? Key objects are pretty small and I don't think we're going to have derived…
	public:			public:
	PythonKeyIterator(PythonObj input) :			PythonKeyIterator(PythonObj input) :
	_input(input) {}			_input(input) {}

	Key *next() {			std::shared_ptr<Key> next() {
	PyObject *item;			PyObject *item;
	while ((item = PyIter_Next((PyObject*)_input)) != NULL) {			while ((item = PyIter_Next((PyObject*)_input)) != NULL) {
	PythonObj itemObj = item;			PythonObj itemObj = item;

	char *name;			char *name;
	Py_ssize_t namelen;			Py_ssize_t namelen;
	char *node;			char *node;
	Py_ssize_t nodelen;			Py_ssize_t nodelen;
	if (!PyArg_ParseTuple(item, "s#s#", &name, &namelen, &node, &nodelen)) {			if (!PyArg_ParseTuple(item, "s#s#", &name, &namelen, &node, &nodelen)) {
	throw pyexception();			throw pyexception();
	}			}

	_current = Key(name, namelen, node, nodelen);			_current = std::make_shared<Key>(name, namelen, node, nodelen);
	return &_current;			return _current;
	}			}

	return NULL;			return NULL;
	}			}
	};			};

	static PyObject datapackstore_getmissing(py_datapackstore self, PyObject *keys) {			static PyObject datapackstore_getmissing(py_datapackstore self, PyObject *keys) {
	try {			try {
	PythonObj result = PyList_New(0);			PythonObj result = PyList_New(0);

	PythonObj inputIterator = PyObject_GetIter(keys);			PythonObj inputIterator = PyObject_GetIter(keys);
	PythonKeyIterator keysIter(inputIterator);			PythonKeyIterator keysIter(inputIterator);

	DatapackStoreKeyIterator missingIter = self->datapackstore.getMissing(keysIter);			std::shared_ptr<KeyIterator> missingIter = self->datapackstore.getMissing(keysIter);

	Key *key;			std::shared_ptr<Key> key;
	while ((key = missingIter.next()) != NULL) {			while ((key = missingIter->next()) != NULL) {
	PythonObj missingKey = Py_BuildValue("(s#s#)", key->name.c_str(), key->name.size(),			PythonObj missingKey = Py_BuildValue("(s#s#)", key->name.c_str(), key->name.size(),
	key->node, 20);			key->node, 20);
	if (PyList_Append(result, (PyObject*)missingKey)) {			if (PyList_Append(result, (PyObject*)missingKey)) {
	return NULL;			return NULL;
	}			}
	}			}

	return result.returnval();			return result.returnval();
	*/			*/
	static int uniondatapackstore_init(py_uniondatapackstore self, PyObject args) {			static int uniondatapackstore_init(py_uniondatapackstore self, PyObject args) {
	PyObject *storeList;			PyObject *storeList;
	if (!PyArg_ParseTuple(args, "O", &storeList)) {			if (!PyArg_ParseTuple(args, "O", &storeList)) {
	return -1;			return -1;
	}			}

	try {			try {
	std::vector<DatapackStore*> stores;			std::vector<DataStore*> stores;
	std::vector<PythonObj> pySubStores;			std::vector<PythonObj> pySubStores;

	PyObject *item;			PyObject *item;
	PythonObj inputIterator = PyObject_GetIter(storeList);			PythonObj inputIterator = PyObject_GetIter(storeList);
	while ((item = PyIter_Next(inputIterator)) != NULL) {			while ((item = PyIter_Next(inputIterator)) != NULL) {
	// Record the substore references, so:			// Record the substore references, so:
	// A) We can decref them in case of an error.			// A) We can decref them in case of an error.
	// B) They don't get GC'd while the uniondatapackstore holds on to them.			// B) They don't get GC'd while the uniondatapackstore holds on to them.
	pySubStores.push_back(PythonObj(item));			pySubStores.push_back(PythonObj(item));

	int isinstance = PyObject_IsInstance(item, (PyObject*)&datapackstoreType);			int isinstance = PyObject_IsInstance(item, (PyObject*)&datapackstoreType);
	if (isinstance == 0) {			if (isinstance == 1) {
	PyErr_SetString(PyExc_RuntimeError, "cuniondatapackstore only accepts cdatapackstore");
	return -1;
	} else if (isinstance != 1) {
	// Error
	return -1;
	}

	py_datapackstore pySubStore = (py_datapackstore)item;			py_datapackstore pySubStore = (py_datapackstore)item;
	stores.push_back(&pySubStore->datapackstore);			stores.push_back(&pySubStore->datapackstore);
				} else {
				//else item is python data store object
				PythonObj store(item);
				stores.push_back(new PythonDataStore(store));
				durhamUnsubmitted Not Done What is the lifetime of this PythonDataStore? The `stores` vector gets passed to the UnionDatapackStore constructor, but I don't think that class assumes ownership. The non-python stores that are passed in have their lifetime maintained by their python wrapper (py_datapackstore), so when UnionDatapackStore is deallocated, it releases it's substore reference, which allows the py_datapackstore python wrappers to be deallocated, which then deallocates the native store. In your case, the store itself will be deallocated that way, but this PythonDataStore will leak I think. durham: What is the lifetime of this PythonDataStore? The `stores` vector gets passed to the…
				ms2316AuthorUnsubmitted Not Done Yes, Agree. ms2316: Yes, Agree.
				}
	}			}

	// We have to manually call the member constructor, since the provided 'self'			// We have to manually call the member constructor, since the provided 'self'
	// is just zerod out memory.			// is just zerod out memory.
	new(&self->uniondatapackstore) std::shared_ptr<UnionDatapackStore>(new UnionDatapackStore(stores));			new(&self->uniondatapackstore) std::shared_ptr<UnionDatapackStore>(new UnionDatapackStore(stores));
	new(&self->substores) std::vector<PythonObj>();			new(&self->substores) std::vector<PythonObj>();
	self->substores = pySubStores;			self->substores = pySubStores;
	} catch (const std::exception &ex) {			} catch (const std::exception &ex) {
	}			}

	Key key(name, namelen, node, nodelen);			Key key(name, namelen, node, nodelen);

	UnionDeltaChainIterator chain = self->uniondatapackstore->getDeltaChain(key);			UnionDeltaChainIterator chain = self->uniondatapackstore->getDeltaChain(key);

	PythonObj resultChain = PyList_New(0);			PythonObj resultChain = PyList_New(0);

	delta_chain_link_t *link;			for (DeltaChainLink link = chain.next(); !link.isnull(); link = chain.next()) {
	while ((link = chain.next()) != NULL) {			PythonObj name = PyString_FromStringAndSize(link.filename(), link.filename_sz());
	PythonObj name = PyString_FromStringAndSize(link->filename, link->filename_sz);			PythonObj retnode = PyString_FromStringAndSize((const char *) link.node(), NODE_SZ);
	PythonObj retnode = PyString_FromStringAndSize((const char *) link->node, NODE_SZ);
	PythonObj deltabasenode = PyString_FromStringAndSize(			PythonObj deltabasenode = PyString_FromStringAndSize(
	(const char *) link->deltabase_node, NODE_SZ);			(const char *) link.deltabase_node(), NODE_SZ);
	PythonObj delta = PyString_FromStringAndSize(			PythonObj delta = PyString_FromStringAndSize(
	(const char *) link->delta, (Py_ssize_t) link->delta_sz);			(const char *) link.delta(), (Py_ssize_t) link.delta_sz());

	PythonObj tuple = PyTuple_Pack(5, (PyObject)name, (PyObject)retnode,			PythonObj tuple = PyTuple_Pack(5, (PyObject)name, (PyObject)retnode,
	(PyObject)name, (PyObject)deltabasenode,			(PyObject)name, (PyObject)deltabasenode,
	(PyObject*)delta);			(PyObject*)delta);

	if (PyList_Append((PyObject*)resultChain, tuple.returnval())) {			if (PyList_Append((PyObject*)resultChain, tuple.returnval())) {
	return NULL;			return NULL;
	}			}
	try {			try {
	PythonObj result = PyList_New(0);			PythonObj result = PyList_New(0);

	PythonObj inputIterator = PyObject_GetIter(keys);			PythonObj inputIterator = PyObject_GetIter(keys);
	PythonKeyIterator keysIter((PyObject*)inputIterator);			PythonKeyIterator keysIter((PyObject*)inputIterator);

	UnionDatapackStoreKeyIterator missingIter = self->uniondatapackstore->getMissing(keysIter);			UnionDatapackStoreKeyIterator missingIter = self->uniondatapackstore->getMissing(keysIter);

	Key *key;			std::shared_ptr<Key> key;
	while ((key = missingIter.next()) != NULL) {			while ((key = missingIter.next()) != NULL) {
	PythonObj missingKey = Py_BuildValue("(s#s#)", key->name.c_str(), key->name.size(),			PythonObj missingKey = Py_BuildValue("(s#s#)", key->name.c_str(), key->name.size(),
	key->node, 20);			key->node, 20);
	if (PyList_Append(result, (PyObject*)missingKey)) {			if (PyList_Append(result, (PyObject*)missingKey)) {
	return NULL;			return NULL;
	}			}
	}			}

cstore/py-pythondatastore.h

This file was added.

				// python implementation of a common interface
				//
				// Copyright 2017 Facebook, Inc.
				//
				// This software may be used and distributed according to the terms of the
				// GNU General Public License version 2 or any later version.
				//
				// no-check-code

				// The PY_SSIZE_T_CLEAN define must be defined before the Python.h include,
				// as per the documentation.

				#define PY_SSIZE_T_CLEAN
				#include <Python.h>
				#include <memory>
				#include <string>
				#include <vector>

				#include "pythonutil.h"
				#include "datastore.h"
				#include "pythondatastore.h"
				#include "key.h"

				/*
				* Build python delta chain
				*/
				static std::shared_ptr<DeltaChain> pythonstore_getdeltachain(PythonObj &store,
				const Key &key) {
				durhamUnsubmitted Not Done Why is this in the .h instead of in the c++ where it's used? Do we expect this to be used by other c++ files? Same for the other functions in this file. durham: Why is this in the .h instead of in the c++ where it's used? Do we expect this to be used by…
				ms2316AuthorUnsubmitted Not Done This .h file is included by pythondatastore.cpp. Shouldnt it be .h? ms2316: This .h file is included by pythondatastore.cpp. Shouldnt it be .h?
				durhamUnsubmitted Not Done .h files are usually used to declare function and structure definitions that need to be reused between C++ files. In this case, since they're only used on the pythondatastore.cpp file, you could just declare the functions directly in there (even though they aren't part of the PythonDataStore class). durham: .h files are usually used to declare function and structure definitions that need to be reused…

				// Call getdeltachain method of the underlying store
				PythonObj args(Py_BuildValue("ss", (key.name).c_str(),
				std::string(key.node).c_str()));

				PythonObj list = store.callmethod("getdeltachain", args);

				// Extract the delta chain from the list of tuples
				// and build a DeltaChain object from them
				std::shared_ptr< std::vector<DeltaChainLink> > links =
				std::make_shared< std::vector<DeltaChainLink> >();

				PythonObj iter = PyObject_GetIter(list);
				PyObject *tuple;
				while ((tuple = PyIter_Next(iter)) != NULL) {
				const char filename1, filename2;
				char node, deltabasenode, *delta;

				int ok = PyArg_ParseTuple(tuple, "swsww",
				&filename1, &node, &filename2, &deltabasenode, &delta);

				// the item is a five tuple
				if (ok) {
				links->push_back(DeltaChainLink(filename1, filename2, node, deltabasenode, delta));
				}
				}

				return std::make_shared<DeltaChain>(links);
				}

				/*
				* Return a pointer to a KeyIterator
				*/
				static std::shared_ptr<KeyIterator> pythonstore_getmissing(PythonObj &store, KeyIterator &missing) {

				/*
				* To implement:
				* 1. Unfold missing iterator into a list of keys
				* 2. Pass this list to the underlying python store
				* 3. Get the list of keys and transform it into vector of Keys
				* 4. Pass the vector to PythonDataStoreKeyIterator constructor
				* 5. Return shared pointer to that Iterator
				*/

				/*
				*
				* Or Use PythonKeyIterator
				*/

				//return std::make_shared<PythonDataStoreKeyIterator>();
				}


				/*
				* Pass a markforrefreshcall
				*/
				static void pythonstore_markforrefresh(PythonObj &store) {
				PythonObj args(Py_BuildValue(""));
				store.callmethod("markforrefresh", args);
				}

cstore/pythondatastore.h

This file was added.

				// pythondatastore.h - c++ declarations for a python data store
				//
				// Copyright 2017 Facebook, Inc.
				//
				// This software may be used and distributed according to the terms of the
				// GNU General Public License version 2 or any later version.
				//
				// no-check-code

				#ifndef PYTHONDATASTORE_H
				#define PYTHONDATASTORE_H

				//I Know I shouldnt include it here but I need PyObject to pass to constructor
				#include <Python.h>

				#include <memory>
				#include <vector>

				#include "datastore.h"
				#include "pythonutil.h"
				#include "key.h"


				#include "portability/portability.h"

				class PythonDataStore;
				class PythonDataStoreKeyIterator : public KeyIterator {
				private:
				std::shared_ptr< std::vector<Key> > _keys;
				public:
				PythonDataStoreKeyIterator(std::shared_ptr< std::vector<Key> > keys) :
				_keys(keys) {}

				std::shared_ptr<Key> next();
				};

				/* Manages access to a directory of datapack files. */
				class PythonDataStore : public DataStore {
				private:
				PythonObj _store; // there is a pointer to python object
				public:
				PythonDataStore(PythonObj store);

				~PythonDataStore();

				DeltaChainIterator getDeltaChain(const Key &key);

				std::shared_ptr<KeyIterator> getMissing(KeyIterator &missing);

				std::shared_ptr<DeltaChain> getDeltaChainRaw(const Key &key);

				bool contains(const Key &key);

				void markForRefresh();
				};

				#endif //PYTHONDATASTORE_H

cstore/pythondatastore.cpp

This file was added.

				// pythondatastore.cpp - implementation of a python data store
				//
				// Copyright 2017 Facebook, Inc.
				//
				// This software may be used and distributed according to the terms of the
				// GNU General Public License version 2 or any later version.
				//
				// no-check-code

				#include "pythondatastore.h"
				#include "py-pythondatastore.h"

				PythonDataStore::PythonDataStore(PythonObj store) : _store(store) {}

				DeltaChainIterator PythonDataStore::getDeltaChain(const Key &key) {
				std::shared_ptr<DeltaChain> chain = pythonstore_getdeltachain(_store, key);
				return DeltaChainIterator(chain);
				}

				std::shared_ptr<DeltaChain> PythonDataStore::getDeltaChainRaw(const Key &key) {
				return pythonstore_getdeltachain(_store, key);
				}

				std::shared_ptr<KeyIterator> PythonDataStore::getMissing(KeyIterator &missing) {
				return pythonstore_getmissing(_store, missing);
				}

				void PythonDataStore::markForRefresh() {
				pythonstore_markforrefresh(_store);
				}

				bool PythonDataStore::contains(const Key &key) {

				std::shared_ptr< std::vector<Key> > keys =
				std::make_shared< std::vector<Key> >();
				keys->push_back(key);

				PythonDataStoreKeyIterator iter(keys);

				std::shared_ptr<KeyIterator> it = pythonstore_getmissing(_store, iter);
				return (it->next() == NULL);
				}

				//The method below has to be reimplemented
				std::shared_ptr<Key> PythonDataStoreKeyIterator::next() {
				if (_keys->empty()) {
				return NULL;
				}

				std::shared_ptr<Key> key = std::make_shared<Key>(_keys->back());
				_keys->pop_back();

				return key;
				}

cstore/uniondatapackstore.h

	UnionDatapackStore &_store;			UnionDatapackStore &_store;
	KeyIterator &_missing;			KeyIterator &_missing;

	public:			public:
	UnionDatapackStoreKeyIterator(UnionDatapackStore &store, KeyIterator &missing) :			UnionDatapackStoreKeyIterator(UnionDatapackStore &store, KeyIterator &missing) :
	_store(store),			_store(store),
	_missing(missing) {}			_missing(missing) {}

	Key *next();			std::shared_ptr<Key> next();
	};			};

	class UnionDeltaChainIterator: public DeltaChainIterator {			class UnionDeltaChainIterator: public DeltaChainIterator {
	private:			private:
	UnionDatapackStore &_store;			UnionDatapackStore &_store;
	protected:			protected:
	delta_chain_t getNextChain(const Key &key);			std::shared_ptr<DeltaChain> getNextChain(const Key &key);
	public:			public:
	UnionDeltaChainIterator(UnionDatapackStore &store, const Key &key) :			UnionDeltaChainIterator(UnionDatapackStore &store, const Key &key) :
	DeltaChainIterator(),			DeltaChainIterator(),
	_store(store) {			_store(store) {
	_chains.push_back(this->getNextChain(key));			_chains.push_back(this->getNextChain(key));
	}			}
	};			};

	class UnionDatapackStore : public Store {			class UnionDatapackStore : public Store {
	public:			public:
	std::vector<DatapackStore*> _stores;			std::vector<DataStore*> _stores;

	UnionDatapackStore(std::vector<DatapackStore*> stores);			UnionDatapackStore(std::vector<DataStore*> stores);

	~UnionDatapackStore();			~UnionDatapackStore();

	ConstantStringRef get(const Key &key);			ConstantStringRef get(const Key &key);

	UnionDeltaChainIterator getDeltaChain(const Key &key);			UnionDeltaChainIterator getDeltaChain(const Key &key);

	bool contains(const Key &key);			bool contains(const Key &key);

	UnionDatapackStoreKeyIterator getMissing(KeyIterator &missing);			UnionDatapackStoreKeyIterator getMissing(KeyIterator &missing);

	void markForRefresh();			void markForRefresh();
	};			};

	#endif //UNIONDATAPACKSTORE_H			#endif //UNIONDATAPACKSTORE_H

cstore/uniondatapackstore.cpp

	#include <memory>			#include <memory>

	#include "uniondatapackstore.h"			#include "uniondatapackstore.h"

	extern "C" {			extern "C" {
	#include "mpatch.h"			#include "mpatch.h"
	}			}

	UnionDatapackStore::UnionDatapackStore(std::vector<DatapackStore*> stores) :			UnionDatapackStore::UnionDatapackStore(std::vector<DataStore*> stores) :
	_stores(stores) {			_stores(stores) {
	}			}

	UnionDatapackStore::~UnionDatapackStore() {			UnionDatapackStore::~UnionDatapackStore() {
	// TODO: we should manage the substore lifetimes here, but because they are			// TODO: we should manage the substore lifetimes here, but because they are
	// also controlled by Python, we need to let python handle it and manage the			// also controlled by Python, we need to let python handle it and manage the
	// refcount in the py_uniondatapackstore type.			// refcount in the py_uniondatapackstore type.
	}			}

	mpatch_flist* getNextLink(void* container, ssize_t index) {			mpatch_flist* getNextLink(void* container, ssize_t index) {
	std::vector<delta_chain_link_t> links = (std::vector<delta_chain_link_t>)container;			std::vector<DeltaChainLink> links = (std::vector<DeltaChainLink>)container;

	if (index < 0 \|\| (size_t)index >= links->size()) {			if (index < 0 \|\| (size_t)index >= links->size()) {
	return NULL;			return NULL;
	}			}

	delta_chain_link_t *link = links->at(index);			DeltaChainLink link = links->at(index);

	struct mpatch_flist *res;			struct mpatch_flist *res;
	if ((mpatch_decode((const char*)link->delta,			if ((mpatch_decode((const char*)link.delta(),
	(ssize_t)link->delta_sz, &res)) < 0) {			(ssize_t)link.delta_sz(), &res)) < 0) {
	throw std::logic_error("invalid patch during patch application");			throw std::logic_error("invalid patch during patch application");
	}			}

	return res;			return res;
	}			}

	ConstantStringRef UnionDatapackStore::get(const Key &key) {			ConstantStringRef UnionDatapackStore::get(const Key &key) {
	UnionDeltaChainIterator chain = this->getDeltaChain(key);			UnionDeltaChainIterator chain = this->getDeltaChain(key);

	std::vector<delta_chain_link_t*> links;			std::vector<DeltaChainLink> links;

	delta_chain_link_t *link;			for (DeltaChainLink link = chain.next(); !link.isnull(); link = chain.next()) {
	while ((link = chain.next()) != NULL) {
	links.push_back(link);			links.push_back(link);
	}			}

	delta_chain_link_t *fulltextLink = links.back();			DeltaChainLink fulltextLink = links.back();
	links.pop_back();			links.pop_back();

	// Short circuit and just return the full text if it's one long			// Short circuit and just return the full text if it's one long
	if (links.size() == 0) {			if (links.size() == 0) {
	return ConstantStringRef((const char*)fulltextLink->delta,			return ConstantStringRef((const char*)fulltextLink.delta(),
	(size_t)fulltextLink->delta_sz);			(size_t)fulltextLink.delta_sz());
	}			}

	std::reverse(links.begin(), links.end());			std::reverse(links.begin(), links.end());

	mpatch_flist *patch = mpatch_fold(&links, getNextLink, 0, links.size());			mpatch_flist *patch = mpatch_fold(&links, getNextLink, 0, links.size());
	if (!patch) { /* error already set or memory error */			if (!patch) { /* error already set or memory error */
	throw std::logic_error("mpatch failed to fold patches");			throw std::logic_error("mpatch failed to fold patches");
	}			}

	ssize_t outlen = mpatch_calcsize((ssize_t)fulltextLink->delta_sz, patch);			ssize_t outlen = mpatch_calcsize((ssize_t)fulltextLink.delta_sz(), patch);
	if (outlen < 0) {			if (outlen < 0) {
	mpatch_lfree(patch);			mpatch_lfree(patch);
	throw std::logic_error("mpatch failed to calculate size");			throw std::logic_error("mpatch failed to calculate size");
	}			}

	auto result = std::make_shared<std::string>(outlen, '\0');			auto result = std::make_shared<std::string>(outlen, '\0');
	if (mpatch_apply(&(result)[0], (const char)fulltextLink->delta,			if (mpatch_apply(&(result)[0], (const char)fulltextLink.delta(),
	(ssize_t)fulltextLink->delta_sz, patch) < 0) {			(ssize_t)fulltextLink.delta_sz(), patch) < 0) {
	mpatch_lfree(patch);			mpatch_lfree(patch);
	throw std::logic_error("mpatch failed to apply patches");			throw std::logic_error("mpatch failed to apply patches");
	}			}

	mpatch_lfree(patch);			mpatch_lfree(patch);
	return ConstantStringRef(result);			return ConstantStringRef(result);
	}			}

	delta_chain_t UnionDeltaChainIterator::getNextChain(const Key &key) {			std::shared_ptr<DeltaChain> UnionDeltaChainIterator::getNextChain(const Key &key) {
	for(std::vector<DatapackStore*>::iterator it = _store._stores.begin();			for(std::vector<DataStore*>::iterator it = _store._stores.begin();
	it != _store._stores.end();			it != _store._stores.end();
	it++) {			it++) {
	DatapackStore substore = it;			DataStore substore = it;
	delta_chain_t chain = substore->getDeltaChainRaw(key);			std::shared_ptr<DeltaChain> chain = substore->getDeltaChainRaw(key);
	if (chain.code == GET_DELTA_CHAIN_OK) {
				//if chain is delta_chain_t then performs this check
				if (chain->code() == GET_DELTA_CHAIN_OK) {
	return chain;			return chain;
	}			}
	freedeltachain(chain);
	}			}

	throw MissingKeyError("unable to find delta chain");			throw MissingKeyError("unable to find delta chain");
	}			}

	UnionDeltaChainIterator UnionDatapackStore::getDeltaChain(const Key &key) {			UnionDeltaChainIterator UnionDatapackStore::getDeltaChain(const Key &key) {
	return UnionDeltaChainIterator(*this, key);			return UnionDeltaChainIterator(*this, key);
	}			}

	Key *UnionDatapackStoreKeyIterator::next() {			std::shared_ptr<Key> UnionDatapackStoreKeyIterator::next() {
	Key *key;			std::shared_ptr<Key> key;
	while ((key = _missing.next()) != NULL) {			while ((key = _missing.next()) != NULL) {
	if (!_store.contains(*key)) {			if (!_store.contains(*key)) {
	return key;			return key;
	}			}
	}			}

	return NULL;			return NULL;
	}			}

	bool UnionDatapackStore::contains(const Key &key) {			bool UnionDatapackStore::contains(const Key &key) {
	for(std::vector<DatapackStore*>::iterator it = _stores.begin();			for(std::vector<DataStore*>::iterator it = _stores.begin();
	it != _stores.end();			it != _stores.end();
	it++) {			it++) {
	DatapackStore substore = it;			DataStore substore = it;
	if (substore->contains(key)) {			if (substore->contains(key)) {
	return true;			return true;
	}			}
	}			}
	return false;			return false;
	}			}

	UnionDatapackStoreKeyIterator UnionDatapackStore::getMissing(KeyIterator &missing) {			UnionDatapackStoreKeyIterator UnionDatapackStore::getMissing(KeyIterator &missing) {
	return UnionDatapackStoreKeyIterator(*this, missing);			return UnionDatapackStoreKeyIterator(*this, missing);
	}			}

	void UnionDatapackStore::markForRefresh() {			void UnionDatapackStore::markForRefresh() {
	for(std::vector<DatapackStore*>::iterator it = _stores.begin();			for(std::vector<DataStore*>::iterator it = _stores.begin();
	it != _stores.end();			it != _stores.end();
	it++) {			it++) {
	DatapackStore substore = it;			DataStore substore = it;
	substore->markForRefresh();			substore->markForRefresh();
	}			}
	}			}

tests/test-check-code-hg.t

	Skipping cfastmanifest/tree_path.c it has no-che?k-code (glob)			Skipping cfastmanifest/tree_path.c it has no-che?k-code (glob)
	Skipping cfastmanifest/tree_path.h it has no-che?k-code (glob)			Skipping cfastmanifest/tree_path.h it has no-che?k-code (glob)
	Skipping cfastmanifest/tree_test.c it has no-che?k-code (glob)			Skipping cfastmanifest/tree_test.c it has no-che?k-code (glob)
	Skipping clib/buffer.c it has no-che?k-code (glob)			Skipping clib/buffer.c it has no-che?k-code (glob)
	Skipping clib/buffer.h it has no-che?k-code (glob)			Skipping clib/buffer.h it has no-che?k-code (glob)
	Skipping clib/null_test.c it has no-che?k-code (glob)			Skipping clib/null_test.c it has no-che?k-code (glob)
	Skipping cstore/datapackstore.cpp it has no-che?k-code (glob)			Skipping cstore/datapackstore.cpp it has no-che?k-code (glob)
	Skipping cstore/datapackstore.h it has no-che?k-code (glob)			Skipping cstore/datapackstore.h it has no-che?k-code (glob)
				Skipping cstore/datastore.h it has no-che?k-code (glob)
	Skipping cstore/key.h it has no-che?k-code (glob)			Skipping cstore/key.h it has no-che?k-code (glob)
	Skipping cstore/match.h it has no-che?k-code (glob)			Skipping cstore/match.h it has no-che?k-code (glob)
	Skipping cstore/py-cdatapack.h it has no-che?k-code (glob)			Skipping cstore/py-cdatapack.h it has no-che?k-code (glob)
	Skipping cstore/py-cstore.cpp it has no-che?k-code (glob)			Skipping cstore/py-cstore.cpp it has no-che?k-code (glob)
	Skipping cstore/py-datapackstore.h it has no-che?k-code (glob)			Skipping cstore/py-datapackstore.h it has no-che?k-code (glob)
	Skipping cstore/py-structs.h it has no-che?k-code (glob)			Skipping cstore/py-structs.h it has no-che?k-code (glob)
	Skipping cstore/py-treemanifest.h it has no-che?k-code (glob)			Skipping cstore/py-treemanifest.h it has no-che?k-code (glob)
	Skipping cstore/pythonutil.cpp it has no-che?k-code (glob)			Skipping cstore/pythonutil.cpp it has no-che?k-code (glob)

		Path
M		cstore/datapackstore.h (76 lines)
M		cstore/datapackstore.cpp (23 lines)
A	M	cstore/datastore.h (181 lines)
M		cstore/key.h (8 lines)
M		cstore/py-datapackstore.h (58 lines)
A	M	cstore/py-pythondatastore.h (88 lines)
A	M	cstore/pythondatastore.h (57 lines)
A	M	cstore/pythondatastore.cpp (54 lines)
M		cstore/uniondatapackstore.h (8 lines)
M		cstore/uniondatapackstore.cpp (52 lines)
M		tests/test-check-code-hg.t (1 line)

This is an archive of the discontinued Mercurial Phabricator instance.

RFC cstore: gerenic uniondatapackstore
AbandonedPublic

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 1262

cstore/datapackstore.h

cstore/datapackstore.cpp

cstore/datastore.h

cstore/key.h

cstore/py-datapackstore.h

cstore/py-pythondatastore.h

cstore/pythondatastore.h

cstore/pythondatastore.cpp

cstore/uniondatapackstore.h

cstore/uniondatapackstore.cpp

tests/test-check-code-hg.t

This is an archive of the discontinued Mercurial Phabricator instance.

RFC cstore: gerenic uniondatapackstoreAbandonedPublic

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 1262

cstore/datapackstore.h

cstore/datapackstore.cpp

cstore/datastore.h

cstore/key.h

cstore/py-datapackstore.h

cstore/py-pythondatastore.h

cstore/pythondatastore.h

cstore/pythondatastore.cpp

cstore/uniondatapackstore.h

cstore/uniondatapackstore.cpp

tests/test-check-code-hg.t

RFC cstore: gerenic uniondatapackstore
AbandonedPublic

Revision Contents
Changeset List