D985: tersestatus: re-implement the functionality to terse the status

[dlax (Denis Laxalde)]

dlax requested changes to this revision.
dlax added a comment.
This revision now requires changes to proceed.


  Just a couple of comments (mostly Python things) based on a first quick read. I haven't closely looked at the algorithm yet.
  
  Thanks for working back on this!

INLINE COMMENTS

> cmdutil.py:540
>          except KeyError:
> -            # TODO: Need a better error message here
> -            raise error.Abort("'%s' not recognized" % st)
> -
> -        sfiles = statlist[ind]
> -        if not sfiles:
> -            continue
> -        pardict = {}
> -        for a in sfiles:
> -            par = os.path.dirname(a)
> -            pardict.setdefault(par, []).append(a)
> -
> -        rs = []
> -        newls = []
> -        for par, files in sorted(pardict.iteritems()):
> -            lenpar = numfiles(par)
> -            if lenpar == len(files):
> -                newls.append(par)
> -
> -        if not newls:
> -            continue
> -
> -        while newls:
> -            newel = newls.pop()
> -            if newel == '':
> -                continue
> -            parn = os.path.dirname(newel)
> -            pardict[newel] = []
> -            # Adding pycompat.ossep as newel is a directory.
> -            pardict.setdefault(parn, []).append(newel + pycompat.ossep)
> -            lenpar = numfiles(parn)
> -            if lenpar == len(pardict[parn]):
> -                newls.append(parn)
> -
> -        # dict.values() for Py3 compatibility
> -        for files in pardict.values():
> -            rs.extend(files)
> -
> -        rs.sort()
> -        finalrs[ind] = rs
> -        didsomethingchanged = True
> -
> -    # If nothing is changed, make sure the order of files is preserved.
> -    if not didsomethingchanged:
> -        return statlist
> -
> -    for x in xrange(len(indexes)):
> -        if not finalrs[x]:
> -            finalrs[x] = statlist[x]
> -
> -    return finalrs
> +            raise error.Abort(_("'%s' not recognized") % s)
> +

if s not in stdic:
      raise ...

> cmdutil.py:549
> +        for f in getattr(statuslist, attrname):
> +            rootobj.addfile(f, sname)
> +

No need for the intermediate `statusdic`, just iterate as `for name in ('modified', 'added', ...):` and use `name[0]` as `sname`.

Alternatively (probably better), `dirnode`'s `__init__` could take the `statuslist` as an argument and do the `addfile()` calls itself.

> cmdutil.py:554
> +    # a list containing list for each status
> +    tersedlist = [[], [], [], [], [], [], []]
> +

I wonder if using a dict keyed by "status short name" (later transformed into a list of lists) instead of `tersedlist` that is queried by index through the `stdic` indirection wouldn't be simpler.

> cmdutil.py:563
> +    # process each sub-directory and build tersedlist
> +    for subdir in subdirlist:
> +        _processtersestatus(subdir, tersedlist, terseargs)

Unless I'm missing something, there's no need for this `subdirlist`, you can just do `for subdir in rootobj.subdirs.values():`.

> cmdutil.py:568
> +    for x in xrange(7):
> +        tersedlist[x].sort()
> +    return tersedlist

for sublist in tersedlist:
      sublist.sort()

> commands.py:4794
> +                           'ignored' in show, 'clean' in show,
> +                           'unknown' in show, opts.get('subrepos'))
> +

For readability, I'd suggest to only have a single call to `repo.status()` and define the `ignored`, `clean` and `unknown` kwargs in the `if terse:` before that call.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D985

To: pulkit, #hg-reviewers, dlax
Cc: dlax, mercurial-devel