( )⚙ D10031 revlog-index: add `replace_sidedata_info` method

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10031

revlog-index: add `replace_sidedata_info` method
ClosedPublic

Authored by Alphare on Feb 19 2021, 6:16 AM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Commits
rHG502e795b55ac: revlog-index: add `replace_sidedata_info` method
Summary

During a pull operation where the server does not provide sidedata, the client
that requires it should generate them on-the-fly. In the generic case, we need
to wait for the changelog + manifests + filelogs to be added, since we don't
know what the sidedata computers might need: this means rewriting the sidedata
of index entries from within the pull transaction (and no further back) right
after we've added them.

Both Python and C implementations only allow for rewriting the sidedata offset
and length for revs within the transaction where they were created.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

Alphare created this revision.Feb 19 2021, 6:16 AM
baymax updated this revision to Diff 25998.Mar 1 2021, 11:52 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

marmoute requested changes to this revision.Mar 3 2021, 8:54 AM
marmoute added a subscriber: marmoute.
marmoute added inline comments.
mercurial/cext/revlog.c
519–536

Should we explicitly check that no value other than the sidedata have been changed ?

mercurial/pure/parsers.py
120–124

The changeset description state that we don't check the transaction thing in the python version. I assume that this is because we have some extra information in the C code that the python version don't have. Could we fix it ?

128–134

Same feedback as the C code, could we validate than we don't modify anything else than the side-data index ?

This revision now requires changes to proceed.Mar 3 2021, 8:54 AM
Alphare marked an inline comment as done.Mar 4 2021, 10:06 AM
Alphare added inline comments.
mercurial/cext/revlog.c
519–536

I changed it so the only arguments are sidedata length and offset

mercurial/pure/parsers.py
120–124

I misunderstood the Python implementation, this is now fixed.

Alphare edited the summary of this revision. (Show Details)Mar 4 2021, 10:18 AM
Alphare updated this revision to Diff 26094.
marmoute requested changes to this revision.Mar 9 2021, 11:52 AM

Great, I just have a final feedback. We should probably rename this method to something less generic than replace. What about replace_sidedata_info ?

This revision now requires changes to proceed.Mar 9 2021, 11:52 AM
Alphare added a comment.Mar 10 2021, 1:46 PM
In D10031#154121, @marmoute wrote:

Great, I just have a final feedback. We should probably rename this method to something less generic than replace. What about replace_sidedata_info ?

Sure

