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

[pulkit (Pulkit Goyal)]

pulkit marked 3 inline comments as done.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D985#17043, @dlax wrote:
  
  > Took the time to read this in details. I agree that the algorigthm/idea is fairly simple to grasp.
  
  
  Yay! thanks.
  
  > 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.
  
  For context, the idea of old algorithm was using os.listdir() to list files in each directory and check their status and terse accordingly. But due to edge cases arising due to the idea, the implementation was very much hacky.
  I am sorry I didn't mentioned about flakyness. The flakyness is observed in our mercurial buildbots and some internal buildbots at Google. I am unable to find failed builds at the moment but the builds occasionally fails with test-status-terse.t and it was happening from couple of months. I will include this thing in updated version.

INLINE COMMENTS

> dlax wrote in cmdutil.py:558
> 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.

Well I agree with you on this. Yesterday I accidentally stumbled on your repo at hg.logilab.org/ by clicking a link in one of the commit message and saw that you have the above feedback implemented yourself. Being lazy, I want to ask you, will you like to followup on these or I can just pull from them and fold them in this commit giving proper credits to you.

REPOSITORY
  rHG Mercurial

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

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