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

[dlax (Denis Laxalde)]

dlax added a comment.


  Took the time to read this in details. I agree that the algorigthm/idea is fairly simple to grasp.
  
  I don't remember the old algorithm so I'd trust @pulkit when he says that it's easier to follow/understand. I'm not sure about the "flakyness" mentioned in the commit message, but even if this does not resolve this issue (which is pretty vaguely described at this point), I think having an understandable algorithm would help in the future.

INLINE COMMENTS

> cmdutil.py:451
> +            # does the dirnode object for subdir exists
> +            if not self.subdirs.get(subdir):
> +                subdirpath = os.path.join(self.path, subdir)

`if subdir not in self.subdirs`

> cmdutil.py:475
> +
> +def _processtersestatus(subdir, tersedict, terseargs):
> +    """a recursive function which process status for a certain directory.

I think this function and the one above could be methods of the `dirnode` class as they essentially manipulate the state of a `dirnode` object. At least for me, while reading the code, that would have been clearer.

> cmdutil.py:558
> +    for subdir in rootobj.subdirs.values():
> +        _processtersestatus(subdir, tersedict, terseargs)
> +

Here and in the above `_addfilestotersed()` call, it's not easy to follow if something happens to `tersedict`. In fact, from a general point of view, I think it's not a very good practice to mutate function arguments. So instead, you could have these functions return (or better yield) `(shortstatus, filepath)` tuples from which you could update the dict here and thus avoid passing it in these functions.

In the caller, that would give:

  diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
  --- a/mercurial/cmdutil.py
  +++ b/mercurial/cmdutil.py
  @@ -551,11 +549,13 @@ def tersedir(statuslist, terseargs):
           tersedict[attrname[0]] = []
   
       # we won't be tersing the root dir, so add files in it
  -    _addfilestotersed(rootobj, tersedict)
  +    for st, fpath in _addfilestotersed(rootobj):
  +        tersedict[st].append(fpath)
   
       # process each sub-directory and build tersedlist
       for subdir in rootobj.subdirs.values():
  -        _processtersestatus(subdir, tersedict, terseargs)
  +        for st, f in _processtersestatus(subdir, terseargs):
  +            tersedict[st].append(f)

(of course function names should be changed).

Finally, I think `_processtersestatus()` can be slightly modified to avoid the call to `_addfilestotersed()` here by behaving differently if the current dirnode is the root dir or not. That would give a single loop in the diff above.

REPOSITORY
  rHG Mercurial

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

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