Patchwork D153: test-dirstate-race: replace BROKEN line with explanation of changed output

login
register
mail settings
Submitter phabricator
Date July 20, 2017, 3:33 a.m.
Message ID <differential-rev-PHID-DREV-qx6xbfky6ud26y5uwgx5-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/22531/
State Superseded, archived
Headers show

Comments

phabricator - July 20, 2017, 3:33 a.m.
sid0 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  See the explanation for more.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  tests/test-dirstate-race.t

CHANGE DETAILS




EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: sid0, #hg-reviewers
Cc: mercurial-devel
phabricator - July 20, 2017, 3:38 a.m.
sid0 added a comment.


  (I think this should go into stable as a "test fix")

REPOSITORY
  rHG Mercurial

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

EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: sid0, #hg-reviewers
Cc: mercurial-devel
phabricator - July 20, 2017, 4:14 a.m.
martinvonz added inline comments.

INLINE COMMENTS

> test-dirstate-race.t:223-231
> +The "M b" line in the hg status output used to not exist, but the changes in
> +b63351f6a246 caused the ordering of operations to change such that the dirstate
> +gets written out at a different time. This causes the 'hg status' in the middle
> +to think that "b" is modified. Note that strictly speaking isn't wrong since c4
> +does actually modify "b".
> +
> +Regardless, we're more interested in testing what happens to 'hg status'

The commit that introduced this test case (https://phab.mercurial-scm.org/rHG15e85dded93323d286dc0a758edde1b9dbea5cdd) referred to bug https://bz.mercurial-scm.org/show_bug.cgi?id=5581. The description of that issue (filed by you) finishes with "That last 'hg status' should be empty, but it returns 'M b'.". You changed your mind about what the expected behavior should be?

REPOSITORY
  rHG Mercurial

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

EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: sid0, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - July 20, 2017, 4:42 a.m.
sid0 added inline comments.

INLINE COMMENTS

> martinvonz wrote in test-dirstate-race.t:223-231
> The commit that introduced this test case (https://phab.mercurial-scm.org/rHG15e85dded93323d286dc0a758edde1b9dbea5cdd) referred to bug https://bz.mercurial-scm.org/show_bug.cgi?id=5581. The description of that issue (filed by you) finishes with "That last 'hg status' should be empty, but it returns 'M b'.". You changed your mind about what the expected behavior should be?

The *last* hg status should be empty. This is an intermediate hg status.

REPOSITORY
  rHG Mercurial

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

EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: sid0, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - July 20, 2017, 4:43 a.m.
sid0 added inline comments.

INLINE COMMENTS

> test-dirstate-race.t:244
>  
>    $ hg status

^^ this one should be empty.

REPOSITORY
  rHG Mercurial

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

EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: sid0, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - July 20, 2017, 5:24 a.m.
martinvonz added inline comments.

INLINE COMMENTS

> sid0 wrote in test-dirstate-race.t:223-231
> The *last* hg status should be empty. This is an intermediate hg status.

Oh, the "hg status" is just to cause it to write the dirstate? Would "hg debugrebuilddirstate" work? That doesn't produce any output and it's clearer that that will result in the dirstate getting written ("hg status" doesn't always write it, as I'm sure you know). If that would work, we can delete the whole comment, I think.

REPOSITORY
  rHG Mercurial

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

EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: sid0, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - July 20, 2017, 4:04 p.m.
sid0 added inline comments.

INLINE COMMENTS

> martinvonz wrote in test-dirstate-race.t:223-231
> Oh, the "hg status" is just to cause it to write the dirstate? Would "hg debugrebuilddirstate" work? That doesn't produce any output and it's clearer that that will result in the dirstate getting written ("hg status" doesn't always write it, as I'm sure you know). If that would work, we can delete the whole comment, I think.

I'm afraid not -- debugrebuilddirstate will try to acquire the wlock, which is already held by the rebase.

REPOSITORY
  rHG Mercurial

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

EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: sid0, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - July 20, 2017, 4:33 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> sid0 wrote in test-dirstate-race.t:223-231
> I'm afraid not -- debugrebuilddirstate will try to acquire the wlock, which is already held by the rebase.

Makes sense (for others' info: "hg status" will write the dirstate if the lock can be acquired, but will not wait for the lock, IIRC).

How about piping the result of that "hg status" to /dev/null and adding a comment above it saying that we're only calling it to cause the dirstate to be written if the repo is not locked? Then you can get rid of the comment above.

REPOSITORY
  rHG Mercurial

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

EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: sid0, #hg-reviewers
Cc: martinvonz, mercurial-devel

Patch

diff --git a/tests/test-dirstate-race.t b/tests/test-dirstate-race.t
--- a/tests/test-dirstate-race.t
+++ b/tests/test-dirstate-race.t
@@ -220,7 +220,15 @@ 
   > test.args=$TESTTMP/mergetool-race.sh \$output
   > EOF
 
-BROKEN: the "M b" line should not be there
+The "M b" line in the hg status output used to not exist, but the changes in
+b63351f6a246 caused the ordering of operations to change such that the dirstate
+gets written out at a different time. This causes the 'hg status' in the middle
+to think that "b" is modified. Note that strictly speaking isn't wrong since c4
+does actually modify "b".
+
+Regardless, we're more interested in testing what happens to 'hg status'
+afterwards, both with and without fsmonitor.
+
   $ hg rebase -s . -d 3 --tool test
   rebasing 4:b08445fd6b2a "c4" (tip)
   merging a