This is an archive of the discontinued Mercurial Phabricator instance.

Differential D985

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

Authored by pulkit on Oct 6 2017, 4:12 PM.

Details

Reviewers
dlax
durin42
Group Reviewers
hg-reviewers
Commits
rHG7e3001b74ab3: tersestatus: re-implement the functionality to terse the status
Summary

The previous terse status implementation was hacking around os.listdir() and was
flaky. There have been a lot of instances of mercurial buildbots failing
and google's internal builds failing because of the
hacky implementation of terse status. Even though I wrote the last
implementation but it was hard for me to find the reason for the flake.

The new implementation can be slower than the old one but is clean and easy to
understand.

In this we create a node object for each directory and create a tree
like structure starting from the root of the working copy. While building the
tree like structure we store some information on the nodes which will be helpful
for deciding later whether we can terse the dir or not.
Once the whole tree is build we traverse and built the list of files for each
status with required tersing.

There is no behaviour change as the old test, test-status-terse.t passes with
the new implementation.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit created this revision.Oct 6 2017, 4:12 PM
dlax requested changes to this revision.Oct 7 2017, 4:23 AM
dlax added a subscriber: dlax.

Just a couple of comments (mostly Python things) based on a first quick read. I haven't closely looked at the algorithm yet.

Thanks for working back on this!

mercurial/cmdutil.py
540
if s not in stdic:
    raise ...
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.

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.

563

Unless I'm missing something, there's no need for this subdirlist, you can just do for subdir in rootobj.subdirs.values():.

568
for sublist in tersedlist:
    sublist.sort()
mercurial/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.

This revision now requires changes to proceed.Oct 7 2017, 4:23 AM
pulkit updated this revision to Diff 2520.Oct 7 2017, 5:40 PM
pulkit marked 4 inline comments as done.Oct 7 2017, 5:53 PM
pulkit added inline comments.
mercurial/cmdutil.py
540

Done thanks!

549

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.

554

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!

563

Ah yes. Forgot to cleanup. Thanks!

568

Thanks!

mercurial/commands.py
4794

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
  1. 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.

dlax added a comment.Oct 12 2017, 6:22 AM

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.

mercurial/cmdutil.py
547

if subdir not in self.subdirs

571

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.

654

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.

dlax requested changes to this revision.Oct 12 2017, 6:23 AM
This revision now requires changes to proceed.Oct 12 2017, 6:23 AM
dlax added inline comments.Oct 12 2017, 6:24 AM
mercurial/commands.py
4794

Ok, let's keep this for later (but not forget it :))

pulkit marked 3 inline comments as done.Oct 12 2017, 1:20 PM
In 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.

mercurial/cmdutil.py
654

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.

dlax added inline comments.Oct 12 2017, 3:21 PM
mercurial/cmdutil.py
654

I'm uncovered... Please pull and fold; that's a good start and there's probably more to do.

pulkit edited the summary of this revision. (Show Details)Oct 12 2017, 7:56 PM
pulkit added a child revision: D1042: tersestatus: make methods part of the dirnode class.
pulkit updated this revision to Diff 2645.
pulkit added a comment.Oct 12 2017, 8:03 PM

The next two patches D1042 and D1043, though sent by me are patches by Denis. They were some good cleanups which I think was wrong to fold into my original patch, hence sent separately.

dlax added a comment.Oct 13 2017, 5:42 AM
In D985#17380, @pulkit wrote:

The next two patches D1042 and D1043, though sent by me are patches by Denis. They were some good cleanups which I think was wrong to fold into my original patch, hence sent separately.

Surprisingly, Phabricator does not make the patch user information visible... but through phabread, I get it.

durin42 added a subscriber: durin42.Oct 14 2017, 12:45 AM
durin42 added inline comments.
mercurial/cmdutil.py
406–408

Perversely, locals are significantly faster than globals in Python. Not going to block landing this patch on the change, but it *might* show up in a perf* command to cache this locally in the function it's used in.

durin42 accepted this revision.Oct 14 2017, 12:46 AM
Closed by commit rHG7e3001b74ab3: tersestatus: re-implement the functionality to terse the status (authored by pulkit). · Explain WhyOct 14 2017, 12:55 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/cmdutil.py (324 lines)
Mmercurial/commands.py (16 lines)

Diff 2515

mercurial/cmdutil.py

pass pass
def recordinwlock(ui, repo, message, match, opts): def recordinwlock(ui, repo, message, match, opts):
with repo.wlock(): with repo.wlock():
return recordfunc(ui, repo, message, match, opts) return recordfunc(ui, repo, message, match, opts)
return commit(ui, repo, recordinwlock, pats, opts) return commit(ui, repo, recordinwlock, pats, opts)
def tersestatus(root, statlist, status, ignorefn, ignore):
"""
Returns a list of statuses with directory collapsed if all the files in the
directory has the same status.
"""
def numfiles(dirname): stdic = {'m': 0, 'a': 1, 'r': 2, 'd': 3, 'u': 4, 'i': 5, 'c': 6}
# extracted at module level as it's required each time a file will be added
# to dirnode class object below
durin42Unsubmitted
Not Done

Perversely, locals are significantly faster than globals in Python. Not going to block landing this patch on the change, but it *might* show up in a perf* command to cache this locally in the function it's used in.

