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

pulkit (Pulkit Goyal) phabricator at mercurial-scm.org
Sat Oct 7 17:53:09 EDT 2017


pulkit marked 4 inline comments as done.
pulkit added inline comments.

INLINE COMMENTS

> dlax wrote in cmdutil.py:540
>   if s not in stdic:
>       raise ...

Done thanks!

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

I have implemented the first advice.

For passing `statlist` to `__init__`, I disagree with that. If you see the line above, that's also calling  `__init__`  which is creating node objects for the subdirectories.  If I pass `statlist` to `__init__`,  I will need some kind of new way to find what will be the `statlist` which is needed to pass there.

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

I didn't exactly understand whether you mean that will be simpler or not but I have updated the implementation using a dictionary. It that's what you meant, thanks. Good advice!

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

Ah yes. Forgot to cleanup. Thanks!

> dlax wrote in cmdutil.py:568
>   for sublist in tersedlist:
>       sublist.sort()

Thanks!

> dlax wrote in commands.py:4794
> 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.

I can implement your suggestion using following ways:

1. add 'clean' and 'unknown' to `show` variable and take them out later as that variable is used later also

2. add a new variable which handles this.

I certainly don't like first one and hesitant to go for second one as that seems kind of ugly. But if you insist me I can go with second option or a better way if you suggest any.

Have not adopted the suggestion in the updated version.

REPOSITORY
  rHG Mercurial

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

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


More information about the Mercurial-devel mailing list