( )⚙ D11320 dirstate-item: feed more information to `__init__`

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11320

dirstate-item: feed more information to `__init__`
ClosedPublic

Authored by marmoute on Aug 21 2021, 5:56 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG22c39f8acf78: dirstate-item: feed more information to `__init__`
Summary

Instead of processing the "rich" value at the dirstatemap level, we can now directly pass them to the DirstateItem object. This will make the object free to store whatever its want and to implements it logic whatever its want.

For now… we simply process the flag and store the same good old value. However
this pave the way for doing things differently once the rest of dirstatemap
code is updated.

Nobody call this code yet.

Diff Detail

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

Event Timeline

marmoute created this revision.Aug 21 2021, 5:56 AM
Alphare added a subscriber: Alphare.Aug 25 2021, 10:12 AM
Alphare added inline comments.
mercurial/cext/parsers.c
107–161

This differs from the Python implementation (it uses None). Why should 'r' be a good idea here, in particular with the arms that don't set another state later?

Alphare requested changes to this revision.Aug 25 2021, 10:40 AM

(marking this as requesting changes so that it's easier to see where an answer is needed)

This revision now requires changes to proceed.Aug 25 2021, 10:40 AM
marmoute requested review of this revision.Aug 26 2021, 3:43 AM
marmoute added inline comments.
mercurial/cext/parsers.c
107–161

We don't have "None" for char, and there are no arms that don't set another state.

Alphare added inline comments.Aug 26 2021, 3:46 AM
mercurial/cext/parsers.c
107–161

Sure, but there still is the default state (!(p1_tracked || p2_tracked || wc_tracked)), so calling this without any information yields a removed item. I don't think that's a big issue, but probably I would set it to "?" or some other char that doesn't risk silently being eaten anywhere without the dirstate complaining.

marmoute added inline comments.Aug 26 2021, 4:43 AM
mercurial/cext/parsers.c
107–161

In that default state, the file is untracked, (and not entry should actually exist) so r is the least unsuitable state at hand.

Alphare accepted this revision.Aug 26 2021, 4:43 AM
This revision is now accepted and ready to land.Aug 26 2021, 4:43 AM
baymax updated this revision to Diff 30046.Aug 26 2021, 9:00 AM

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

marmoute added a commit: rHG22c39f8acf78: dirstate-item: feed more information to `__init__`.Aug 27 2021, 8:37 AM
Closed by commit rHG22c39f8acf78: dirstate-item: feed more information to `__init__` (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/cext/parsers.c (97 lines)
Mmercurial/pure/parsers.py (73 lines)

Diff 30046

mercurial/cext/parsers.c

} }
static PyObject *dirstate_item_new(PyTypeObject *subtype, PyObject *args, static PyObject *dirstate_item_new(PyTypeObject *subtype, PyObject *args,
PyObject *kwds) PyObject *kwds)
{ {
/* We do all the initialization here and not a tp_init function because /* We do all the initialization here and not a tp_init function because
* dirstate_item is immutable. */ * dirstate_item is immutable. */
dirstateItemObject *t; dirstateItemObject *t;
char state; int wc_tracked;
int size, mode, mtime; int p1_tracked;
if (!PyArg_ParseTuple(args, "ciii", &state, &mode, &size, &mtime)) { int p2_tracked;
int merged;
int clean_p1;
int clean_p2;
int possibly_dirty;
PyObject *parentfiledata;
static char *keywords_name[] = {
"wc_tracked", "p1_tracked", "p2_tracked",
"merged", "clean_p1", "clean_p2",
"possibly_dirty", "parentfiledata", NULL,
};
wc_tracked = 0;
p1_tracked = 0;
p2_tracked = 0;
merged = 0;
clean_p1 = 0;
clean_p2 = 0;
possibly_dirty = 0;
parentfiledata = Py_None;
if (!PyArg_ParseTupleAndKeywords(args, kwds, "iiiiiiiO", keywords_name,
&wc_tracked, &p1_tracked, &p2_tracked,
&merged, &clean_p1, &clean_p2,
&possibly_dirty, &parentfiledata
)) {
return NULL;
}
if (merged && (clean_p1 || clean_p2)) {
PyErr_SetString(PyExc_RuntimeError,
"`merged` argument incompatible with "
"`clean_p1`/`clean_p2`");
return NULL; return NULL;
} }
t = (dirstateItemObject *)subtype->tp_alloc(subtype, 1); t = (dirstateItemObject *)subtype->tp_alloc(subtype, 1);
if (!t) { if (!t) {
return NULL; return NULL;
} }
t->state = state; t->state = 'r';
t->mode = mode; t->mode = 0;
t->size = size; t->size = dirstate_v1_nonnormal;
t->mtime = mtime; t->mtime = ambiguous_time;
if (!(p1_tracked || p2_tracked || wc_tracked)) {
/* Nothing special to do, file is untracked */
} else if (merged) {
t->state = 'm';
t->size = dirstate_v1_from_p2;
t->mtime = ambiguous_time;
} else if (!(p1_tracked || p2_tracked) && wc_tracked) {
t->state = 'a';
t->size = dirstate_v1_nonnormal;
t->mtime = ambiguous_time;
} else if ((p1_tracked || p2_tracked) && !wc_tracked) {
t->state = 'r';
t->size = 0;
t->mtime = 0;
} else if (clean_p2 && wc_tracked) {
t->state = 'n';
t->size = dirstate_v1_from_p2;
t->mtime = ambiguous_time;
} else if (!p1_tracked && p2_tracked && wc_tracked) {
t->state = 'n';
t->size = dirstate_v1_from_p2;
t->mtime = ambiguous_time;
} else if (possibly_dirty) {
t->state = 'n';
t->size = dirstate_v1_nonnormal;
t->mtime = ambiguous_time;
} else if (wc_tracked) {
/* this is a "normal" file */
if (parentfiledata == Py_None) {
PyErr_SetString(
PyExc_RuntimeError,
"failed to pass parentfiledata for a normal file");
return NULL;
}
if (!PyTuple_CheckExact(parentfiledata)) {
PyErr_SetString(
PyExc_TypeError,
"parentfiledata should be a Tuple or None");
return NULL;
}
t->state = 'n';
t->mode =
(int)PyLong_AsLong(PyTuple_GetItem(parentfiledata, 0));
t->size =
(int)PyLong_AsLong(PyTuple_GetItem(parentfiledata, 1));
t->mtime =
(int)PyLong_AsLong(PyTuple_GetItem(parentfiledata, 2));
} else {
PyErr_SetString(PyExc_RuntimeError, "unreachable");
return NULL;
}
AlphareUnsubmitted
Not Done

This differs from the Python implementation (it uses None). Why should 'r' be a good idea here, in particular with the arms that don't set another state later?

Alphare: This differs from the Python implementation (it uses `None`). Why should `'r'` be a good idea…
marmouteAuthorUnsubmitted
Done

We don't have "None" for char, and there are no arms that don't set another state.

marmoute: We don't have "None" for char, and there are no arms that don't set another state.
AlphareUnsubmitted
Not Done

Sure, but there still is the default state (!(p1_tracked || p2_tracked || wc_tracked)), so calling this without any information yields a removed item. I don't think that's a big issue, but probably I would set it to "?" or some other char that doesn't risk silently being eaten anywhere without the dirstate complaining.

Alphare: Sure, but there still is the default state `(!(p1_tracked || p2_tracked || wc_tracked))`, so…
marmouteAuthorUnsubmitted
Done

In that default state, the file is untracked, (and not entry should actually exist) so r is the least unsuitable state at hand.

marmoute: In that default state, the file is untracked, (and not entry should actually exist) so `r` is…
return (PyObject *)t; return (PyObject *)t;
} }
static void dirstate_item_dealloc(PyObject *o) static void dirstate_item_dealloc(PyObject *o)
{ {
PyObject_Del(o); PyObject_Del(o);
} }