durin42: Perversely, locals are significantly faster than globals in Python. Not going to block landing…
pathsep = pycompat.ossep
class dirnode(object):
""" represents a directory in user working copy
stores information which is required for purpose of tersing the status
path is the path to the directory
statuses is a set of statuses of all files in this directory (this includes
all the files in all the subdirectories too)
files is a list of files which are direct child of this directory
subdirs is a dictionary of sub-directory name as the key and it's own
dirnode object as the value
""" """
Calculates the number of tracked files in a given directory which also
includes files which were removed or deleted. Considers ignored files def __init__(self, dirpath):
if ignore argument is True or 'i' is present in status argument. self.path = dirpath
self.statuses = set([])
self.files = []
self.subdirs = {}
def _addfileindir(self, filename, status):
""" adds a file in this directory as the direct child
""" """
if lencache.get(dirname): self.files.append((filename, status))
return lencache[dirname]
if 'i' in status or ignore: def addfile(self, filename, status):
def match(localpath): """ adds a file which is present in this directory to its direct parent
absolutepath = os.path.join(root, localpath) dirnode object
if os.path.isdir(absolutepath) and isemptydir(absolutepath):
return True
return False
else:
def match(localpath):
# there can be directory whose all the files are ignored and
# hence the drectory should also be ignored while counting
# number of files or subdirs in it's parent directory. This
# checks the same.
# XXX: We need a better logic here.
if os.path.isdir(os.path.join(root, localpath)):
return isignoreddir(localpath)
else:
# XXX: there can be files which have the ignored pattern but
# are not ignored. That leads to bug in counting number of
# tracked files in the directory.
return ignorefn(localpath)
lendir = 0
abspath = os.path.join(root, dirname)
# There might be cases when a directory does not exists as the whole
# directory can be removed and/or deleted.
try:
for f in os.listdir(abspath):
localpath = os.path.join(dirname, f)
if not match(localpath):
lendir += 1
except OSError:
pass
lendir += len(absentdir.get(dirname, []))
lencache[dirname] = lendir
return lendir
def isemptydir(abspath):
"""
Check whether a directory is empty or not, i.e. there is no files in the
directory and all its subdirectories.
"""
for f in os.listdir(abspath):
fullpath = os.path.join(abspath, f)
if os.path.isdir(fullpath):
# recursion here
ret = isemptydir(fullpath)
if not ret:
return False
else:
return False
return True
def isignoreddir(localpath): if the file is not direct child of this directory, we traverse to the
"""Return True if `localpath` directory is ignored or contains only directory of which this file is a direct child of and add the file there
ignored files and should hence be considered ignored.
""" """
dirpath = os.path.join(root, localpath)
if ignorefn(dirpath): # the filename contains a path separator, it means it's not the direct
return True # child of this directory
for f in os.listdir(dirpath): if pathsep in filename:
filepath = os.path.join(dirpath, f) subdir, filep = filename.split(pathsep, 1)
if os.path.isdir(filepath):
# recursion here # does the dirnode object for subdir exists
ret = isignoreddir(os.path.join(localpath, f)) if not self.subdirs.get(subdir):
if not ret: subdirpath = os.path.join(self.path, subdir)
return False self.subdirs[subdir] = dirnode(subdirpath)
# try adding the file in subdir
self.subdirs[subdir].addfile(filep, status)
else: else:
if not ignorefn(os.path.join(localpath, f)): self._addfileindir(filename, status)
return False
return True
def absentones(removedfiles, missingfiles): if status not in self.statuses:
self.statuses.add(status)
def _addfilestotersedlist(path, files, tersedlist):
""" adds files to the their respective status list in the final tersed list
path is the path of parent directory of the file
files is a list of tuple where each tuple is (filename, status)
tersedlist is the status list in which we are storing the statuses after
tersing
""" """
Returns a dictionary of directories with files in it which are either for f, st in files:
removed or missing (deleted) in them. tersedlist[stdic[st]].append(os.path.join(path, f))
def _processtersestatus(subdir, tersedlist, terseargs):
"""a recursive function which process status for a certain directory.
subdir is an oject of dirnode class defined below. each object of dirnode
class has a set of statuses which files in that directory has. This ease our
check whether we can terse that directory or not.
tersedlist is the list which contains currently processed file status and
will be the final list which contains statuses as per required tersing.
terseargs is the string of arguments passed by the user with `--terse` flag.
Following are the cases which can happen:
1) All the files in the directory (including all the files in its
subdirectories) share the same status and the user has asked us to terse
that status. -> we add the directory name to status list and return
2) If '1)' does not happen, we do following:
a) Add all the files which are in this directory (only the ones in
this directory, not the subdirs) to their respective status list
b) Recurse the function on all the subdirectories of this directory
""" """
absentdir = {}
absentfiles = removedfiles + missingfiles
while absentfiles:
f = absentfiles.pop()
par = os.path.dirname(f)
if par == '':
continue
# we need to store files rather than number of files as some files
# or subdirectories in a directory can be counted twice. This is
# also we have used sets here.
try:
absentdir[par].add(f)
except KeyError:
absentdir[par] = set([f])
absentfiles.append(par)
return absentdir
indexes = {'m': 0, 'a': 1, 'r': 2, 'd': 3, 'u': 4, 'i': 5, 'c': 6}
# get a dictonary of directories and files which are missing as os.listdir()
# won't be able to list them.
absentdir = absentones(statlist[2], statlist[3])
finalrs = [[]] * len(indexes)
didsomethingchanged = False
# dictionary to store number of files and subdir in a directory so that we
# don't compute that again.
lencache = {}
for st in pycompat.bytestr(status): if len(subdir.statuses) == 1:
onlyst = subdir.statuses.pop()
try: # Making sure we terse only when the status abbreviation is passed as
ind = indexes[st] # terse argument
except KeyError: if onlyst in terseargs:
# TODO: Need a better error message here tersedlist[stdic[onlyst]].append(subdir.path + pycompat.ossep)
raise error.Abort("'%s' not recognized" % st) return
sfiles = statlist[ind] # add the files to status list
if not sfiles: _addfilestotersedlist(subdir.path, subdir.files, tersedlist)
continue
pardict = {}
for a in sfiles:
par = os.path.dirname(a)
pardict.setdefault(par, []).append(a)
rs = []
newls = []
for par, files in sorted(pardict.iteritems()):
lenpar = numfiles(par)
if lenpar == len(files):
newls.append(par)
if not newls: #recurse on the subdirs
continue for dirobj in subdir.subdirs.values():
_processtersestatus(dirobj, tersedlist, terseargs)
while newls: def tersedir(statuslist, terseargs):
newel = newls.pop() """ terses the status if all the files in a directory shares the same status
if newel == '':
continue statuslist is scmutil.status() object which contains a list of files for
parn = os.path.dirname(newel) each status.
pardict[newel] = [] terseargs is string which is passed by the user as the argument to `--terse`
# Adding pycompat.ossep as newel is a directory. flag.
pardict.setdefault(parn, []).append(newel + pycompat.ossep)
lenpar = numfiles(parn) The function makes a tree of objects of dirnode class, and at each node it
if lenpar == len(pardict[parn]): stores the information required to know whether we can terse a certain
newls.append(parn) directory or not.
# dict.values() for Py3 compatibility tersedlist (defined in the function) is a list of lists which in the end
for files in pardict.values(): will contain files for each status. This list is passed to other functions
rs.extend(files) which builds it.
"""
rs.sort()
finalrs[ind] = rs # checking the argument validity
didsomethingchanged = True for s in terseargs:
try:
# If nothing is changed, make sure the order of files is preserved. stdic[s]
if not didsomethingchanged: except KeyError:
return statlist raise error.Abort(_("'%s' not recognized") % s)
dlaxUnsubmitted
Not Done
if s not in stdic:
    raise ...
