| Submitter | phabricator |
|---|---|
| Date | Jan. 25, 2018, 6:52 p.m. |
| Message ID | <differential-rev-PHID-DREV-jecqblhga4xg33kjerhz-req@phab.mercurial-scm.org> |
| Download | mbox | patch |
| Permalink | /patch/27059/ |
| State | Superseded |
| Headers | show |
Comments
joerg.sonnenberger added inline comments. INLINE COMMENTS > cmdutil.py:1627 > + args = [iter(iterable)] * n > + return izip_longest(*args, fillvalue=('', '')) > + The grouping is necessary to avoid computing the full diff in memory with the associated memory use. Turns out splitting it up is even slightly faster, too. The grouper function itself is pretty generic (modulo the hard-coded fillvalue), so putting it in util.py or so would be a good idea. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1938 To: joerg.sonnenberger, #hg-reviewers Cc: mercurial-devel
lothiraldan added inline comments. INLINE COMMENTS > ui.py:887 > + return self._colormode is None > + def canbatchlabelwrites(self, **opts): > + '''check if write calls with labels are batchable''' Style nit, missing new line between the two methods REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1938 To: joerg.sonnenberger, #hg-reviewers Cc: lothiraldan, mercurial-devel
yuja requested changes to this revision. yuja added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > cmdutil.py:1593 > else: > def write(s, **kw): > fp.write(s) We no longer need this `write()` wrapper because no labeling is required if fp is given. > cmdutil.py:1620 > else: > - for chunk, label in patch.diffui(repo, node1, node2, match, > - changes, opts=diffopts, prefix=prefix, > - relroot=relroot, > - hunksfilterfn=hunksfilterfn): > + chunksiter = patch.diffui(repo, node1, node2, match, changes, > + opts=diffopts, prefix=prefix, If we don't care labels, we can simply use `patch.diff()` and `.diffstat()` instead of `patch.diffui()` and `.diffstatui()` respectively. > cmdutil.py:1624 > + > + from itertools import izip_longest > + def grouper(iterable, n): Please move the import line to top. > cmdutil.py:1629 > + > + if fp is None and ui.writenolabels(): > + for block in grouper(chunksiter, 512): `grouper()` could be replaced by `util.chunkbuffer()` over chunks of bytes. if nolabel: chunksiter = patch.diff() util.chunkbuffer(chunksiter) elif batchable: chunksiter = patch.diffui() util.chunkbuffer(ui.label(chunk, label) for chunk, label in chunksiter) > ui.py:884 > + if self._buffers and not opts.get(r'prompt', False): > + if not self._bufferapplylabels: > + return True Perhaps we can simply make _colormode public instead of adding these functions? If ui is buffered, batchable state wouldn't matter as all write()s will be combined into a string. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1938 To: joerg.sonnenberger, #hg-reviewers, yuja Cc: yuja, lothiraldan, mercurial-devel
Patch
diff --git a/mercurial/ui.py b/mercurial/ui.py --- a/mercurial/ui.py +++ b/mercurial/ui.py @@ -878,6 +878,18 @@ return "".join(self._buffers.pop()) + def writenolabels(self, **opts): + '''check if write actually uses the label''' + if self._buffers and not opts.get(r'prompt', False): + if not self._bufferapplylabels: + return True + return self._colormode is None + def canbatchlabelwrites(self, **opts): + '''check if write calls with labels are batchable''' + assert not self.writenolabels() + # Windows color printing is special, see ``write``. + return self._colormode != 'win32' + def write(self, *args, **opts): '''write args to output diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py --- a/mercurial/cmdutil.py +++ b/mercurial/cmdutil.py @@ -1615,14 +1615,21 @@ chunks = patch.diff(repo, node1, node2, match, changes, opts=diffopts, prefix=prefix, relroot=relroot, hunksfilterfn=hunksfilterfn) - for chunk, label in patch.diffstatui(util.iterlines(chunks), - width=width): - write(chunk, label=label) + chunksiter = patch.diffstatui(util.iterlines(chunks), width=width) else: - for chunk, label in patch.diffui(repo, node1, node2, match, - changes, opts=diffopts, prefix=prefix, - relroot=relroot, - hunksfilterfn=hunksfilterfn): + chunksiter = patch.diffui(repo, node1, node2, match, changes, + opts=diffopts, prefix=prefix, + relroot=relroot, hunksfilterfn=hunksfilterfn) + + if fp is None and ui.writenolabels(): + write(*(chunk for chunk, label in chunksiter)) + elif fp is None and ui.canbatchlabelwrites(): + output = [] + for chunk, label in chunksiter: + output.append(ui.label(chunk, label=label)) + write(*output) + else: + for chunk, label in chunksiter: write(chunk, label=label) if listsubrepos: