Submitter | Pulkit Goyal |
---|---|
Date | June 21, 2017, 1:37 a.m. |
Message ID | <79bed8c52dfc6dcb0c30.1498009069@workspace> |
Download | mbox | patch |
Permalink | /patch/21566/ |
State | Superseded |
Headers | show |
Comments
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. :)
Patch
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. + """ + + 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] + 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. + + 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')) changestates = zip(states, pycompat.iterbytestr('MAR!?IC'), stat) if (opts.get('all') or opts.get('copies') diff --git a/tests/test-completion.t b/tests/test-completion.t --- a/tests/test-completion.t +++ b/tests/test-completion.t @@ -230,7 +230,7 @@ push: force, rev, bookmark, branch, new-branch, ssh, remotecmd, insecure remove: after, force, subrepos, include, exclude serve: accesslog, daemon, daemon-postexec, errorlog, port, address, prefix, name, web-conf, webdir-conf, pid-file, stdio, cmdserver, templates, style, ipv6, certificate, subrepos - status: all, modified, added, removed, deleted, clean, unknown, ignored, no-status, copies, print0, rev, change, include, exclude, subrepos, template + status: all, modified, added, removed, deleted, clean, unknown, ignored, no-status, terse, copies, print0, rev, change, include, exclude, subrepos, template summary: remote update: clean, check, merge, date, rev, tool addremove: similarity, subrepos, include, exclude, dry-run diff --git a/tests/test-terse-status.t b/tests/test-terse-status.t new file mode 100644 --- /dev/null +++ b/tests/test-terse-status.t @@ -0,0 +1,139 @@ + $ mkdir folder + $ cd folder + $ hg init + $ mkdir x + $ touch a b x/aa.o x/bb.o + $ hg status + ? a + ? b + ? x/aa.o + ? x/bb.o + + $ hg status --terse u + ? a + ? b + ? x/ + $ hg status --terse maudric + ? a + ? b + ? x/ + $ hg status --terse madric + ? a + ? b + ? x/aa.o + ? x/bb.o + $ hg status --terse f + abort: 'f' not recognized + [255] + +Add a .hgignore so that we can also have ignored files + + $ echo ".*\.o" > .hgignore + $ hg status + ? .hgignore + ? a + ? b + $ hg status -i + I x/aa.o + I x/bb.o + +Tersing ignored files + $ hg status -t i + ? .hgignore + ? a + ? b + I x/ + +Adding more files + $ mkdir y + $ touch x/aa x/bb y/l y/m y/l.o y/m.o + + $ hg status --terse u + ? .hgignore + ? a + ? b + ? x/ + ? y/ + $ hg add x/aa x/bb .hgignore + $ hg status --terse au + A .hgignore + A x/ + ? a + ? b + ? y/ + +Including ignored files + + $ hg status --terse aui + A .hgignore + A x/aa + A x/bb + ? a + ? b + ? y/l + ? y/m + I x/aa.o + I x/bb.o + I y/l.o + I y/m.o + $ hg status --terse au -i + I x/aa.o + I x/bb.o + I y/l.o + I y/m.o + +Committing some of the files + + $ hg commit x/aa x/bb .hgignore -m "First commit" + $ hg status + ? a + ? b + ? y/l + ? y/m + $ hg status --terse marduc + ? a + ? b + ? y/ + C .hgignore + C x/ + +Modifying already committed files + + $ echo "Hello" >> x/aa + $ echo "World" >> x/bb + $ hg status --terse maurdc + M x/ + ? a + ? b + ? y/ + C .hgignore + +Respecting other flags + + $ hg status --terse marduic --all + M x/aa + M x/bb + ? a + ? b + ? y/l + ? y/m + I x/aa.o + I x/bb.o + I y/l.o + I y/m.o + C .hgignore + $ hg status --terse marduic -a + $ hg status --terse marduic -c + C .hgignore + $ hg status --terse marduic -m + M x/aa + M x/bb + $ hg status --terse marduic -u + ? a + ? b + ? y/l + ? y/m + $ hg status --terse marduc -u + ? a + ? b + ? y/