Patchwork [V3] status: add a flag to terse the output (issue4119)

# HG changeset patch
# User Pulkit Goyal <7895pulkit@gmail.com>
# Date 1497710422 -19800
#      Sat Jun 17 20:10:22 2017 +0530
# Node ID 79bed8c52dfc6dcb0c3098a7117ae4df4c009f8c
# Parent  48ffac16c59bf9e2b2c5acab728eb398b25249eb
status: add a flag to terse the output (issue4119)

This adds an experimental flag -t/--terse which will terse the output. The terse flag
will respect other flags which filters the output. The flag takes a string
whose value can be a subsequence of "marduic" (the order does not matter here.)

Ignored files are not considered while tersing unless -i flag is passed or 'i'
is there is the terse flag value.

The flag is experimental for testing as there may be cases which will produce
strange results with the flag. We can set the terse on by default by simply
passing 'u' to the cmdutil.tersestatus().

This patch also adds a test file with tests covering the new feature.

On Wed, Jun 21, 2017 at 7:07 AM, Pulkit Goyal <7895pulkit@gmail.com> wrote:
> # HG changeset patch
> # User Pulkit Goyal <7895pulkit@gmail.com>
> # Date 1497710422 -19800
> #      Sat Jun 17 20:10:22 2017 +0530
> # Node ID 79bed8c52dfc6dcb0c3098a7117ae4df4c009f8c
> # Parent  48ffac16c59bf9e2b2c5acab728eb398b25249eb
> status: add a flag to terse the output (issue4119)

I was curious to know the status of this as it has been in the list
from a week and the next minor release is in few days. I wanted to
make sure this lands in 4.2.2 and get tested before we roll out 4.3.

Pulkit Goyal a écrit :
> # HG changeset patch
> # User Pulkit Goyal <7895pulkit@gmail.com>
> # Date 1497710422 -19800
> #      Sat Jun 17 20:10:22 2017 +0530
> # Node ID 79bed8c52dfc6dcb0c3098a7117ae4df4c009f8c
> # Parent  48ffac16c59bf9e2b2c5acab728eb398b25249eb
> status: add a flag to terse the output (issue4119)
>
> This adds an experimental flag -t/--terse which will terse the output. The terse flag
> will respect other flags which filters the output. The flag takes a string
> whose value can be a subsequence of "marduic" (the order does not matter here.)
>
> Ignored files are not considered while tersing unless -i flag is passed or 'i'
> is there is the terse flag value.

On the other hand (having played a bit locally with this), it's
surprising that `hg status --terse i` does show ignored files even
without a `--ignored` flag. In fact the same occurs with `hg status
--terse c` (shows "clean" files, even without `--clean`) so this is not
specific to "ignored" files. Is this intended?

> The flag is experimental for testing as there may be cases which will produce
> strange results with the flag. We can set the terse on by default by simply
> passing 'u' to the cmdutil.tersestatus().
>
> This patch also adds a test file with tests covering the new feature.
>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -401,6 +401,117 @@
>
>      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.
> +    """

Considering how the function is use in commands.py (stat =
cmdutil.tersestatus(repo.root, stat, ...)), I think it should return a
mercurial.scmutil.status instance instead of a plain list for consistency.

Looking at function arguments:
* "statlist" is not a proper name since this is not a list but a
    scmutil.status object
* "status" is misleading as it is actually the value of --terse option.

> +
> +    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 = ignorefn
> +        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):
> +                if not match(f):
> +                    lendir += 1
> +        except OSError:
> +            pass
> +        lendir += absentdir.get(dirname, 0)
> +        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 = [[]] * len(indexes)
> +    didsomethingchanged = False
> +
> +    for st in pycompat.bytestr(status):
> +
> +        try:
> +            ind = indexes[st]
> +        except KeyError:
> +            # TODO: Need a better error message here
> +            raise error.Abort("'%s' not recognized" % st)
> +
> +        sfiles = statlist[ind]

The code might be easier to follow if "statlist" was queried by
attribute name instead of relying on tuple indexing.
For instance:

   statusnames = {'m': 'modified', 'a': 'added', ...}
   finalrs = scmutil.status([], [], [], [], [], [], [])
   for st in pycompat.bytestr(status):
      try:
         statusname = statusnames[st]
      except KeyError:
         raise error.Abort(...)
      sfiles = getattr(statlist, statusname)
      rs = getattr(finalrs, statusname)
      # rs.append()/rs.extend()


(Not a big deal, might be improved later.)

> +        if not sfiles:
> +            continue
> +        pardict = {}
> +        for a in sfiles:
> +            par = os.path.dirname(a)
> +            pardict.setdefault(par, []).append(a)
> +
> +        rs = []
> +        newls = []
> +        for par, files in pardict.iteritems():
> +            lenpar = numfiles(par)
> +            if lenpar == len(files):
> +                newls.append(par)
> +
> +        if not newls:
> +            continue
> +
> +        while newls:
> +            newel = newls.pop()
> +            if newel == '':
> +                continue
> +            parn = os.path.dirname(newel)
> +            pardict[newel] = []
> +            # Adding pycompat.ossep as newel is a directory.
> +            pardict.setdefault(parn, []).append(newel + pycompat.ossep)
> +            lenpar = numfiles(parn)
> +            if lenpar == len(pardict[parn]):
> +                newls.append(parn)
> +
> +        # dict.values() for Py3 compatibility
> +        for files in pardict.values():
> +            rs.extend(files)
> +
> +        rs.sort()
> +        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(len(indexes)):
> +        if not finalrs[x]:
> +            finalrs[x] = statlist[x]
> +
> +    return finalrs
> +
>  def findpossible(cmd, table, strict=False):
>      """
>      Return cmd -> (aliases, command table entry)
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -4709,6 +4709,7 @@
>      ('u', 'unknown', None, _('show only unknown (not tracked) files')),
>      ('i', 'ignored', None, _('show only ignored files')),
>      ('n', 'no-status', None, _('hide status prefix')),
> +    ('t', 'terse', '', _('show the terse output (EXPERIMENTAL)')),
>      ('C', 'copies', None, _('show source of copied files')),
>      ('0', 'print0', None, _('end filenames with NUL, for use with xargs')),
>      ('', 'rev', [], _('show difference from revision'), _('REV')),
> @@ -4754,6 +4755,16 @@
>
>      .. container:: verbose
>
> +      The -t/--terse option abbreviates the output by showing directory name
> +      if all the files in it share the same status. The option expects a value
> +      which can be a string formed by using 'm', 'a', 'r', 'd', 'u', 'i', 'c'
> +      where, 'm' stands for 'modified', 'a' for 'added', 'r' for 'removed',
> +      'd' for 'deleted', 'u' for 'unknown', 'i' for 'ignored' and 'c' for clean.

