| Submitter | Martin von Zweigbergk |
|---|---|
| Date | March 24, 2015, 6:16 p.m. |
| Message ID | <23d12cf3fdddf6b57639.1427221001@martinvonz.mtv.corp.google.com> |
| Download | mbox | patch |
| Permalink | /patch/8237/ |
| State | Accepted |
| Headers | show |
Comments
On Tue, 2015-03-24 at 11:16 -0700, Martin von Zweigbergk wrote: > # HG changeset patch > # User Martin von Zweigbergk <martinvonz@google.com> > # Date 1427177091 25200 > # Mon Mar 23 23:04:51 2015 -0700 > # Node ID 23d12cf3fdddf6b57639e6120e96d754e1427a8e > # Parent a390c931dda089c51b858bb360f12d4e80bddba9 > revert: evaluate filesets against working directory These are queued for default, thanks. This one I tagged with issue4497.
On Tue, 24 Mar 2015 14:16:41 -0400, Martin von Zweigbergk <martinvonz@google.com> wrote: > # HG changeset patch > # User Martin von Zweigbergk <martinvonz@google.com> > # Date 1427177091 25200 > # Mon Mar 23 23:04:51 2015 -0700 > # Node ID 23d12cf3fdddf6b57639e6120e96d754e1427a8e > # Parent a390c931dda089c51b858bb360f12d4e80bddba9 > revert: evaluate filesets against working directory > > As the failing revert tests in test-fileset-generated.t show, > > Revert currently creates one matcher for matching files in the working > copy and another matcher for matching files in the target > revision. The two matchers are created with different contexts, which > means filesets are evaluated differently. Then the union of the sets > of files matching the matchers in the two contexts are reverted. It > doesn't seem to make sense to use two different matchers; only the > context they're applied to should be different. > > It seems very likely that the user wants the filesets to be evaluated > against the working directory, which the tests > test-fileset-generated.t also assume, so let's make it so. > > I willingly admit that the largefiles code was modified by trial and > error (according to tests). > ... > diff -r a390c931dda0 -r 23d12cf3fddd mercurial/cmdutil.py > --- a/mercurial/cmdutil.py Tue Mar 24 10:27:56 2015 -0700 > +++ b/mercurial/cmdutil.py Mon Mar 23 23:04:51 2015 -0700 > @@ -2813,7 +2813,6 @@ > return > ui.warn("%s: %s\n" % (m.rel(path), msg)) > - m = scmutil.match(ctx, pats, opts) > m.bad = badfn > for abs in ctx.walk(m): > if abs not in names: The revert code baffles me, so I'll just ask instead of breaking something... There's a line at the bottom of cmdutil.revert() that creates this same matcher, in order to figure out subrepos that need reverting. Shouldn't that also use the working directory matcher created at the top? (i.e. the line at the bottom should be dropped too) I was going to add path/into/subrepo style reverting, which requires passing a matcher instead of patterns, and I wasn't sure how passing two matchers would be received. That problem goes away if the line at the bottom gets dropped. I'll see if I can add tests for the largefiles code, since I'm not real sure what those changes do. --Matt
On Wed, Mar 25, 2015 at 5:03 PM Matt Harbison <mharbison72@gmail.com> wrote: > On Tue, 24 Mar 2015 14:16:41 -0400, Martin von Zweigbergk > <martinvonz@google.com> wrote: > > > # HG changeset patch > > # User Martin von Zweigbergk <martinvonz@google.com> > > # Date 1427177091 25200 > > # Mon Mar 23 23:04:51 2015 -0700 > > # Node ID 23d12cf3fdddf6b57639e6120e96d754e1427a8e > > # Parent a390c931dda089c51b858bb360f12d4e80bddba9 > > revert: evaluate filesets against working directory > > > > As the failing revert tests in test-fileset-generated.t show, > > > > Revert currently creates one matcher for matching files in the working > > copy and another matcher for matching files in the target > > revision. The two matchers are created with different contexts, which > > means filesets are evaluated differently. Then the union of the sets > > of files matching the matchers in the two contexts are reverted. It > > doesn't seem to make sense to use two different matchers; only the > > context they're applied to should be different. > > > > It seems very likely that the user wants the filesets to be evaluated > > against the working directory, which the tests > > test-fileset-generated.t also assume, so let's make it so. > > > > I willingly admit that the largefiles code was modified by trial and > > error (according to tests). > > > ... > > > diff -r a390c931dda0 -r 23d12cf3fddd mercurial/cmdutil.py > > --- a/mercurial/cmdutil.py Tue Mar 24 10:27:56 2015 -0700 > > +++ b/mercurial/cmdutil.py Mon Mar 23 23:04:51 2015 -0700 > > @@ -2813,7 +2813,6 @@ > > return > > ui.warn("%s: %s\n" % (m.rel(path), msg)) > > - m = scmutil.match(ctx, pats, opts) > > m.bad = badfn > > for abs in ctx.walk(m): > > if abs not in names: > > The revert code baffles me, so I'll just ask instead of breaking > something... There's a line at the bottom of cmdutil.revert() that creates > this same matcher, in order to figure out subrepos that need reverting. > Shouldn't that also use the working directory matcher created at the top? > (i.e. the line at the bottom should be dropped too) > That makes sense to me. I didn't even notice that line. (Note that you can't exactly drop the line, since the 'm' matcher gets overwritten above it. We should perhaps create a new variable for the matchfiles() call instead of overwriting the existing matcher.) > > I was going to add path/into/subrepo style reverting, which requires > passing a matcher instead of patterns, and I wasn't sure how passing two > matchers would be received. That problem goes away if the line at the > bottom gets dropped. > > I'll see if I can add tests for the largefiles code, since I'm not real > sure what those changes do. > > --Matt >
On Wed, 25 Mar 2015 20:10:21 -0400, Martin von Zweigbergk <martinvonz@google.com> wrote: > On Wed, Mar 25, 2015 at 5:03 PM Matt Harbison <mharbison72@gmail.com> > wrote: > >> On Tue, 24 Mar 2015 14:16:41 -0400, Martin von Zweigbergk >> <martinvonz@google.com> wrote: >> >> > # HG changeset patch >> > # User Martin von Zweigbergk <martinvonz@google.com> >> > # Date 1427177091 25200 >> > # Mon Mar 23 23:04:51 2015 -0700 >> > # Node ID 23d12cf3fdddf6b57639e6120e96d754e1427a8e >> > # Parent a390c931dda089c51b858bb360f12d4e80bddba9 >> > revert: evaluate filesets against working directory >> > >> > As the failing revert tests in test-fileset-generated.t show, >> > >> > Revert currently creates one matcher for matching files in the working >> > copy and another matcher for matching files in the target >> > revision. The two matchers are created with different contexts, which >> > means filesets are evaluated differently. Then the union of the sets >> > of files matching the matchers in the two contexts are reverted. It >> > doesn't seem to make sense to use two different matchers; only the >> > context they're applied to should be different. >> > >> > It seems very likely that the user wants the filesets to be evaluated >> > against the working directory, which the tests >> > test-fileset-generated.t also assume, so let's make it so. >> > >> > I willingly admit that the largefiles code was modified by trial and >> > error (according to tests). >> > >> ... >> >> > diff -r a390c931dda0 -r 23d12cf3fddd mercurial/cmdutil.py >> > --- a/mercurial/cmdutil.py Tue Mar 24 10:27:56 2015 -0700 >> > +++ b/mercurial/cmdutil.py Mon Mar 23 23:04:51 2015 -0700 >> > @@ -2813,7 +2813,6 @@ >> > return >> > ui.warn("%s: %s\n" % (m.rel(path), msg)) >> > - m = scmutil.match(ctx, pats, opts) >> > m.bad = badfn >> > for abs in ctx.walk(m): >> > if abs not in names: >> >> The revert code baffles me, so I'll just ask instead of breaking >> something... There's a line at the bottom of cmdutil.revert() that >> creates >> this same matcher, in order to figure out subrepos that need reverting. >> Shouldn't that also use the working directory matcher created at the >> top? >> (i.e. the line at the bottom should be dropped too) >> > > That makes sense to me. I didn't even notice that line. (Note that you > can't exactly drop the line, since the 'm' matcher gets overwritten above > it. We should perhaps create a new variable for the matchfiles() call > instead of overwriting the existing matcher.) Good, point. I missed that. I'll take a run at this (though I'm not sure how hard testing it will be). >> >> I was going to add path/into/subrepo style reverting, which requires >> passing a matcher instead of patterns, and I wasn't sure how passing two >> matchers would be received. That problem goes away if the line at the >> bottom gets dropped. >> >> I'll see if I can add tests for the largefiles code, since I'm not real >> sure what those changes do. >> >> --Matt
Patch
diff -r a390c931dda0 -r 23d12cf3fddd hgext/largefiles/overrides.py --- a/hgext/largefiles/overrides.py Tue Mar 24 10:27:56 2015 -0700 +++ b/hgext/largefiles/overrides.py Mon Mar 23 23:04:51 2015 -0700 @@ -765,7 +765,7 @@ def tostandin(f): standin = lfutil.standin(f) - if standin in mctx: + if standin in ctx or standin in mctx: return standin elif standin in repo[None] or lfdirstate[f] == 'r': return None @@ -777,7 +777,7 @@ def matchfn(f): if lfutil.isstandin(f): return (origmatchfn(lfutil.splitstandin(f)) and - (f in repo[None] or f in mctx)) + (f in ctx or f in mctx)) return origmatchfn(f) m.matchfn = matchfn return m diff -r a390c931dda0 -r 23d12cf3fddd mercurial/cmdutil.py --- a/mercurial/cmdutil.py Tue Mar 24 10:27:56 2015 -0700 +++ b/mercurial/cmdutil.py Mon Mar 23 23:04:51 2015 -0700 @@ -2813,7 +2813,6 @@ return ui.warn("%s: %s\n" % (m.rel(path), msg)) - m = scmutil.match(ctx, pats, opts) m.bad = badfn for abs in ctx.walk(m): if abs not in names: diff -r a390c931dda0 -r 23d12cf3fddd tests/test-fileset-generated.t --- a/tests/test-fileset-generated.t Tue Mar 24 10:27:56 2015 -0700 +++ b/tests/test-fileset-generated.t Mon Mar 23 23:04:51 2015 -0700 @@ -141,39 +141,34 @@ 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 + 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()'