This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9298

copies: introduce the hg-cpython wrapper for `combine_changeset_copies`
ClosedPublic

Authored by marmoute on Nov 12 2020, 10:16 AM.

Details

Reviewers
Alphare
pulkit
Group Reviewers
hg-reviewers
Commits
rHG50c5ee3bdf9a: copies: introduce the hg-cpython wrapper for `combine_changeset_copies`
Summary

This patch focus on the hg-cpython part of this work. Bridging the python code
with the new rust code in hg-core. The next patch will actually plug this in
the python code.

The rust code use multiple Python callback, python related error within this
callback are not expected unless they are a programming error or a data
corruption. In addition, these callback will slowly be replaced by native Rust
code. For these reasons, we use will deal with unexpected error within this
callback using rust Panic and let the rust-cpython layer deal with raising a
Python exception.

The code dealing with the ChangedFile instance is repeating itself a lot. I did
not factor these duplication out because that whole code will get replaced by
entirely different one in a handful of changesets.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

marmoute created this revision.Nov 12 2020, 10:16 AM
Alphare requested changes to this revision.Nov 13 2020, 6:21 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/hg-cpython/src/copy_tracing.rs
45

Please use multi-line string literals and/or intermediate variables for long messages.

let a = "this is \
        "multi-line";
62

second item*

72

I understand that there is usually a lot of boilerplate involved in hg-cpython, but this is worth refactoring into two functions: one for getting parent copies and one for getting a set.

This revision now requires changes to proceed.Nov 13 2020, 6:21 AM
marmoute added inline comments.Nov 13 2020, 5:39 PM
rust/hg-cpython/src/copy_tracing.rs
72

I got tempted to do so, but eventually did not do it, since that code get replaced in a handful of changeset. I'll update the commit message to point that out.

marmoute edited the summary of this revision. (Show Details)Nov 13 2020, 7:05 PM
marmoute updated this revision to Diff 23493.
Alphare accepted this revision.Nov 16 2020, 6:12 AM
pulkit accepted this revision.Nov 28 2020, 4:42 AM
This revision is now accepted and ready to land.Nov 28 2020, 4:42 AM

Revision Contents
Changeset List

Diff 23767

rust/hg-cpython/src/copy_tracing.rs

  • This file was added.
use cpython::ObjectProtocol;
use cpython::PyBool;
use cpython::PyBytes;
use cpython::PyDict;
use cpython::PyList;
use cpython::PyModule;
use cpython::PyObject;
use cpython::PyResult;
use cpython::PyTuple;
use cpython::Python;
use hg::copy_tracing::combine_changeset_copies;
use hg::copy_tracing::ChangedFiles;
use hg::copy_tracing::RevInfo;
use hg::utils::hg_path::HgPathBuf;
use hg::Revision;
/// Combines copies information contained into revision `revs` to build a copy
/// map.
///
/// See mercurial/copies.py for details
pub fn combine_changeset_copies_wrapper(
py: Python,
revs: PyList,
children: PyDict,
target_rev: Revision,
rev_info: PyObject,
is_ancestor: PyObject,
) -> PyResult<PyDict> {
let revs: PyResult<_> =
revs.iter(py).map(|r| Ok(r.extract(py)?)).collect();
// Wrap the `is_ancestor` python callback as a Rust closure
//
// No errors are expected from the Python side, and they will should only
// happens in case of programing error or severe data corruption. Such
// errors will raise panic and the rust-cpython harness will turn them into
// Python exception.
let is_ancestor_wrap = |anc: Revision, desc: Revision| -> bool {
is_ancestor
.call(py, (anc, desc), None)
.expect(
"rust-copy-tracing: python call to `is_ancestor` \
failed",
)
AlphareUnsubmitted
Not Done

Please use multi-line string literals and/or intermediate variables for long messages.

let a = "this is \
        "multi-line";
Alphare: Please use multi-line string literals and/or intermediate variables for long messages. ``` let…
.cast_into::<PyBool>(py)
.expect(
"rust-copy-tracing: python call to `is_ancestor` \
returned unexpected non-Bool value",
)
.is_true()
};
// Wrap the `rev_info_maker` python callback as a Rust closure
//
// No errors are expected from the Python side, and they will should only
// happens in case of programing error or severe data corruption. Such
// errors will raise panic and the rust-cpython harness will turn them into
// Python exception.
let rev_info_maker = |rev: Revision| -> RevInfo {
let res: PyTuple = rev_info
.call(py, (rev,), None)
AlphareUnsubmitted
Not Done

second item*

Alphare: second item*
.expect("rust-copy-tracing: python call to `rev_info` failed")
.cast_into(py)
.expect(
"rust-copy_tracing: python call to `rev_info` returned \
unexpected non-Tuple value",
);
let p1 = res.get_item(py, 0).extract(py).expect(
"rust-copy-tracing: \
rev_info return is invalid, first item is a not a revision",
);
AlphareUnsubmitted
Not Done

I understand that there is usually a lot of boilerplate involved in hg-cpython, but this is worth refactoring into two functions: one for getting parent copies and one for getting a set.

Alphare: I understand that there is usually a lot of boilerplate involved in `hg-cpython`, but this is…
marmouteAuthorUnsubmitted
Done

I got tempted to do so, but eventually did not do it, since that code get replaced in a handful of changeset. I'll update the commit message to point that out.

marmoute: I got tempted to do so, but eventually did not do it, since that code get replaced in a handful…
let p2 = res.get_item(py, 1).extract(py).expect(
"rust-copy-tracing: \
rev_info return is invalid, second item is a not a revision",
);
let changes = res.get_item(py, 2);
let files;
if !changes
.hasattr(py, "copied_from_p1")
.expect("rust-copy-tracing: python call to `hasattr` failed")
{
files = ChangedFiles::new_empty();
} else {
let p1_copies: PyDict = changes
.getattr(py, "copied_from_p1")
.expect(
"rust-copy-tracing: retrieval of python attribute \
`copied_from_p1` failed",
)
.cast_into(py)
.expect(
"rust-copy-tracing: failed to convert `copied_from_p1` \
to PyDict",
);
let p1_copies: PyResult<_> = p1_copies
.items(py)
.iter()
.map(|(key, value)| {
let key = key.extract::<PyBytes>(py).expect(
"rust-copy-tracing: conversion of copy destination to\
PyBytes failed",
);
let key = key.data(py);
let value = value.extract::<PyBytes>(py).expect(
"rust-copy-tracing: conversion of copy source to \
PyBytes failed",
);
let value = value.data(py);
Ok((
HgPathBuf::from_bytes(key),
HgPathBuf::from_bytes(value),
))
})
.collect();
let p2_copies: PyDict = changes
.getattr(py, "copied_from_p2")
.expect(
"rust-copy-tracing: retrieval of python attribute \
`copied_from_p2` failed",
)
.cast_into(py)
.expect(
"rust-copy-tracing: failed to convert `copied_from_p2` \
to PyDict",
);
let p2_copies: PyResult<_> = p2_copies
.items(py)
.iter()
.map(|(key, value)| {
let key = key.extract::<PyBytes>(py).expect(
"rust-copy-tracing: conversion of copy destination to \
PyBytes failed");
let key = key.data(py);
let value = value.extract::<PyBytes>(py).expect(
"rust-copy-tracing: conversion of copy source to \
PyBytes failed",
);
let value = value.data(py);
Ok((
HgPathBuf::from_bytes(key),
HgPathBuf::from_bytes(value),
))
})
.collect();
let removed: PyObject = changes.getattr(py, "removed").expect(
"rust-copy-tracing: retrieval of python attribute \
`removed` failed",
);
let removed: PyResult<_> = removed
.iter(py)
.expect(
"rust-copy-tracing: getting a python iterator over the \
`removed` set failed",
)
.map(|filename| {
let filename = filename
.expect(
"rust-copy-tracing: python iteration over the \
`removed` set failed",
)
.extract::<PyBytes>(py)
.expect(
"rust-copy-tracing: \
conversion of `removed` item to PyBytes failed",
);
let filename = filename.data(py);
Ok(HgPathBuf::from_bytes(filename))
})
.collect();
let merged: PyObject = changes.getattr(py, "merged").expect(
"rust-copy-tracing: retrieval of python attribute \
`merged` failed",
);
let merged: PyResult<_> = merged
.iter(py)
.expect(
"rust-copy-tracing: getting a python iterator over the \
`merged` set failed",
)
.map(|filename| {
let filename = filename
.expect(
"rust-copy-tracing: python iteration over the \
`merged` set failed",
)
.extract::<PyBytes>(py)
.expect(
"rust-copy-tracing: \
conversion of `merged` item to PyBytes failed",
);
let filename = filename.data(py);
Ok(HgPathBuf::from_bytes(filename))
})
.collect();
let salvaged: PyObject = changes.getattr(py, "salvaged").expect(
"rust-copy-tracing: retrieval of python attribute \
`salvaged` failed",
);
let salvaged: PyResult<_> = salvaged
.iter(py)
.expect(
"rust-copy-tracing: getting a python iterator over the \
`salvaged` set failed",
)
.map(|filename| {
let filename = filename
.expect(
"rust-copy-tracing: python iteration over the \
`salvaged` set failed",
)
.extract::<PyBytes>(py)
.expect(
"rust-copy-tracing: \
conversion of `salvaged` item to PyBytes failed",
);
let filename = filename.data(py);
Ok(HgPathBuf::from_bytes(filename))
})
.collect();
files = ChangedFiles::new(
removed.unwrap(),
merged.unwrap(),
salvaged.unwrap(),
p1_copies.unwrap(),
p2_copies.unwrap(),
);
}
(p1, p2, files)
};
let children: PyResult<_> = children
.items(py)
.iter()
.map(|(k, v)| {
let v: &PyList = v.cast_as(py)?;
let v: PyResult<_> =
v.iter(py).map(|child| Ok(child.extract(py)?)).collect();
Ok((k.extract(py)?, v?))
})
.collect();
let res = combine_changeset_copies(
revs?,
children?,
target_rev,
&rev_info_maker,
&is_ancestor_wrap,
);
let out = PyDict::new(py);
for (dest, source) in res.into_iter() {
out.set_item(
py,
PyBytes::new(py, &dest.into_vec()),
PyBytes::new(py, &source.into_vec()),
)?;
}
Ok(out)
}
/// Create the module, with `__package__` given from parent
pub fn init_module(py: Python, package: &str) -> PyResult<PyModule> {
let dotted_name = &format!("{}.copy_tracing", package);
let m = PyModule::new(py, dotted_name)?;
m.add(py, "__package__", package)?;
m.add(py, "__doc__", "Copy tracing - Rust implementation")?;
m.add(
py,
"combine_changeset_copies",
py_fn!(
py,
combine_changeset_copies_wrapper(
revs: PyList,
children: PyDict,
target_rev: Revision,
rev_info: PyObject,
is_ancestor: PyObject
)
),
)?;
let sys = PyModule::import(py, "sys")?;
let sys_modules: PyDict = sys.get(py, "modules")?.extract(py)?;
sys_modules.set_item(py, dotted_name, &m)?;
Ok(m)
}

rust/hg-cpython/src/lib.rs

#[macro_use] #[macro_use]
extern crate cpython; extern crate cpython;
pub mod ancestors; pub mod ancestors;
mod cindex; mod cindex;
mod conversion; mod conversion;
#[macro_use] #[macro_use]
pub mod ref_sharing; pub mod ref_sharing;
pub mod copy_tracing;
pub mod dagops; pub mod dagops;
pub mod debug; pub mod debug;
pub mod dirstate; pub mod dirstate;
pub mod discovery; pub mod discovery;
pub mod exceptions; pub mod exceptions;
pub mod parsers; pub mod parsers;
pub mod revlog; pub mod revlog;
pub mod utils; pub mod utils;
py_module_initializer!(rustext, initrustext, PyInit_rustext, |py, m| { py_module_initializer!(rustext, initrustext, PyInit_rustext, |py, m| {
m.add( m.add(
py, py,
"__doc__", "__doc__",
"Mercurial core concepts - Rust implementation", "Mercurial core concepts - Rust implementation",
)?; )?;
let dotted_name: String = m.get(py, "__name__")?.extract(py)?; let dotted_name: String = m.get(py, "__name__")?.extract(py)?;
m.add(py, "ancestor", ancestors::init_module(py, &dotted_name)?)?; m.add(py, "ancestor", ancestors::init_module(py, &dotted_name)?)?;
m.add(py, "dagop", dagops::init_module(py, &dotted_name)?)?; m.add(py, "dagop", dagops::init_module(py, &dotted_name)?)?;
m.add(py, "debug", debug::init_module(py, &dotted_name)?)?; m.add(py, "debug", debug::init_module(py, &dotted_name)?)?;
m.add(
py,
"copy_tracing",
copy_tracing::init_module(py, &dotted_name)?,
)?;
m.add(py, "discovery", discovery::init_module(py, &dotted_name)?)?; m.add(py, "discovery", discovery::init_module(py, &dotted_name)?)?;
m.add(py, "dirstate", dirstate::init_module(py, &dotted_name)?)?; m.add(py, "dirstate", dirstate::init_module(py, &dotted_name)?)?;
m.add(py, "revlog", revlog::init_module(py, &dotted_name)?)?; m.add(py, "revlog", revlog::init_module(py, &dotted_name)?)?;
m.add( m.add(
py, py,
"parsers", "parsers",
parsers::init_parsers_module(py, &dotted_name)?, parsers::init_parsers_module(py, &dotted_name)?,
)?; )?;