Might be worth also having a "A" for "all" that'd be symmetrical with
-A/--all flag to avoid typing `--terse marduic`.

> +
> +      It terses the output of only those status which are passed. The ignored
> +      files are not considered while tersing until 'i' is there in --terse value
> +      or the --ignored option is used.
> +
>        Examples:
>
>        - show changes in the working directory relative to a
> @@ -4780,6 +4791,7 @@
>      opts = pycompat.byteskwargs(opts)
>      revs = opts.get('rev')
>      change = opts.get('change')
> +    terse = opts.get('terse')
>
>      if revs and change:
>          msg = _('cannot specify --rev and --change at the same time')
> @@ -4804,16 +4816,28 @@
>      show = [k for k in states if opts.get(k)]
>      if opts.get('all'):
>          show += ui.quiet and (states[:4] + ['clean']) or states
> +        if ui.quiet and terse:
> +            for st in ('ignored', 'unknown'):
> +                if st[0] in terse:
> +                    show.append(st)
> +
>      if not show:
>          if ui.quiet:
>              show = states[:4]
>          else:
>              show = states[:5]
> +        if terse:
> +            for st in ('ignored', 'unknown', 'clean'):
> +                if st[0] in terse:
> +                    show.append(st)
>
>      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:
> +        stat = cmdutil.tersestatus(repo.root, stat, terse,
> +                                    repo.dirstate._ignore, opts.get('ignore'))

Isn't it opts.get('ignored')?

Also, the last (wrapped) line has an extra leading space.

