This is an archive of the discontinued Mercurial Phabricator instance.

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

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
Lint Skipped
Unit
Unit Tests Skipped

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

Ping for review.

martinvonz added inline comments.
tests/test-copytrace-heuristics.t
724–728

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

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

@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).

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

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

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 ↗(On Diff #14308)

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

526 ↗(On Diff #14308)

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 ↗(On Diff #14308)

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 ↗(On Diff #14308)

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 ↗(On Diff #14308)

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.

Updating it acc to your suggestions.

tests/test-copies.t
495–497 ↗(On Diff #14308)

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.

I have updated the commit message too.

Ping for review.

martinvonz accepted this revision.Mar 9 2019, 2:49 PM
martinvonz added inline comments.
tests/test-copies.t
541 ↗(On Diff #14346)

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
This revision was automatically updated to reflect the committed changes.