Alphare updated this revision to Diff 26225.Mar 10 2021, 2:12 PM
marmoute accepted this revision.Mar 11 2021, 1:16 PM
Alphare retitled this revision from revlog-index: add `replace` method to revlog-index: add `replace_sidedata_info` method.Mar 12 2021, 6:34 AM
Alphare edited the summary of this revision. (Show Details)
Alphare added a commit: rHG502e795b55ac: revlog-index: add `replace_sidedata_info` method.Mar 15 2021, 2:35 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG502e795b55ac: revlog-index: add `replace_sidedata_info` method (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/cext/revlog.c (76 lines)
Mmercurial/pure/parsers.py (17 lines)

Diff 25998

mercurial/cext/revlog.c

if (self->ntinitialized) if (self->ntinitialized)
nt_insert(&self->nt, c_node_id, rev); nt_insert(&self->nt, c_node_id, rev);
Py_CLEAR(self->headrevs); Py_CLEAR(self->headrevs);
Py_RETURN_NONE; Py_RETURN_NONE;
} }
/* Replace an existing index entry with a new value. This should not be used
outside of the context of sidedata rewriting, inside the transaction that
creates the given revision. */
static PyObject *index_replace(indexObject *self, PyObject *args)
{
uint64_t offset_flags, unified_revlog_id, sidedata_offset;
int rev, comp_len, uncomp_len, base_rev, link_rev, parent_1, parent_2;
Py_ssize_t c_node_id_len, rank, sidedata_comp_len;
const char *c_node_id;
char *data;
PyObject *obj;
if (self->hdrsize == v1_hdrsize || self->inlined) {
/*
There is a bug in the transaction handling when going from an
inline revlog to a separate index and data file. Turn it off until
it's fixed, since v2 revlogs sometimes get rewritten on exchange.
See issue6485.
*/
raise_revlog_error();
return NULL;
}
if (!PyArg_ParseTuple(args, "nO", &rev, &obj))
return NULL;
if (rev < 0 || rev >= index_length(self)) {
PyErr_SetString(PyExc_IndexError, "revision outside index");
return NULL;
}
if (rev < self->length) {
PyErr_SetString(
PyExc_IndexError,
"cannot rewrite entries outside of this transaction");
return NULL;
}
if (!PyArg_ParseTuple(obj, v2_tuple_format, &offset_flags, &comp_len,
&uncomp_len, &base_rev, &link_rev, &parent_1,
&parent_2, &c_node_id, &c_node_id_len,
&unified_revlog_id, &rank, &sidedata_offset,
&sidedata_comp_len)) {
PyErr_SetString(PyExc_TypeError, "12-tuple required");
return NULL;
}
if (c_node_id_len != self->nodelen) {
PyErr_SetString(PyExc_TypeError, "invalid node");
return NULL;
}
/* Rewrite the newly added node, offset from the "already on-disk"
* length */
data = self->added + self->hdrsize * (rev - self->length);
putbe32(offset_flags >> 32, data);
putbe32(offset_flags & 0xffffffffU, data + 4);
putbe32(comp_len, data + 8);
putbe32(uncomp_len, data + 12);
putbe32(base_rev, data + 16);
putbe32(link_rev, data + 20);
putbe32(parent_1, data + 24);
putbe32(parent_2, data + 28);
memcpy(data + 32, c_node_id, c_node_id_len);
/* Padding since SHA-1 is only 20 bytes for now */
memset(data + 32 + c_node_id_len, 0, 32 - c_node_id_len);
putbe64(unified_revlog_id, data + 64);
putbe32(rank, data + 72);
putbe64(sidedata_offset, data + 76);
putbe32(sidedata_comp_len, data + 84);
/* Padding for 96 bytes alignment */
memset(data + 88, 0, self->hdrsize - 88);
marmouteUnsubmitted
Not Done

Should we explicitly check that no value other than the sidedata have been changed ?

marmoute: Should we explicitly check that no value other than the sidedata have been changed ?
AlphareAuthorUnsubmitted
Done

I changed it so the only arguments are sidedata length and offset

Alphare: I changed it so the only arguments are sidedata length and offset
Py_RETURN_NONE;
}
static PyObject *index_stats(indexObject *self) static PyObject *index_stats(indexObject *self)
{ {
PyObject *obj = PyDict_New(); PyObject *obj = PyDict_New();
PyObject *s = NULL; PyObject *s = NULL;
PyObject *t = NULL; PyObject *t = NULL;
if (obj == NULL) if (obj == NULL)
return NULL; return NULL;
{"has_node", (PyCFunction)index_m_has_node, METH_O, {"has_node", (PyCFunction)index_m_has_node, METH_O,
"return True if the node exist in the index"}, "return True if the node exist in the index"},
{"rev", (PyCFunction)index_m_rev, METH_O, {"rev", (PyCFunction)index_m_rev, METH_O,
"return `rev` associated with a node or raise RevlogError"}, "return `rev` associated with a node or raise RevlogError"},
{"computephasesmapsets", (PyCFunction)compute_phases_map_sets, METH_VARARGS, {"computephasesmapsets", (PyCFunction)compute_phases_map_sets, METH_VARARGS,
"compute phases"}, "compute phases"},
{"reachableroots2", (PyCFunction)reachableroots2, METH_VARARGS, {"reachableroots2", (PyCFunction)reachableroots2, METH_VARARGS,
"reachableroots"}, "reachableroots"},
{"replace", (PyCFunction)index_replace, METH_VARARGS,
"replace an existing index entry with a new value"},
{"headrevs", (PyCFunction)index_headrevs, METH_VARARGS, {"headrevs", (PyCFunction)index_headrevs, METH_VARARGS,
"get head revisions"}, /* Can do filtering since 3.2 */ "get head revisions"}, /* Can do filtering since 3.2 */
{"headrevsfiltered", (PyCFunction)index_headrevs, METH_VARARGS, {"headrevsfiltered", (PyCFunction)index_headrevs, METH_VARARGS,
"get filtered head revisions"}, /* Can always do filtering */ "get filtered head revisions"}, /* Can always do filtering */
{"issnapshot", (PyCFunction)index_issnapshot, METH_O, {"issnapshot", (PyCFunction)index_issnapshot, METH_O,
"True if the object is a snapshot"}, "True if the object is a snapshot"},
{"findsnapshots", (PyCFunction)index_findsnapshots, METH_VARARGS, {"findsnapshots", (PyCFunction)index_findsnapshots, METH_VARARGS,
"Gather snapshot data in a cache dict"}, "Gather snapshot data in a cache dict"},

mercurial/pure/parsers.py

else: else:
index = self._calculate_index(i) index = self._calculate_index(i)
data = self._data[index : index + self.index_size] data = self._data[index : index + self.index_size]
r = _unpack(self.index_format, data) r = _unpack(self.index_format, data)
if self._lgt and i == 0: if self._lgt and i == 0:
r = (offset_type(0, gettype(r[0])),) + r[1:] r = (offset_type(0, gettype(r[0])),) + r[1:]
return r return r
def replace(self, i, tup):
"""
Replace an existing index entry with a new value. This should
not be used outside of the context of sidedata rewriting, inside the
transaction that creates the revision `i`.
"""
marmouteUnsubmitted
Not Done

The changeset description state that we don't check the transaction thing in the python version. I assume that this is because we have some extra information in the C code that the python version don't have. Could we fix it ?

marmoute: The changeset description state that we don't check the transaction thing in the python version.
AlphareAuthorUnsubmitted
Done

I misunderstood the Python implementation, this is now fixed.

Alphare: I misunderstood the Python implementation, this is now fixed.
if i < 0:
raise KeyError
self._check_index(i)
if i >= self._lgt:
self._extra[i - self._lgt] = _pack(self.index_format, *tup)
else:
index = self._calculate_index(i)
self._data[index : index + self.index_size] = _pack(
self.index_format, *tup
)
marmouteUnsubmitted
Done

Same feedback as the C code, could we validate than we don't modify anything else than the side-data index ?

marmoute: Same feedback as the C code, could we validate than we don't modify anything else than the side…
class IndexObject(BaseIndexObject): class IndexObject(BaseIndexObject):
def __init__(self, data): def __init__(self, data):
assert len(data) % self.index_size == 0 assert len(data) % self.index_size == 0
self._data = data self._data = data
self._lgt = len(data) // self.index_size self._lgt = len(data) // self.index_size
self._extra = [] self._extra = []