Patchwork [1,of,2,stable] fileset: add tests of generated working copy states

login
register
mail settings
Submitter Yuya Nishihara
Date Jan. 23, 2015, 12:07 p.m.
Message ID <20150123210743.5d777348624bb6d18dc0228f@tcha.org>
Download mbox | patch
Permalink /patch/7537/
State Accepted
Commit 8efb7130a5191c1264831da2233d780ee4843bb7
Headers show

Comments

Yuya Nishihara - Jan. 23, 2015, 12:07 p.m.
On Thu, 22 Jan 2015 14:21:27 -0800, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1421883624 28800
> #      Wed Jan 21 15:40:24 2015 -0800
> # Branch stable
> # Node ID 9e6464357f9dc8f145e30b0af89e3deb1bc5c2dd
> # Parent  a43fdf33a6beb697945a3dbb7253f0436ea278a6
> fileset: add tests of generated working copy states

> +Test revert
> +
> +BROKEN: the files that get undeleted were not modified, they were removed,
> +and content1_content2_missing-tracked was also not modified, it was deleted
> +
> +  $ hg revert 'set:modified()'
> +  reverting content1_content1_content3-tracked
> +  reverting content1_content2_content1-tracked
> +  undeleting content1_content2_content1-untracked
> +  undeleting content1_content2_content2-untracked
> +  reverting content1_content2_content3-tracked
> +  undeleting content1_content2_content3-untracked
> +  reverting content1_content2_missing-tracked
> +  undeleting content1_content2_missing-untracked
> +  reverting missing_content2_content3-tracked
> +
> +BROKEN: only the files that get forgotten are correct
> +
> +  $ hg revert 'set:added()'
> +  forgetting content1_missing_content1-tracked
> +  forgetting content1_missing_content3-tracked
> +  undeleting missing_content2_content2-untracked
> +  undeleting missing_content2_content3-untracked
> +  reverting missing_content2_missing-tracked
> +  undeleting missing_content2_missing-untracked
> +  forgetting missing_missing_content3-tracked
> +
> +  $ hg revert 'set:removed()'
> +  undeleting content1_content1_content1-untracked
> +  undeleting content1_content1_content3-untracked
> +  undeleting content1_content1_missing-untracked
> +
> +  $ hg revert 'set:deleted()'
> +  reverting content1_content1_missing-tracked
> +  forgetting content1_missing_missing-tracked
> +  forgetting missing_missing_missing-tracked
> +
> +  $ hg revert 'set:unknown()'
> +
> +  $ hg revert 'set:clean()'

"revert" tests will have mostly undesired result due of issue4497, so I don't
think it's worth adding them at this point.

http://bz.selenic.com/show_bug.cgi?id=4497

I got many diffs with quick workaround for issue4497.


Regards,
Martin von Zweigbergk - Jan. 23, 2015, 5:34 p.m.
On Fri Jan 23 2015 at 4:08:34 AM Yuya Nishihara <yuya@tcha.org> wrote:

> On Thu, 22 Jan 2015 14:21:27 -0800, Martin von Zweigbergk wrote:
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1421883624 28800
> > #      Wed Jan 21 15:40:24 2015 -0800
> > # Branch stable
> > # Node ID 9e6464357f9dc8f145e30b0af89e3deb1bc5c2dd
> > # Parent  a43fdf33a6beb697945a3dbb7253f0436ea278a6
> > fileset: add tests of generated working copy states
>
> > +Test revert
> > +
> > +BROKEN: the files that get undeleted were not modified, they were
> removed,
> > +and content1_content2_missing-tracked was also not modified, it was
> deleted
> > +
> > +  $ hg revert 'set:modified()'
> > +  reverting content1_content1_content3-tracked
> > +  reverting content1_content2_content1-tracked
> > +  undeleting content1_content2_content1-untracked
> > +  undeleting content1_content2_content2-untracked
> > +  reverting content1_content2_content3-tracked
> > +  undeleting content1_content2_content3-untracked
> > +  reverting content1_content2_missing-tracked
> > +  undeleting content1_content2_missing-untracked
> > +  reverting missing_content2_content3-tracked
> > +
> > +BROKEN: only the files that get forgotten are correct
> > +
> > +  $ hg revert 'set:added()'
> > +  forgetting content1_missing_content1-tracked
> > +  forgetting content1_missing_content3-tracked
> > +  undeleting missing_content2_content2-untracked
> > +  undeleting missing_content2_content3-untracked
> > +  reverting missing_content2_missing-tracked
> > +  undeleting missing_content2_missing-untracked
> > +  forgetting missing_missing_content3-tracked
> > +
> > +  $ hg revert 'set:removed()'
> > +  undeleting content1_content1_content1-untracked
> > +  undeleting content1_content1_content3-untracked
> > +  undeleting content1_content1_missing-untracked
> > +
> > +  $ hg revert 'set:deleted()'
> > +  reverting content1_content1_missing-tracked
> > +  forgetting content1_missing_missing-tracked
> > +  forgetting missing_missing_missing-tracked
> > +
> > +  $ hg revert 'set:unknown()'
> > +
> > +  $ hg revert 'set:clean()'
>
> "revert" tests will have mostly undesired result due of issue4497, so I
> don't
> think it's worth adding them at this point.
>
> http://bz.selenic.com/show_bug.cgi?id=4497


That issue is what made look into this. I started writing these tests to
document what's broken and to make sure the tests caught the broken
behavior. I think you're not alone in your dislike of adding broken tests,
though. I believe mpm also dislikes them. I'll explain the value I see in
adding broken tests to get the debate started:

 * If the tests are written and I don't have time or motivation at the
moment to fix the issue, then at least they're there for the next person
who works on the issue. (In this particular case, my goal was to fix issue
4497, but I don't want to promise I will fix, and if I don't end up fixing
it, it seems like a waste to throw away the tests.)

 * It makes it clear what the change in behavior is as we add a fix. And
that the test case catches it. In fact, when tests are added in the same
changeset as the fix, I often wonder what the output would have been
without the fix. I wonder both because I'm trying to understand the effect
of the patch, and because I want to make sure the test case would have
caught the bug (i.e. that it fails before the fix).

The disadvantages I can see:

* It's easy to leave a stray "BROKEN" marker after fixing the issue. This
is a little annoying. I don't know if some infrastructure could help with
it.

* Maybe what you think is broken is the way it's supposed to work.
Hopefully the reviewers would catch this, but if not, it's just a matter of
dropping the "BROKEN" once people agree that it's working as intended.

* It bloats the repository. I'm hoping that non-binary changes are
considered cheap enough that this is not really an issue.

