martinvonz added a comment.
I haven't reviewed much of it, but here are some initial comments INLINE COMMENTS > uncommit.py:101 > > -def _fixdirstate(repo, oldctx, newctx, match=None): > - """ fix the dirstate after switching the working directory from oldctx to > - newctx which can be result of either unamend or uncommit. > +def _uncommitdirstate(repo, oldctx, newctx, match, interactive): > + """Fix the dirstate after switching the working directory from I don't think we need to rename it from `_fixdirstate()`, especially since it's used for both uncommit and unamend. (Fix commit message too, of course, if you switch back to the old name.) > uncommit.py:114 > + # us in defining the exact behavior > + m, a, r = repo.status(oldctx, ctx, match=match)[:3] > + for f in m: I've been trying to avoid accessing status fields by index because it's not very readable. Can you use the same pattern as below instead (i.e. `s.modifed` etc)? > uncommit.py:399 > + match=None > + interactive=0 > + _uncommitdirstate(repo, curctx, newpredctx, match, interactive) Instead of defining a value here, specify the argument by name when you call the function: `_uncommitdirstate(..., interactive=False)`. But it's probably better to just make `interactive` default to `False` and not pass it here. > uncommit.py:400 > + interactive=0 > + _uncommitdirstate(repo, curctx, newpredctx, match, interactive) > Keep the match optional instead? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6005 To: taapas1128, #hg-reviewers Cc: martinvonz, mercurial-devel _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
