Patchwork D6369: templatekw: get file_{adds, mods, dels} directly from context (issue4292)

login
register
mail settings
Submitter phabricator
Date May 11, 2019, 7:18 a.m.
Message ID <differential-rev-PHID-DREV-nmyaqrgrqlbfmbvr4v4k-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/40011/
State New
Headers show

Comments

phabricator - May 11, 2019, 7:18 a.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This redefines the template keywords by getting the lists from the
  recently introduced context methods instead of getting them from
  status compared to p1. A mentioned before, these are better defined on
  merge commits. The total number of files from the three lists now
  always add up to the number of files in {files}.
  
  I timed this command:
  
    hg log -r 4.0::5.0 -T '{rev}\n {file_mods}\n {file_adds}\n {file_dels}\n'
  
  It went from 7.6s to 5.6s with this patch. So it's actually faster
  than before.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/templatekw.py
  tests/test-template-keywords.t

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - May 12, 2019, 3:06 a.m.
>   This redefines the template keywords by getting the lists from the
>   recently introduced context methods instead of getting them from
>   status compared to p1. A mentioned before, these are better defined on
>   merge commits. The total number of files from the three lists now
>   always add up to the number of files in {files}.

So this is a BC, and makes `log -T status` output differ from
`status --change REV`. Just pointed out.

I'm 0 on this change. I feel the new definition makes more sense, but
the old one seems also okay and is simpler.
phabricator - May 12, 2019, 3:07 a.m.
yuja added a comment.


  >   This redefines the template keywords by getting the lists from the
  >   recently introduced context methods instead of getting them from
  >   status compared to p1. A mentioned before, these are better defined on
  >   merge commits. The total number of files from the three lists now
  >   always add up to the number of files in {files}.
  
  So this is a BC, and makes `log -T status` output differ from
  `status --change REV`. Just pointed out.
  
  I'm 0 on this change. I feel the new definition makes more sense, but
  the old one seems also okay and is simpler.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers
Cc: yuja, mercurial-devel
via Mercurial-devel - May 12, 2019, 5:14 p.m.
*From: *yuja (Yuya Nishihara) <phabricator@mercurial-scm.org>
*Date: *Sat, May 11, 2019, 20:17
*To: * <mercurial-devel@mercurial-scm.org>

yuja added a comment.
>
>
>   >   This redefines the template keywords by getting the lists from the
>   >   recently introduced context methods instead of getting them from
>   >   status compared to p1. A mentioned before, these are better defined
> on
>   >   merge commits. The total number of files from the three lists now
>   >   always add up to the number of files in {files}.
>
>   So this is a BC, and makes `log -T status` output differ from
>   `status --change REV`. Just pointed out.
>

Depends on whether you consider the bug valid, right? We don't mark bug
fixes as BC. But I agree with still listing this in the "backwards
compatibility" section of the release notes.


>   I'm 0 on this change. I feel the new definition makes more sense, but
>   the old one seems also okay and is simpler.
>

I agree, but it also seems unlikely that anyone would depend on the old
behavior.

BTW, and this is also related to your comments on the previous patch,
`{files}` is probably already inconsistent in the working copy when merging
and after committing the merge. I don't think we should change that. But I
think it means that the number of modified, added, and removed files add up
to the files in that keyword in the working copy, even though they're all
compared to just p1. It's just another example of the working copy status
being weird before committing a merge.


> REPOSITORY
>   rHG Mercurial
>
> REVISION DETAIL
>   https://phab.mercurial-scm.org/D6369
>
> To: martinvonz, #hg-reviewers
> Cc: yuja, mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

Patch

diff --git a/tests/test-template-keywords.t b/tests/test-template-keywords.t
--- a/tests/test-template-keywords.t
+++ b/tests/test-template-keywords.t
@@ -809,9 +809,9 @@ 
   $ hg log -l1 -T '{files}\n'
   a fourth
   $ hg log -l1 -T '{file_mods}\n'
-  third
+  
   $ hg log -l1 -T '{file_adds}\n'
-  b fifth
+  
   $ hg log -l1 -T '{file_dels}\n'
   a fourth
 
diff --git a/mercurial/templatekw.py b/mercurial/templatekw.py
--- a/mercurial/templatekw.py
+++ b/mercurial/templatekw.py
@@ -290,15 +290,12 @@ 
             statmap.update((f, char) for f in files)
     return revcache['filestatusmap']  # {path: statchar}
 
-def _showfilesbystat(context, mapping, name, index):
-    stat = _getfilestatus(context, mapping)
-    files = stat[index]
-    return templateutil.compatfileslist(context, mapping, name, files)
-
 @templatekeyword('file_adds', requires={'ctx', 'revcache'})
 def showfileadds(context, mapping):
     """List of strings. Files added by this changeset."""
-    return _showfilesbystat(context, mapping, 'file_add', 1)
+    ctx = context.resource(mapping, 'ctx')
+    return templateutil.compatfileslist(context, mapping, 'file_add',
+                                        ctx.added())
 
 @templatekeyword('file_copies',
                  requires={'repo', 'ctx', 'cache', 'revcache'})
@@ -337,12 +334,16 @@ 
 @templatekeyword('file_dels', requires={'ctx', 'revcache'})
 def showfiledels(context, mapping):
     """List of strings. Files removed by this changeset."""
-    return _showfilesbystat(context, mapping, 'file_del', 2)
+    ctx = context.resource(mapping, 'ctx')
+    return templateutil.compatfileslist(context, mapping, 'file_del',
+                                        ctx.removed())
 
 @templatekeyword('file_mods', requires={'ctx', 'revcache'})
 def showfilemods(context, mapping):
     """List of strings. Files modified by this changeset."""
-    return _showfilesbystat(context, mapping, 'file_mod', 0)
+    ctx = context.resource(mapping, 'ctx')
+    return templateutil.compatfileslist(context, mapping, 'file_mod',
+                                        ctx.modified())
 
 @templatekeyword('files', requires={'ctx'})
 def showfiles(context, mapping):