dlax: ``` if s not in stdic: raise ... ```
pulkitAuthorUnsubmitted
Not Done

Done thanks!

pulkit: Done thanks!
for x in xrange(len(indexes)):
if not finalrs[x]:
finalrs[x] = statlist[x]
return finalrs # creating a dirnode object for the root of the repo
rootobj = dirnode('')
statusdic = {'modified': 'm', 'added': 'a', 'deleted': 'd', 'clean': 'c',
'unknown': 'u', 'ignored': 'i', 'removed': 'r'}
for attrname, sname in statusdic.iteritems():
dlaxUnsubmitted
Not Done

if subdir not in self.subdirs

dlax: `if subdir not in self.subdirs`
for f in getattr(statuslist, attrname):
rootobj.addfile(f, sname)
dlaxUnsubmitted
Not Done

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.

dlax: No need for the intermediate `statusdic`, just iterate as `for name in ('modified', 'added', ...
pulkitAuthorUnsubmitted
Not Done

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.

pulkit: I have implemented the first advice. For passing `statlist` to `__init__`, I disagree with…
# list of subdirectory of the root dir
subdirlist = []
# a list containing list for each status
tersedlist = [[], [], [], [], [], [], []]
dlaxUnsubmitted
Done

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.

dlax: I wonder if using a dict keyed by "status short name" (later transformed into a list of lists)…
pulkitAuthorUnsubmitted
Not Done

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!

pulkit: I didn't exactly understand whether you mean that will be simpler or not but I have updated the…
# we won't be tersing the root dir, so add files in it
_addfilestotersedlist(rootobj.path, rootobj.files, tersedlist)
for subdirobj in rootobj.subdirs.values():
subdirlist.append(subdirobj)
# process each sub-directory and build tersedlist
for subdir in subdirlist:
dlaxUnsubmitted
Done

Unless I'm missing something, there's no need for this subdirlist, you can just do for subdir in rootobj.subdirs.values():.

dlax: Unless I'm missing something, there's no need for this `subdirlist`, you can just do `for…
pulkitAuthorUnsubmitted
Not Done

Ah yes. Forgot to cleanup. Thanks!

pulkit: Ah yes. Forgot to cleanup. Thanks!
_processtersestatus(subdir, tersedlist, terseargs)
# sort each list
for x in xrange(7):
tersedlist[x].sort()
dlaxUnsubmitted
Done
for sublist in tersedlist:
    sublist.sort()
dlax: ``` for sublist in tersedlist: sublist.sort() ```
pulkitAuthorUnsubmitted
Not Done

Thanks!

pulkit: Thanks!
return tersedlist
def _commentlines(raw): def _commentlines(raw):
dlaxUnsubmitted
Not Done

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.

dlax: I think this function and the one above could be methods of the `dirnode` class as they…
'''Surround lineswith a comment char and a new line''' '''Surround lineswith a comment char and a new line'''
lines = raw.splitlines() lines = raw.splitlines()
commentedlines = ['# %s' % line for line in lines] commentedlines = ['# %s' % line for line in lines]
return '\n'.join(commentedlines) + '\n' return '\n'.join(commentedlines) + '\n'
def _conflictsmsg(repo): def _conflictsmsg(repo):
# avoid merge cycle # avoid merge cycle
from . import merge as mergemod from . import merge as mergemod
# The merge state is part of a list that will be iterated over. # The merge state is part of a list that will be iterated over.
# They need to be last because some of the other unfinished states may also # They need to be last because some of the other unfinished states may also
# be in a merge or update state (eg. rebase, histedit, graft, etc). # be in a merge or update state (eg. rebase, histedit, graft, etc).
# We want those to have priority. # We want those to have priority.
('merge', _mergepredicate, _mergemsg), ('merge', _mergepredicate, _mergemsg),
) )
def _getrepostate(repo): def _getrepostate(repo):
# experimental config: commands.status.skipstates # experimental config: commands.status.skipstates
dlaxUnsubmitted
Not Done

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.

dlax: Here and in the above `_addfilestotersed()` call, it's not easy to follow if something happens…
pulkitAuthorUnsubmitted
Not Done

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.

pulkit: Well I agree with you on this. Yesterday I accidentally stumbled on your repo at hg.logilab.
dlaxUnsubmitted
Not Done

I'm uncovered... Please pull and fold; that's a good start and there's probably more to do.

dlax: I'm uncovered... Please pull and fold; that's a good start and there's probably more to do.
skip = set(repo.ui.configlist('commands', 'status.skipstates')) skip = set(repo.ui.configlist('commands', 'status.skipstates'))
for state, statedetectionpredicate, msgfn in STATES: for state, statedetectionpredicate, msgfn in STATES:
if state in skip: if state in skip:
continue continue
if statedetectionpredicate(repo): if statedetectionpredicate(repo):
return (state, statedetectionpredicate, msgfn) return (state, statedetectionpredicate, msgfn)
def morestatus(repo, fm): def morestatus(repo, fm):

mercurial/commands.py

if not show: if not show:
if ui.quiet: if ui.quiet:
show = states[:4] show = states[:4]
else: else:
show = states[:5] show = states[:5]
m = scmutil.match(repo[node2], pats, opts) m = scmutil.match(repo[node2], pats, opts)
stat = repo.status(node1, node2, m,
'ignored' in show, 'clean' in show, 'unknown' in show,
opts.get('subrepos'))
if terse: if terse:
stat = cmdutil.tersestatus(repo.root, stat, terse, # we need to compute clean and unknown to terse
repo.dirstate._ignore, opts.get('ignored')) stat = repo.status(node1, node2, m,
'ignored' in show or 'i' in terse,
True, True, opts.get('subrepos'))
stat = cmdutil.tersedir(stat, terse)
else:
stat = repo.status(node1, node2, m,
'ignored' in show, 'clean' in show,
'unknown' in show, opts.get('subrepos'))
dlaxUnsubmitted
Not Done

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.

dlax: For readability, I'd suggest to only have a single call to `repo.status()` and define the…
pulkitAuthorUnsubmitted
Not Done

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
  1. 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.

pulkit: I can implement your suggestion using following ways: 1) add 'clean' and 'unknown' to `show`…
dlaxUnsubmitted
Not Done

Ok, let's keep this for later (but not forget it :))

dlax: Ok, let's keep this for later (but not forget it :))
changestates = zip(states, pycompat.iterbytestr('MAR!?IC'), stat) changestates = zip(states, pycompat.iterbytestr('MAR!?IC'), stat)
if (opts.get('all') or opts.get('copies') if (opts.get('all') or opts.get('copies')
or ui.configbool('ui', 'statuscopies')) and not opts.get('no_status'): or ui.configbool('ui', 'statuscopies')) and not opts.get('no_status'):
copy = copies.pathcopies(repo[node1], repo[node2], m) copy = copies.pathcopies(repo[node1], repo[node2], m)
ui.pager('status') ui.pager('status')
fm = ui.formatter('status', opts) fm = ui.formatter('status', opts)