[PATCH 1 of 3] cmdutil: add a function to terse the status

[Denis Laxalde]

Pulkit Goyal a écrit :
> # HG changeset patch
> # User Pulkit Goyal <7895pulkit at gmail.com>
> # Date 1497462524 -19800
> #      Wed Jun 14 23:18:44 2017 +0530
> # Node ID 622220c3eb0a6ae2f0e7dfc94ac103ad4bfc53aa
> # Parent  f12a51d828c6bac3ed7683bd07f7d04232727da5
> cmdutil: add a function to terse the status
>
> This function will be used to terse the output of `hg status`. It collapses a
> directory if all the files (except ignored) shares the same status and consider
> ignored ones when ignore is passed through either terse or normally. A further
> patch will add tests which will make things clearer.

Thanks for working on this!

I wonder if this tersestatus function couldn't be unit-tested (might be
easier if it didn't need the "repo" argument). I'd make it easier to
understand its behaviour I think.

A couple of remarks (mostly style) below.

> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -402,6 +402,115 @@
>
>      return commit(ui, repo, recordinwlock, pats, opts)
>
> +def tersestatus(repo, statlist, status, ignore):
> +    """
> +    Returns a list of statuses with directory collapsed if all the files in the
> +    directory has the same status.
> +    """
> +
> +    def numfiles(dirname):
> +        """
> +        Calculates the number of tracked files in a given directory which also
> +        includes files which were removed or deleted. Considers ignored files
> +        if ignore argument is True or 'i' is present in status argument.
> +        """
> +        if 'i' in status or ignore:
> +            def match(fname):
> +                return False
> +        else:
> +            match = repo.dirstate._ignore
> +        lendir = 0
> +        abspath = repo.root + pycompat.ossep + dirname

Could you use os.path.join() ?

> +        for f in os.listdir(abspath):
> +            if not match(f):
> +                lendir += 1
> +        if absentdir.get(dirname):
> +            lendir += absentdir[dirname]

Python nit: `if dict.get(key):` is better written as `if key in dict:`.
Here, you could also have written `lendir += absentdir.get(dirname, 0)`
and drop the 'if'.

> +        return lendir
> +
> +    def absentones(removedfiles, missingfiles):
> +        """
> +        Returns a dictionary of directories and number of files which are either
> +        removed or missing (deleted) in them.
> +        """
> +        absentdir = {}
> +        absentfiles = removedfiles + missingfiles
> +        while absentfiles:
> +            f = absentfiles.pop()
> +            par = os.path.dirname(f)
> +            if par == '':
> +                continue
> +            try:
> +                absentdir[par] += 1
> +            except KeyError:
> +                absentdir[par] = 1
> +            if par not in removedfiles:
> +                absentfiles.append(par)
> +        return absentdir
> +
> +    indexes = {'m': 0, 'a': 1, 'r': 2, 'd': 3, 'u': 4, 'i': 5, 'c': 6}
> +    absentdir = absentones(statlist[2], statlist[3])
> +    finalrs = [[], [], [], [], [], [], []]

is it `finalrs = [[]] * len(indexes)` ?

> +    didsomethingchanged = False
> +
> +    for st in pycompat.bytestr(status):
> +
> +        try:
> +            ind = indexes[st]
> +        except KeyError:
> +            raise error.Abort("Unable to parse the terse status, use marduic")

What's "marduic"?
Also, is it really an user error? If not an assertion might be better maybe.

> +
> +        sfiles = statlist[ind]
> +        if not sfiles:
> +            continue
> +        pardict = {}
> +        for a in sfiles:
> +            par = os.path.dirname(a)
> +            try:
> +                pardict[par].append(a)
> +            except KeyError:
> +                pardict[par] = [a]

Other python nit: `pardict.setdefault(par, []).append(a)` to replace the
try/except block.

> +
> +        rs = []
> +        newls = []
> +        for par in iter(pardict):

iteritems() since you further need the value (pardict[par])

> +            lenpar = numfiles(par)
> +            if lenpar == len(pardict[par]):
> +                newls.append(par)
> +
> +        if not newls:
> +            continue
> +
> +        while newls:
> +            newel = newls.pop()
> +            if newel == '':
> +                continue
> +            parn = os.path.dirname(newel)
> +            pardict[newel] = []
> +            try:
> +                pardict[parn].append(newel + pycompat.ossep)
> +            except KeyError:
> +                pardict[parn] = [newel + pycompat.ossep]

setdefault could be used here as well.
Also, why the `+ pycompat.ossep`? Might be worth adding a comment.

> +            lenpar = numfiles(parn)
> +            if lenpar == len(pardict[parn]):
> +                newls.append(parn)
> +
> +        for par, files in pardict.iteritems():

iteritems() -> itervalues(), since "par" is not used.

> +            rs.extend(files)
> +
> +        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(7):
> +        if not finalrs[x]:
> +            finalrs[x] = statlist[x]

for rs in finalrs:
   if not rs:
      rs[:] = statlist[x]

> +
> +    return finalrs
> +
>  def findpossible(cmd, table, strict=False):
>      """
>      Return cmd -> (aliases, command table entry)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>