mercurial/pure/parsers.py

- mtime, - mtime,
""" """
_state = attr.ib() _state = attr.ib()
_mode = attr.ib() _mode = attr.ib()
_size = attr.ib() _size = attr.ib()
_mtime = attr.ib() _mtime = attr.ib()
def __init__(self, state, mode, size, mtime): def __init__(
self._state = state self,
self._mode = mode wc_tracked=False,
self._size = size p1_tracked=False,
self._mtime = mtime p2_tracked=False,
merged=False,
clean_p1=False,
clean_p2=False,
possibly_dirty=False,
parentfiledata=None,
):
if merged and (clean_p1 or clean_p2):
msg = b'`merged` argument incompatible with `clean_p1`/`clean_p2`'
raise error.ProgrammingError(msg)
self._state = None
self._mode = 0
self._size = NONNORMAL
self._mtime = AMBIGUOUS_TIME
if not (p1_tracked or p2_tracked or wc_tracked):
pass # the object has no state to record
elif merged:
self._state = b'm'
self._size = FROM_P2
self._mtime = AMBIGUOUS_TIME
elif not (p1_tracked or p2_tracked) and wc_tracked:
self._state = b'a'
self._size = NONNORMAL
self._mtime = AMBIGUOUS_TIME
elif (p1_tracked or p2_tracked) and not wc_tracked:
self._state = b'r'
self._size = 0
self._mtime = 0
elif clean_p2 and wc_tracked:
self._state = b'n'
self._size = FROM_P2
self._mtime = AMBIGUOUS_TIME
elif not p1_tracked and p2_tracked and wc_tracked:
self._state = b'n'
self._size = FROM_P2
self._mtime = AMBIGUOUS_TIME
elif possibly_dirty:
self._state = b'n'
self._size = NONNORMAL
self._mtime = AMBIGUOUS_TIME
elif wc_tracked:
# this is a "normal" file
if parentfiledata is None:
msg = b'failed to pass parentfiledata for a normal file'
raise error.ProgrammingError(msg)
self._state = b'n'
self._mode = parentfiledata[0]
self._size = parentfiledata[1]
self._mtime = parentfiledata[2]
else:
assert False, 'unreachable'
@classmethod @classmethod
def from_v1_data(cls, state, mode, size, mtime): def from_v1_data(cls, state, mode, size, mtime):
"""Build a new DirstateItem object from V1 data """Build a new DirstateItem object from V1 data
Since the dirstate-v1 format is frozen, the signature of this function Since the dirstate-v1 format is frozen, the signature of this function
is not expected to change, unlike the __init__ one. is not expected to change, unlike the __init__ one.
""" """
return cls( instance = cls()
state=state, instance._state = state
mode=mode, instance._mode = mode
size=size, instance._size = size
mtime=mtime, instance._mtime = mtime
) return instance
def set_possibly_dirty(self): def set_possibly_dirty(self):
"""Mark a file as "possibly dirty" """Mark a file as "possibly dirty"
This means the next status call will have to actually check its content This means the next status call will have to actually check its content
to make sure it is correct. to make sure it is correct.
""" """
self._mtime = AMBIGUOUS_TIME self._mtime = AMBIGUOUS_TIME