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

[Denis Laxalde]

Pulkit Goyal a écrit :
>> 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.
>
> Okay in that case, I will need to pass two other arguments for
> repo.root and repo.dirstate._ignore as root and ignorefn respectively.
> Or you mean to say not using them?

If you need data, pass them to the function. Having two arguments with a
defined purpose is probably clearer/safer than one "repo" argument on
which you can basically do anything.

>>
>> A couple of remarks (mostly style) below.
>
> Thanks for them, I will follow all of them in V2.
>>
>>> 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.
>
> I am not sure what a good user error will be. I am changing it to
> ("'%s' not recognized" % st) in V2.

I mean: is it really a user error (i.e. something the user can solve)?
Or is it an assertion of the algorithm? In the former case,
error.Abort() is fine. Otherwise, it's misleading as it would give the
impression that the user did something wrong.

>>
>>> +
>>> +        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]
>
> In this case 'x' won't be defined. So can't use this.

Correct. So `for x, rs in enumerate(finalrs):` would work I think.

>>
>>> +
>>> +    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
>>>
>>