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
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
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?
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 :-(
+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 :-( >
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.
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.)
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()'