Martin
Martin von Zweigbergk - Jan. 23, 2015, 5:39 p.m.
On Fri Jan 23 2015 at 4:08:34 AM Yuya Nishihara <yuya@tcha.org> wrote:
>
> I got many diffs with quick workaround for issue4497.
>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -2578,7 +2578,8 @@ def revert(ui, repo, ctx, parents, *pats
>                          return
>                  ui.warn("%s: %s\n" % (m.rel(path), msg))
>
> -            m = scmutil.match(ctx, pats, opts)
> +            # XXX not work with largefiles, can't use 'set:**', etc.
> +            m = scmutil.match(repo[None], pats, opts)
>

That same patch was my first attempt too, but it seems to be a deeper issue
of what context to evaluate the filesets in. Filesets accept a revision so
you can do things like " hg files 'set:added()' -r bead0c7b4f68" and see
which files were added in that revision. However, while most people expect
"hg revert 'set:added()'" to revert added files (i.e. to forget them),
since that is equivalent to "hg revert -r . 'set:added()'" it also makes
sense to say that set:added() should be evaluated in '.'. Perhaps the
answer is, as I think Pierre-Yves suggested, to add a revision parameter to
set:added() and make it default to the working copy, so "hg revert
'set:added()'" still reverts to the parent of the working copy, but the
fileset is evaluated against the working copy. That would mean that "hg
files 'set:added()' -r bead0c7b4f68" instead has to be written "hg files
'set:added(bead0c7b4f68)'". What do you think?
Martin von Zweigbergk - Jan. 23, 2015, 5:47 p.m.
On Fri Jan 23 2015 at 9:39:06 AM Martin von Zweigbergk <
martinvonz@google.com> wrote:

> On Fri Jan 23 2015 at 4:08:34 AM Yuya Nishihara <yuya@tcha.org> wrote:
>
>> I got many diffs with quick workaround for issue4497.
>>
>> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
>> --- a/mercurial/cmdutil.py
>> +++ b/mercurial/cmdutil.py
>> @@ -2578,7 +2578,8 @@ def revert(ui, repo, ctx, parents, *pats
>>                          return
>>                  ui.warn("%s: %s\n" % (m.rel(path), msg))
>>
>> -            m = scmutil.match(ctx, pats, opts)
>> +            # XXX not work with largefiles, can't use 'set:**', etc.
>> +            m = scmutil.match(repo[None], pats, opts)
>>
>
> That same patch was my first attempt too, but it seems to be a deeper
> issue of what context to evaluate the filesets in. Filesets accept a
> revision so you can do things like " hg files 'set:added()' -r
> bead0c7b4f68" and see which files were added in that revision. However,
> while most people expect "hg revert 'set:added()'" to revert added files
> (i.e. to forget them), since that is equivalent to "hg revert -r .
> 'set:added()'" it also makes sense to say that set:added() should be
> evaluated in '.'. Perhaps the answer is, as I think Pierre-Yves suggested,
> to add a revision parameter to set:added() and make it default to the
> working copy, so "hg revert 'set:added()'" still reverts to the parent of
> the working copy, but the fileset is evaluated against the working copy.
> That would mean that "hg files 'set:added()' -r bead0c7b4f68" instead has
> to be written "hg files 'set:added(bead0c7b4f68)'". What do you think?
>
>
Wait, that "hg files 'set:added(bead0c7b4f68)'" should have been "hg files
'set:added(bead0c7b4f68)' -r bead0c7b4f68" in order to list the files in
that revision that were added in that revision, not the files in the
working copy (?) that were added in that revision. This is confusing :-(
Martin von Zweigbergk - Jan. 23, 2015, 5:55 p.m.
+Pierre-Yves & Mathias (others: sorry about the spam)

On Fri Jan 23 2015 at 9:47:01 AM Martin von Zweigbergk <
martinvonz@google.com> wrote:

> On Fri Jan 23 2015 at 9:39:06 AM Martin von Zweigbergk <
> martinvonz@google.com> wrote:
>
>> On Fri Jan 23 2015 at 4:08:34 AM Yuya Nishihara <yuya@tcha.org> wrote:
>>
>>> I got many diffs with quick workaround for issue4497.
>>>
>>> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
>>> --- a/mercurial/cmdutil.py
>>> +++ b/mercurial/cmdutil.py
>>> @@ -2578,7 +2578,8 @@ def revert(ui, repo, ctx, parents, *pats
>>>                          return
>>>                  ui.warn("%s: %s\n" % (m.rel(path), msg))
>>>
>>> -            m = scmutil.match(ctx, pats, opts)
>>> +            # XXX not work with largefiles, can't use 'set:**', etc.
>>> +            m = scmutil.match(repo[None], pats, opts)
>>>
>>
>> That same patch was my first attempt too, but it seems to be a deeper
>> issue of what context to evaluate the filesets in. Filesets accept a
>> revision so you can do things like " hg files 'set:added()' -r
>> bead0c7b4f68" and see which files were added in that revision. However,
>> while most people expect "hg revert 'set:added()'" to revert added files
>> (i.e. to forget them), since that is equivalent to "hg revert -r .
>> 'set:added()'" it also makes sense to say that set:added() should be
>> evaluated in '.'. Perhaps the answer is, as I think Pierre-Yves suggested,
>> to add a revision parameter to set:added() and make it default to the
>> working copy, so "hg revert 'set:added()'" still reverts to the parent of
>> the working copy, but the fileset is evaluated against the working copy.
>> That would mean that "hg files 'set:added()' -r bead0c7b4f68" instead has
>> to be written "hg files 'set:added(bead0c7b4f68)'". What do you think?
>>
>>
> Wait, that "hg files 'set:added(bead0c7b4f68)'" should have been "hg files
> 'set:added(bead0c7b4f68)' -r bead0c7b4f68" in order to list the files in
> that revision that were added in that revision, not the files in the
> working copy (?) that were added in that revision. This is confusing :-(
>
Matt Mackall - Jan. 23, 2015, 10:50 p.m.
On Fri, 2015-01-23 at 17:34 +0000, Martin von Zweigbergk wrote:
> The disadvantages I can see:
> 
> * It's easy to leave a stray "BROKEN" marker after fixing the issue. This
> is a little annoying. I don't know if some infrastructure could help with
> it.

Actually, I think the biggest issue is that when you fix something
(possibly as a side-effect), you now have false-positives from the test
suite to dig through.

I'm at -0 on this sort of thing. If people are actually working through
issues in the area, I'm lately inclined to take documented-broken tests.
But I don't really like to take them on their own unless I think
someone's going to follow up soon.

(And I'm definitely -1 on tests that actually FAIL, rather than document
incorrect behavior.)

Perhaps we can augment the test suite to be more explicit here. For
instance, we could flag entire blocks including descriptions as
known-broken:

---
!This should do something:
!
! $ hg something
! abort: huh?
! [255]
!
---

..and then it'd be really obvious from the test output when you'd hit
this, and also obvious from diffs if you hadn't fixed up the whole
block.
Martin von Zweigbergk - Jan. 23, 2015, 11 p.m.
On Fri Jan 23 2015 at 2:50:24 PM Matt Mackall <mpm@selenic.com> wrote:

> On Fri, 2015-01-23 at 17:34 +0000, Martin von Zweigbergk wrote:
> > The disadvantages I can see:
> >
> > * It's easy to leave a stray "BROKEN" marker after fixing the issue. This
> > is a little annoying. I don't know if some infrastructure could help with
> > it.
>
> Actually, I think the biggest issue is that when you fix something
> (possibly as a side-effect), you now have false-positives from the test
> suite to dig through.
>
> I'm at -0 on this sort of thing. If people are actually working through
> issues in the area, I'm lately inclined to take documented-broken tests.
> But I don't really like to take them on their own unless I think
> someone's going to follow up soon.
>
> (And I'm definitely -1 on tests that actually FAIL, rather than document
> incorrect behavior.)
>
> Perhaps we can augment the test suite to be more explicit here. For
> instance, we could flag entire blocks including descriptions as
> known-broken:
>
> ---
> !This should do something:
> !
> ! $ hg something
> ! abort: huh?
> ! [255]
> !
> ---
>
> ..and then it'd be really obvious from the test output when you'd hit
> this, and also obvious from diffs if you hadn't fixed up the whole
> block.
>

+1 to making it mark a whole block. I like the syntax too. (And I realize
it's probably on me to add support in the test runner.)
Yuya Nishihara - Jan. 24, 2015, 3:26 a.m.
On Fri, 23 Jan 2015 17:55:40 +0000, Martin von Zweigbergk wrote:
> > On Fri Jan 23 2015 at 9:39:06 AM Martin von Zweigbergk <
> > martinvonz@google.com> wrote:
> >> On Fri Jan 23 2015 at 4:08:34 AM Yuya Nishihara <yuya@tcha.org> wrote:
> >>
> >>> I got many diffs with quick workaround for issue4497.
> >>>
> >>> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> >>> --- a/mercurial/cmdutil.py
> >>> +++ b/mercurial/cmdutil.py
> >>> @@ -2578,7 +2578,8 @@ def revert(ui, repo, ctx, parents, *pats
> >>>                          return
> >>>                  ui.warn("%s: %s\n" % (m.rel(path), msg))
> >>>
> >>> -            m = scmutil.match(ctx, pats, opts)
> >>> +            # XXX not work with largefiles, can't use 'set:**', etc.
> >>> +            m = scmutil.match(repo[None], pats, opts)
> >>>
> >>
> >> That same patch was my first attempt too, but it seems to be a deeper
> >> issue of what context to evaluate the filesets in. Filesets accept a
> >> revision so you can do things like " hg files 'set:added()' -r
> >> bead0c7b4f68" and see which files were added in that revision. However,
> >> while most people expect "hg revert 'set:added()'" to revert added files
> >> (i.e. to forget them), since that is equivalent to "hg revert -r .
> >> 'set:added()'" it also makes sense to say that set:added() should be
> >> evaluated in '.'. Perhaps the answer is, as I think Pierre-Yves suggested,
> >> to add a revision parameter to set:added() and make it default to the
> >> working copy, so "hg revert 'set:added()'" still reverts to the parent of
> >> the working copy, but the fileset is evaluated against the working copy.
> >> That would mean that "hg files 'set:added()' -r bead0c7b4f68" instead has
> >> to be written "hg files 'set:added(bead0c7b4f68)'". What do you think?
> >>
> > Wait, that "hg files 'set:added(bead0c7b4f68)'" should have been "hg files
> > 'set:added(bead0c7b4f68)' -r bead0c7b4f68" in order to list the files in
> > that revision that were added in that revision, not the files in the
> > working copy (?) that were added in that revision. This is confusing :-(

I'm thinking about a function that switches matchctx:

  set:rev(revspec, fileset)
  # and set:wdir(fileset) aliased to rev(wdir, fileset),
  # and set:p1(fileset) aliased to rev(., fileset) ?

  set:added()               # will be evaluated in the provided ctx
  set:wdir(added())         # select files added to wctx
  set:rev(12345, size(1k))  # select files that were 1kB at rev 12345

We have to specify rev() because "revert" involves two revisions.

  hg revert 'set:wdir(added())'  # to revert files added to wctx
  hg revert 'set:p1(added())'    # to revert files added at p1

Regards,

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2578,7 +2578,8 @@  def revert(ui, repo, ctx, parents, *pats
                         return
                 ui.warn("%s: %s\n" % (m.rel(path), msg))
 
-            m = scmutil.match(ctx, pats, opts)
+            # XXX not work with largefiles, can't use 'set:**', etc.
+            m = scmutil.match(repo[None], pats, opts)
             m.bad = badfn
             for abs in ctx.walk(m):
                 if abs not in names:
diff --git a/tests/test-fileset-generated.t b/tests/test-fileset-generated.t
--- a/tests/test-fileset-generated.t
+++ b/tests/test-fileset-generated.t
@@ -130,12 +130,7 @@  and content1_content2_missing-tracked wa
   $ hg revert 'set:modified()'
   reverting content1_content1_content3-tracked
   reverting content1_content2_content1-tracked
-  undeleting content1_content2_content1-untracked
-  undeleting content1_content2_content2-untracked
   reverting content1_content2_content3-tracked
-  undeleting content1_content2_content3-untracked
-  reverting content1_content2_missing-tracked
-  undeleting content1_content2_missing-untracked
   reverting missing_content2_content3-tracked
 
 BROKEN: only the files that get forgotten are correct
@@ -143,20 +138,25 @@  BROKEN: only the files that get forgotte
   $ hg revert 'set:added()'
   forgetting content1_missing_content1-tracked
   forgetting content1_missing_content3-tracked
-  undeleting missing_content2_content2-untracked
-  undeleting missing_content2_content3-untracked
-  reverting missing_content2_missing-tracked
-  undeleting missing_content2_missing-untracked
   forgetting missing_missing_content3-tracked
 
   $ hg revert 'set:removed()'
   undeleting content1_content1_content1-untracked
   undeleting content1_content1_content3-untracked
   undeleting content1_content1_missing-untracked
+  undeleting content1_content2_content1-untracked
+  undeleting content1_content2_content2-untracked
+  undeleting content1_content2_content3-untracked
+  undeleting content1_content2_missing-untracked
+  undeleting missing_content2_content2-untracked
+  undeleting missing_content2_content3-untracked
+  undeleting missing_content2_missing-untracked
 
   $ hg revert 'set:deleted()'
   reverting content1_content1_missing-tracked
+  reverting content1_content2_missing-tracked
   forgetting content1_missing_missing-tracked
+  reverting missing_content2_missing-tracked
   forgetting missing_missing_missing-tracked
 
   $ hg revert 'set:unknown()'