This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5961

copies: add test that makes both the merging csets dirty and run w/o error
ClosedPublic

Authored by khanchi97 on Feb 14 2019, 9:15 AM.

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Commits
rHGcbdd2b56d4c3: copies: add test that makes both the merging csets dirty and run w/o error
Summary

This series of patches is to cover a case in fullcopytracing algorithms
where both the merging csets are not descendant of merge base.

In this algorithm we call a merging cset "dirty" if that cset is not the
descendant of merge base. That said, added test in this patch cover case
when both the merging csets are "dirty".

Actually this case of "both dirty" was encountered by Pulkit when he was
working on content-divergence where it is possible that both the csets
are not descendant of merging base.
For reference you can look into: https://phab.mercurial-scm.org/D3896

As this test run fine without any error and correctly traced the copies, I
added this test to make sure that it doesn't break even after I will modify
some code in next patches to fix an error. Next patch adds the tests where
this algorithm throws an error for the same case of "both dirty".

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

khanchi97 created this revision.Feb 14 2019, 9:15 AM
khanchi97 edited the summary of this revision. (Show Details)Feb 20 2019, 12:42 PM
khanchi97 added a comment.Feb 24 2019, 11:42 PM

Ping for review.

martinvonz added a subscriber: martinvonz.Feb 25 2019, 12:26 PM
martinvonz added inline comments.
tests/test-copytrace-heuristics.t
724–728 ↗(On Diff #14090)

Since this does not seem to set experimental.copytrace = heuristics (right?), this is probably not the right file for the test case. I recently added test-copies.t, which may be a better place for this. That file doesn't yet have any tests for mergecopies() (only for pathcopies()), but I was probably going to add that later anyway. Depending on how many tests I would add, I might have put all mergecopies() tests in a separate file, but since there isn't much at this point (only these few that you're adding), it probably makes sense for them to live in test-copies.t.

khanchi97 updated this revision to Diff 14239.Feb 25 2019, 2:32 PM
khanchi97 added a comment.Feb 27 2019, 9:14 AM

@martinvonz I have moved the tests in test-copies.t

martinvonz added a comment.Feb 27 2019, 7:37 PM
In D5961#87961, @khanchi97 wrote:

@martinvonz I have moved the tests in test-copies.t

Thanks. D5963 seems more complicated, so I'll review this series when I can find a larger chunk of time (this week has been extremely busy).

khanchi97 added a comment.Feb 27 2019, 11:55 PM

That's totally fine, reviews it when you have time. Thanks :)

martinvonz added a comment.Mar 2 2019, 6:37 PM

From test-check-code.t:

@@ -15,6 +15,13 @@
   Skipping i18n/polib.py it has no-che?k-code (glob)
   Skipping mercurial/statprof.py it has no-che?k-code (glob)
   Skipping tests/badserverext.py it has no-che?k-code (glob)
+  tests/test-copies.t:515:
+   >
+   trailing whitespace on non-output
+  tests/test-copies.t:516:
+   >   $ hg up .^
+   warning: ^ must be quoted
+  [1]

 @commands in debugcommands.py should be in alphabetical order.
khanchi97 updated this revision to Diff 14308.Mar 3 2019, 1:50 AM
khanchi97 added a comment.Mar 3 2019, 1:55 AM
In D5961#88217, @martinvonz wrote:

From test-check-code.t:

@@ -15,6 +15,13 @@
   Skipping i18n/polib.py it has no-che?k-code (glob)
   Skipping mercurial/statprof.py it has no-che?k-code (glob)
   Skipping tests/badserverext.py it has no-che?k-code (glob)
+  tests/test-copies.t:515:
+   >
+   trailing whitespace on non-output
+  tests/test-copies.t:516:
+   >   $ hg up .^
+   warning: ^ must be quoted
+  [1]
 @commands in debugcommands.py should be in alphabetical order.

So test-check-code.t checks test files too. I wasn't aware of that and didn't run tests on the patches which adds tests only. I have updated the revisions now :)

martinvonz added inline comments.Mar 4 2019, 1:54 AM
tests/test-copies.t
524

Could you change the message here to "added d, modified b"? (Also see my next comment for why I care)

526

The output is very verbose. Does hg l --hidden (using the same alias as tests above) give enough information (given that the commit messages are useful).

584

Can you s/c9241b0f2d5b/3/ here so we don't depend on the nodeids? I'm hoping to add another #testcases case to this file where copy metadata is stored in the changeset and that will necessarily affect the hashes.

588

This also seems like unnecessarily much information. I'd like to at least remove the nodeid for the reason mentioned above. Actually, won't just hg diff --git tip have enough information?

martinvonz added inline comments.Mar 4 2019, 1:57 AM
tests/test-copies.t
495–497

I don't follow this. What does "dirty" mean here? We normally mean that there are changes in the working copy when we say dirty, but that's clearly not what you mean here. The commit message is similarly confusing to me.

khanchi97 added a comment.Mar 4 2019, 2:50 PM

Updating it acc to your suggestions.

tests/test-copies.t
495–497

In this algorithms we call a merging cset "dirty" if it is not a descendant of merge base. And this test cover the case when both the csets are "dirty".

khanchi97 edited the summary of this revision. (Show Details)Mar 4 2019, 2:52 PM
khanchi97 updated this revision to Diff 14346.
khanchi97 added a comment.Mar 4 2019, 3:03 PM

I have updated the commit message too.

khanchi97 added a comment.Mar 8 2019, 11:57 PM

Ping for review.

martinvonz accepted this revision.Mar 9 2019, 2:49 PM
martinvonz added inline comments.
tests/test-copies.t
541

Interesting, I wasn't aware about the --base option. That seems like a useful tool for various workarounds. I'll try to remember it.

This revision is now accepted and ready to land.Mar 9 2019, 2:49 PM
Closed by commit rHGcbdd2b56d4c3: copies: add test that makes both the merging csets dirty and run w/o error (authored by khanchi97). · Explain WhyMar 11 2019, 11:44 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mtests/test-copies.t (61 lines)

Diff 14458

tests/test-copies.t

$ hg debugpathcopies 0 4 $ hg debugpathcopies 0 4
x -> z (filelog !) x -> z (filelog !)
y -> z (compatibility !) y -> z (compatibility !)
$ hg debugpathcopies 1 5 $ hg debugpathcopies 1 5
$ hg debugpathcopies 2 5 $ hg debugpathcopies 2 5
$ hg debugpathcopies 0 5 $ hg debugpathcopies 0 5
x -> z x -> z
Test for a case in fullcopytracing algorithm where both the merging csets are
"dirty"; where a dirty cset means that cset is descendant of merge base. This
test reflect that for this particular case this algorithm correctly find the copies:
martinvonzUnsubmitted
Not Done

I don't follow this. What does "dirty" mean here? We normally mean that there are changes in the working copy when we say dirty, but that's clearly not what you mean here. The commit message is similarly confusing to me.

martinvonz: I don't follow this. What does "dirty" mean here? We normally mean that there are changes in…
khanchi97AuthorUnsubmitted
Not Done

In this algorithms we call a merging cset "dirty" if it is not a descendant of merge base. And this test cover the case when both the csets are "dirty".

khanchi97: In this algorithms we call a merging cset "dirty" if it is not a descendant of merge base. And…
$ cat >> $HGRCPATH << EOF
> [experimental]
> evolution.createmarkers=True
> evolution.allowunstable=True
> EOF
$ newrepo
$ echo a > a
$ hg add a
$ hg ci -m "added a"
$ echo b > b
$ hg add b
$ hg ci -m "added b"
$ hg mv b b1
$ hg ci -m "rename b to b1"
$ hg up ".^"
1 files updated, 0 files merged, 1 files removed, 0 files unresolved
$ echo d > d
$ hg add d
$ hg ci -m "added d"
created new head
$ echo baba >> b
$ hg ci --amend -m "added d, modified b"
martinvonzUnsubmitted
Not Done

Could you change the message here to "added d, modified b"? (Also see my next comment for why I care)

martinvonz: Could you change the message here to "added d, modified b"? (Also see my next comment for why I…
$ hg l --hidden
martinvonzUnsubmitted
Not Done

The output is very verbose. Does hg l --hidden (using the same alias as tests above) give enough information (given that the commit messages are useful).

martinvonz: The output is very verbose. Does `hg l --hidden` (using the same alias as tests above) give…
@ 4 added d, modified b
| b d
| x 3 added d
|/ d
| o 2 rename b to b1
|/ b b1
o 1 added b
| b
o 0 added a
a
Grafting revision 4 on top of revision 2, showing that it respect the rename:
$ hg up 2 -q
$ hg graft -r 4 --base 3 --hidden
martinvonzUnsubmitted
Not Done

Interesting, I wasn't aware about the --base option. That seems like a useful tool for various workarounds. I'll try to remember it.

martinvonz: Interesting, I wasn't aware about the `--base` option. That seems like a useful tool for…
grafting 4:af28412ec03c "added d, modified b" (tip)
merging b1 and b to b1
$ hg l -l1 -p
@ 5 added d, modified b
| b1
~ diff -r 5a4825cc2926 -r 94a2f1a0e8e2 b1
--- a/b1 Thu Jan 01 00:00:00 1970 +0000
+++ b/b1 Thu Jan 01 00:00:00 1970 +0000
@@ -1,1 +1,2 @@
b
+baba
martinvonzUnsubmitted
Not Done

Can you s/c9241b0f2d5b/3/ here so we don't depend on the nodeids? I'm hoping to add another #testcases case to this file where copy metadata is stored in the changeset and that will necessarily affect the hashes.

martinvonz: Can you s/c9241b0f2d5b/3/ here so we don't depend on the nodeids? I'm hoping to add another…
martinvonzUnsubmitted
Not Done

This also seems like unnecessarily much information. I'd like to at least remove the nodeid for the reason mentioned above. Actually, won't just hg diff --git tip have enough information?

martinvonz: This also seems like unnecessarily much information. I'd like to at least remove the nodeid for…