Page MenuHomePhabricator

utils: move the `dirs` definition in pathutil (API)
ClosedPublic

Authored by marmoute on Fri, Nov 8, 4:28 AM.

Details

Summary

Before this change, the dirs class was accessible through the mercurial.util
module. That module is expected to stay free of scm specific content.

The pathutil destination has been selection by Martin von Zweigbergk.

This work is part of a refactoring to unify the revlog index and the nodemap.
This unification prepare the use of a persistent nodemap.

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.Fri, Nov 8, 4:28 AM
Alphare accepted this revision.Fri, Nov 8, 5:21 AM

Can you fix up the fuzzer in contrib/fuzz/dirstate.cc?

Note that there is a comment in match.py about util.dirs. It's not that important, but it should be updated.

Can you fix up the fuzzer in contrib/fuzz/dirstate.cc?

I do not understand what need fixing there. Are you talking about D7314 ?

Can you fix up the fuzzer in contrib/fuzz/dirstate.cc?

I do not understand what need fixing there. Are you talking about D7314 ?

I was talking about this patch. https://www.mercurial-scm.org/repo/hg/file/tip/contrib/fuzz/dirs.cc#l18 is what I was talking about: some of our fuzzers actually contain a tiny Python script that gets evaluated to drive the fuzzing.

It turns out in this case it's not a problem. :)

Alphare added inline comments.Fri, Nov 8, 10:22 AM
mercurial/match.py
632–633

Should be dirstateutil.dirs

marmoute updated this revision to Diff 17750.Fri, Nov 8, 10:34 AM
martinvonz added inline comments.Fri, Nov 8, 2:05 PM
mercurial/utils/dirstateutil.py
14 ↗(On Diff #17750)

Is pathutil.py an appropriate home for this instead of creating a new file?

indygreg requested changes to this revision.Fri, Nov 8, 2:23 PM
indygreg added a subscriber: indygreg.

I agree with Martin that we should put this in pathutils.

This revision now requires changes to proceed.Fri, Nov 8, 2:23 PM
marmoute added inline comments.Fri, Nov 8, 7:19 PM
mercurial/utils/dirstateutil.py
14 ↗(On Diff #17750)

I feel like pathutil.py is mostly scm agnostic and I would rather keep it this way.

martinvonz added inline comments.Fri, Nov 8, 7:22 PM
mercurial/utils/dirstateutil.py
14 ↗(On Diff #17750)

But how is dirs not scm-agnostic? Oh, you mean the ugly skip thing? Can we see if we can just remove that instead (move it outside of the class)?

marmoute added inline comments.Fri, Nov 8, 7:39 PM
mercurial/utils/dirstateutil.py
14 ↗(On Diff #17750)

It explicitly mention dirstate and manifest. Both are scm related context.

martinvonz added inline comments.Fri, Nov 8, 7:41 PM
mercurial/utils/dirstateutil.py
14 ↗(On Diff #17750)

That's easy to fix, though. Just replace "dirstate or manifest" by "set of file paths".

marmoute retitled this revision from utils: move the `dirs` definition in a dedicated module (API) to utils: move the `dirs` definition in pathutil (API).Fri, Nov 8, 8:09 PM
marmoute edited the summary of this revision. (Show Details)
marmoute updated this revision to Diff 17821.
martinvonz added inline comments.Fri, Nov 8, 8:32 PM
mercurial/util.py
3494–3499

Could you follow up with a patch to move this one as well?

FYI, I had to resolve some conflicts with Augie's recent patches for removing r'' prefixes and for consecutive slashes. I'm pretty sure I did it right, but take a look if you want.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.