>      changestates = zip(states, pycompat.iterbytestr('MAR!?IC'), stat)
>
>      if (opts.get('all') or opts.get('copies')

>> Ignored files are not considered while tersing unless -i flag is passed or
>> 'i'
>> is there is the terse flag value.
>
>
> On the other hand (having played a bit locally with this), it's
> surprising that `hg status --terse i` does show ignored files even
> without a `--ignored` flag. In fact the same occurs with `hg status
> --terse c` (shows "clean" files, even without `--clean`) so this is not
> specific to "ignored" files. Is this intended?

Yeah this is intended and I agree it's surprising. I will change it to
consider the ignored files while tersing if `-i` is passed, but don't
show them in status until `--ignored` is there. Same for clean files.
Does it sound good to you?

>> +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.
>> +    """
>
> Considering how the function is use in commands.py (stat =
> cmdutil.tersestatus(repo.root, stat, ...)), I think it should return a
> mercurial.scmutil.status instance instead of a plain list for consistency.
>
> Looking at function arguments:
> * "statlist" is not a proper name since this is not a list but a
>    scmutil.status object
> * "status" is misleading as it is actually the value of --terse option.
>
Will fix these.

>> +    indexes = {'m': 0, 'a': 1, 'r': 2, 'd': 3, 'u': 4, 'i': 5, 'c': 6}
>> +    absentdir = absentones(statlist[2], statlist[3])
>> +    finalrs = [[]] * len(indexes)
>> +    didsomethingchanged = False
>> +
>> +    for st in pycompat.bytestr(status):
>> +
>> +        try:
>> +            ind = indexes[st]
>> +        except KeyError:
>> +            # TODO: Need a better error message here
>> +            raise error.Abort("'%s' not recognized" % st)
>> +
>> +        sfiles = statlist[ind]
>
> The code might be easier to follow if "statlist" was queried by
> attribute name instead of relying on tuple indexing.
> For instance:
>
>   statusnames = {'m': 'modified', 'a': 'added', ...}
>   finalrs = scmutil.status([], [], [], [], [], [], [])
>   for st in pycompat.bytestr(status):
>      try:
>         statusname = statusnames[st]
>      except KeyError:
>         raise error.Abort(...)
>      sfiles = getattr(statlist, statusname)
>      rs = getattr(finalrs, statusname)
>      # rs.append()/rs.extend()
>
>
> (Not a big deal, might be improved later.)

Will do this.

>>
>> +      The -t/--terse option abbreviates the output by showing directory
>> name
>> +      if all the files in it share the same status. The option expects a
>> value
>> +      which can be a string formed by using 'm', 'a', 'r', 'd', 'u', 'i',
>> 'c'
>> +      where, 'm' stands for 'modified', 'a' for 'added', 'r' for
>> 'removed',
>> +      'd' for 'deleted', 'u' for 'unknown', 'i' for 'ignored' and 'c' for
>> clean.
>
>
> Might be worth also having a "A" for "all" that'd be symmetrical with
> -A/--all flag to avoid typing `--terse marduic`.

That's a good idea, will do.
>
>> +    if terse:
>> +        stat = cmdutil.tersestatus(repo.root, stat, terse,
>> +                                    repo.dirstate._ignore,
>> opts.get('ignore'))
>
>
> Isn't it opts.get('ignored')?
>
> Also, the last (wrapped) line has an extra leading space.

Ah, I will fix these too.

The general idea looks good to me. I think "numfiles" needs improve to
support non-working-copy revs and I have a question about an "if" in
"absentones".

To land this fast, I think there are just 3 blocking items:

  - '-t iu' does not imply 'iu', pointed out by Denis
  - typo of 'ignore', pointed out by Denis
  - forbid '-t' to be used together with double '--rev'

Other things could be fixed as follow-ups.


Excerpts from 's message of 2017-06-21 07:07:49 +0530:
> [...]  
> +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):
> +        """
> +        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.
> +        """

This will have trouble with non-working copy revisions, like "hg status
--rev A --rev B". The more correct way might be using matcher and ctx to
check non-ignored files, avoiding os.listdir.

For the first version, I think we can say "-t" does not work with "--rev" to
avoid the issue.

> [...]
> +    def absentones(removedfiles, missingfiles):
> [...]
> +            if par not in removedfiles:
> +                absentfiles.append(par)

I don't get the  above "if". For example, with the following status:

  ! x/b/c/d/A
  ! x/b/c/d/B
  ! x/b/k
  ? y/b/c/d/e
  ? y/c/z

Using the current code, `hg st -t du` outputs:

  ! x/b/c/d/
  ! x/b/k
  ? y/

With that "if" removed, `hg st -t du` outputs:

  ! x/
  ? y/

Is the former output desired?

> [...]
> +        for par, files in pardict.iteritems():
> +            lenpar = numfiles(par)

There could be some caching around "numfiles" calls. Like if different "st"
needs data for a same "par". Previous result could be reused. That might be
also helpful to avoid race-condition. This is optional and shouldn't block
the first version.

> [...]
>      if opts.get('all'):
>          show += ui.quiet and (states[:4] + ['clean']) or states
> +        if ui.quiet and terse:
> +            for st in ('ignored', 'unknown'):
> +                if st[0] in terse:
> +                    show.append(st)
> +

Might remove this "if" as Denis suggested. That allows people to have an
alias like "status = status -t all" and do not print ignored files by
accident.


> [...]
> diff --git a/tests/test-terse-status.t b/tests/test-terse-status.t

Tests could be stronger by having nested directories cases.

On Wed, Jun 28, 2017 at 04:55:46AM +0530, Pulkit Goyal wrote:
> On Wed, Jun 21, 2017 at 7:07 AM, Pulkit Goyal <7895pulkit@gmail.com> wrote:
> > # HG changeset patch
> > # User Pulkit Goyal <7895pulkit@gmail.com>
> > # Date 1497710422 -19800
> > #      Sat Jun 17 20:10:22 2017 +0530
> > # Node ID 79bed8c52dfc6dcb0c3098a7117ae4df4c009f8c
> > # Parent  48ffac16c59bf9e2b2c5acab728eb398b25249eb
> > status: add a flag to terse the output (issue4119)
>
> I was curious to know the status of this as it has been in the list
> from a week and the next minor release is in few days. I wanted to
> make sure this lands in 4.2.2 and get tested before we roll out 4.3.

Looks like Jun et al had some comments. Can I get a v4? I'd really
like this to be in 4.3.

Thanks!

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

On Tue, Jul 11, 2017 at 11:40 PM, Augie Fackler <raf@durin42.com> wrote:

> Looks like Jun et al had some comments. Can I get a v4? I'd really
> like this to be in 4.3.
>
> Thanks!

Oh, sorry I forgot about it. Will send a v4 soon. :)