Page MenuHomePhabricator

templatekw: make {file_*} compare to both merge parents (issue4292)
ClosedPublic

Authored by martinvonz on May 11 2019, 3:18 AM.

Details

Summary

This redefines the {file_adds}, {file_dels}, {file_mods} template
keywords by getting the lists from the recently introduced context
methods instead of getting them from status compared to p1. As
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.

Note that the "files:" field in the bazaar test log output was using
"{file_mods}" (not "{files}" as one might think based on the label).

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.May 11 2019, 3:18 AM
yuja added a subscriber: yuja.May 11 2019, 11:07 PM
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.

*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

martinvonz updated this revision to Diff 15082.May 14 2019, 6:58 PM
martinvonz updated this revision to Diff 15153.May 16 2019, 1:41 PM
martinvonz retitled this revision from templatekw: get file_{adds,mods,dels} directly from context (issue4292) to templatekw: make {file_*} compare to both merge parents (issue4292).May 16 2019, 4:53 PM
martinvonz edited the summary of this revision. (Show Details)
martinvonz updated this revision to Diff 15266.May 26 2019, 1:02 AM
yuja added a comment.May 26 2019, 7:31 PM

Test failures:

--- /home/yuya/work/hghacks/mercurial-review/tests/test-convert-bzr-merges.t
+++ /home/yuya/work/hghacks/mercurial-review/tests/test-convert-bzr-merges.t.err
@@ -59,7 +59,7 @@
   $ glog -R source-hg
   o    5@source "(octopus merge fixup)" files+: [], files-: [], files: [renamed]
   |\
-  | o    4@source "Merged branches" files+: [file-branch1 file-branch2 renamed], files-: [rename_me], files: [file]
+  | o    4@source "Merged branches" files+: [file-branch2 renamed], files-: [rename_me], files: []
   | |\
   o---+  3@source-branch2 "Added brach2 file" files+: [file-branch2 renamed], files-: [rename_me], files: []
    / /
@@ -154,7 +154,7 @@
   $ glog -R hg2hg
   @    5@source "(octopus merge fixup)" files+: [], files-: [], files: []
   |\
-  | o    4@source "Merged branches" files+: [file-branch1 file-branch2 renamed], files-: [rename_me], files: [file]
+  | o    4@source "Merged branches" files+: [file-branch2 renamed], files-: [rename_me], files: []
   | |\
   o---+  3@source-branch2 "Added brach2 file" files+: [file-branch2 renamed], files-: [rename_me], files: []
    / /

ERROR: test-convert-bzr-merges.t output changed
!......................................ss..........
--- /home/yuya/work/hghacks/mercurial-review/tests/test-convert-bzr.t
+++ /home/yuya/work/hghacks/mercurial-review/tests/test-convert-bzr.t.err
@@ -147,7 +147,7 @@
   1 Editing b
   0 Merged improve branch
   $ glog -R source-hg
-  o    3@source "Merged improve branch" files+: [], files-: [], files: [b]
+  o    3@source "Merged improve branch" files+: [], files-: [], files: []
   |\
   | o  2@source-improve "Editing b" files+: [], files-: [], files: [b]
   | |

ERROR: test-convert-bzr.t output changed
In D6369#93709, @yuja wrote:

Test failures:

Thanks for letting me know. I've installed bzr now and I've fixed the test cases (the files: field in the output was from file_mods).

martinvonz edited the summary of this revision. (Show Details)Tue, May 28, 12:43 PM

Yuya, are you okay with this patch? My other patches don't depend on it, so I don't care much from that perspective, but it really feels like a bug fix to me.

This revision was automatically updated to reflect the committed changes.