Patchwork [V2] forget: add --dry-run mode

login
register
mail settings
Submitter sushil khanchi
Date March 11, 2018, 9:49 a.m.
Message ID <a1be8989c0158abc69eb.1520761766@workspace>
Download mbox | patch
Permalink /patch/29302/
State Superseded
Headers show

Comments

sushil khanchi - March 11, 2018, 9:49 a.m.
# HG changeset patch
# User Sushil khanchi <sushilkhanchi97@gmail.com>
# Date 1520665399 -19800
#      Sat Mar 10 12:33:19 2018 +0530
# Node ID a1be8989c0158abc69ebd97ca8a0cc7dc3801be9
# Parent  4c71a26a4009d88590c9ae3d64a5912fd556d82e
forget: add --dry-run mode
Yuya Nishihara - March 11, 2018, 10:20 a.m.
On Sun, 11 Mar 2018 15:19:26 +0530, Sushil khanchi wrote:
> # HG changeset patch
> # User Sushil khanchi <sushilkhanchi97@gmail.com>
> # Date 1520665399 -19800
> #      Sat Mar 10 12:33:19 2018 +0530
> # Node ID a1be8989c0158abc69ebd97ca8a0cc7dc3801be9
> # Parent  4c71a26a4009d88590c9ae3d64a5912fd556d82e
> forget: add --dry-run mode

Can you add some tests?

> -def forget(ui, repo, match, prefix, explicitonly):
> +def forget(ui, repo, match, prefix, explicitonly, **opts):

I slightly prefer an explicit parameter, 'opts' or 'dryrun' instead of **opts.
That will show you we'll have to propagate the option to sub.forget().

> -    rejected = wctx.forget(forget, prefix)
> -    bad.extend(f for f in rejected if f in match.files())
> -    forgot.extend(f for f in forget if f not in rejected)
> +    if not opts.get(r'dry_run'):
> +        rejected = wctx.forget(forget, prefix)
> +        bad.extend(f for f in rejected if f in match.files())
> +        forgot.extend(f for f in forget if f not in rejected)
>      return bad, forgot
>  
>  def files(ui, ctx, m, fm, fmt, subrepos):
> diff -r 4c71a26a4009 -r a1be8989c015 mercurial/commands.py
> --- a/mercurial/commands.py	Sun Mar 04 21:16:36 2018 -0500
> +++ b/mercurial/commands.py	Sat Mar 10 12:33:19 2018 +0530
> @@ -2036,7 +2036,11 @@
>      with ui.formatter('files', opts) as fm:
>          return cmdutil.files(ui, ctx, m, fm, fmt, opts.get('subrepos'))
>  
> -@command('^forget', walkopts, _('[OPTION]... FILE...'), inferrepo=True)
> +@command(
> +    '^forget',
> +    [('', 'dry-run', None, _('only print output'))]
> +    + walkopts,

The constant 'dryrunopts' can be used instead.
Matt Harbison - March 12, 2018, 10:27 p.m.
On Sun, 11 Mar 2018 05:49:26 -0400, Sushil khanchi  
<sushilkhanchi97@gmail.com> wrote:

> # HG changeset patch
> # User Sushil khanchi <sushilkhanchi97@gmail.com>
> # Date 1520665399 -19800
> #      Sat Mar 10 12:33:19 2018 +0530
> # Node ID a1be8989c0158abc69ebd97ca8a0cc7dc3801be9
> # Parent  4c71a26a4009d88590c9ae3d64a5912fd556d82e
> forget: add --dry-run mode
>
> diff -r 4c71a26a4009 -r a1be8989c015 mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py	Sun Mar 04 21:16:36 2018 -0500
> +++ b/mercurial/cmdutil.py	Sat Mar 10 12:33:19 2018 +0530
> @@ -1996,7 +1996,7 @@
>          for subpath in ctx.substate:
>              ctx.sub(subpath).addwebdirpath(serverpath, webconf)
> -def forget(ui, repo, match, prefix, explicitonly):
> +def forget(ui, repo, match, prefix, explicitonly, **opts):
>      join = lambda f: os.path.join(prefix, f)
>      bad = []
>      badfn = lambda x, y: bad.append(x) or match.bad(x, y)
> @@ -2039,9 +2039,10 @@
>          if ui.verbose or not match.exact(f):
>              ui.status(_('removing %s\n') % match.rel(f))
> -    rejected = wctx.forget(forget, prefix)

Shouldn't --dry-run be passed into wctx.forget() too?  Then the warning  
about bad paths there will be emitted, and you won't have to  
conditionalize the following lines here.  That in turn won't affect the  
exit code.

> -    bad.extend(f for f in rejected if f in match.files())
> -    forgot.extend(f for f in forget if f not in rejected)
> +    if not opts.get(r'dry_run'):
> +        rejected = wctx.forget(forget, prefix)
> +        bad.extend(f for f in rejected if f in match.files())
> +        forgot.extend(f for f in forget if f not in rejected)
>      return bad, forgot
> def files(ui, ctx, m, fm, fmt, subrepos):
> diff -r 4c71a26a4009 -r a1be8989c015 mercurial/commands.py
> --- a/mercurial/commands.py	Sun Mar 04 21:16:36 2018 -0500
> +++ b/mercurial/commands.py	Sat Mar 10 12:33:19 2018 +0530
> @@ -2036,7 +2036,11 @@
>      with ui.formatter('files', opts) as fm:
>          return cmdutil.files(ui, ctx, m, fm, fmt, opts.get('subrepos'))
> -@command('^forget', walkopts, _('[OPTION]... FILE...'), inferrepo=True)
> +@command(
> +    '^forget',
> +    [('', 'dry-run', None, _('only print output'))]
> +    + walkopts,
> +    _('[OPTION]... FILE...'), inferrepo=True)
>  def forget(ui, repo, *pats, **opts):
>      """forget the specified files on the next commit
> @@ -2071,7 +2075,7 @@
>          raise error.Abort(_('no files specified'))
>     m = scmutil.match(repo[None], pats, opts)
> -    rejected = cmdutil.forget(ui, repo, m, prefix="",  
> explicitonly=False)[0]
> +    rejected = cmdutil.forget(ui, repo, m, prefix="",  
> explicitonly=False, **opts)[0]
>      return rejected and 1 or 0
> @command(
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - March 13, 2018, 2:25 p.m.
On Mon, 12 Mar 2018 18:27:13 -0400, Matt Harbison wrote:
> On Sun, 11 Mar 2018 05:49:26 -0400, Sushil khanchi  
> <sushilkhanchi97@gmail.com> wrote:
> 
> > # HG changeset patch
> > # User Sushil khanchi <sushilkhanchi97@gmail.com>
> > # Date 1520665399 -19800
> > #      Sat Mar 10 12:33:19 2018 +0530
> > # Node ID a1be8989c0158abc69ebd97ca8a0cc7dc3801be9
> > # Parent  4c71a26a4009d88590c9ae3d64a5912fd556d82e
> > forget: add --dry-run mode
> >
> > diff -r 4c71a26a4009 -r a1be8989c015 mercurial/cmdutil.py
> > --- a/mercurial/cmdutil.py	Sun Mar 04 21:16:36 2018 -0500
> > +++ b/mercurial/cmdutil.py	Sat Mar 10 12:33:19 2018 +0530
> > @@ -1996,7 +1996,7 @@
> >          for subpath in ctx.substate:
> >              ctx.sub(subpath).addwebdirpath(serverpath, webconf)
> > -def forget(ui, repo, match, prefix, explicitonly):
> > +def forget(ui, repo, match, prefix, explicitonly, **opts):
> >      join = lambda f: os.path.join(prefix, f)
> >      bad = []
> >      badfn = lambda x, y: bad.append(x) or match.bad(x, y)
> > @@ -2039,9 +2039,10 @@
> >          if ui.verbose or not match.exact(f):
> >              ui.status(_('removing %s\n') % match.rel(f))
> > -    rejected = wctx.forget(forget, prefix)
> 
> Shouldn't --dry-run be passed into wctx.forget() too?  Then the warning  
> about bad paths there will be emitted, and you won't have to  
> conditionalize the following lines here.  That in turn won't affect the  
> exit code.

I slightly prefer not to pass --dry-run deep into the context layer
because it's a command-line business. Just my two cents.
sushil khanchi - March 13, 2018, 9:30 p.m.
Shall I remove --dry-run from context layer now?

On Tue, Mar 13, 2018 at 7:55 PM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Mon, 12 Mar 2018 18:27:13 -0400, Matt Harbison wrote:
> > On Sun, 11 Mar 2018 05:49:26 -0400, Sushil khanchi
> > <sushilkhanchi97@gmail.com> wrote:
> >
> > > # HG changeset patch
> > > # User Sushil khanchi <sushilkhanchi97@gmail.com>
> > > # Date 1520665399 -19800
> > > #      Sat Mar 10 12:33:19 2018 +0530
> > > # Node ID a1be8989c0158abc69ebd97ca8a0cc7dc3801be9
> > > # Parent  4c71a26a4009d88590c9ae3d64a5912fd556d82e
> > > forget: add --dry-run mode
> > >
> > > diff -r 4c71a26a4009 -r a1be8989c015 mercurial/cmdutil.py
> > > --- a/mercurial/cmdutil.py  Sun Mar 04 21:16:36 2018 -0500
> > > +++ b/mercurial/cmdutil.py  Sat Mar 10 12:33:19 2018 +0530
> > > @@ -1996,7 +1996,7 @@
> > >          for subpath in ctx.substate:
> > >              ctx.sub(subpath).addwebdirpath(serverpath, webconf)
> > > -def forget(ui, repo, match, prefix, explicitonly):
> > > +def forget(ui, repo, match, prefix, explicitonly, **opts):
> > >      join = lambda f: os.path.join(prefix, f)
> > >      bad = []
> > >      badfn = lambda x, y: bad.append(x) or match.bad(x, y)
> > > @@ -2039,9 +2039,10 @@
> > >          if ui.verbose or not match.exact(f):
> > >              ui.status(_('removing %s\n') % match.rel(f))
> > > -    rejected = wctx.forget(forget, prefix)
> >
> > Shouldn't --dry-run be passed into wctx.forget() too?  Then the warning
> > about bad paths there will be emitted, and you won't have to
> > conditionalize the following lines here.  That in turn won't affect the
> > exit code.
>
> I slightly prefer not to pass --dry-run deep into the context layer
> because it's a command-line business. Just my two cents.
>
Matt Harbison - March 13, 2018, 10:51 p.m.
On Tue, 13 Mar 2018 17:30:55 -0400, sushil khanchi  
<sushilkhanchi97@gmail.com> wrote:

> Shall I remove --dry-run from context layer now?

It looks like cmdutil.add() and scmutil.addremove() don't pass it along to  
wctx either, so I guess it's OK to drop for consistency.

But the fact that it doesn't seems like a bug- wctx.add() prints out  
various warnings about portable file names, sizes, etc.  I'd think that  
the whole point of a dry run is to test against that.  Maybe the solution  
to that is to have a mock wctx somehow that simply doesn't flush dirstate  
to disk?
Yuya Nishihara - March 14, 2018, 1:39 p.m.
On Tue, 13 Mar 2018 18:51:33 -0400, Matt Harbison wrote:
> On Tue, 13 Mar 2018 17:30:55 -0400, sushil khanchi  
> <sushilkhanchi97@gmail.com> wrote:
> 
> > Shall I remove --dry-run from context layer now?
> 
> It looks like cmdutil.add() and scmutil.addremove() don't pass it along to  
> wctx either, so I guess it's OK to drop for consistency.
> 
> But the fact that it doesn't seems like a bug- wctx.add() prints out  
> various warnings about portable file names, sizes, etc.  I'd think that  
> the whole point of a dry run is to test against that.  Maybe the solution  
> to that is to have a mock wctx somehow that simply doesn't flush dirstate  
> to disk?

That could be, but I think the check for bad paths is actually done by
cmdutil.forget(). Because ctx.forget() doesn't take a matcher, the caller
has to resolve patterns against dirstate.

Patch

diff -r 4c71a26a4009 -r a1be8989c015 mercurial/cmdutil.py
--- a/mercurial/cmdutil.py	Sun Mar 04 21:16:36 2018 -0500
+++ b/mercurial/cmdutil.py	Sat Mar 10 12:33:19 2018 +0530
@@ -1996,7 +1996,7 @@ 
         for subpath in ctx.substate:
             ctx.sub(subpath).addwebdirpath(serverpath, webconf)
 
-def forget(ui, repo, match, prefix, explicitonly):
+def forget(ui, repo, match, prefix, explicitonly, **opts):
     join = lambda f: os.path.join(prefix, f)
     bad = []
     badfn = lambda x, y: bad.append(x) or match.bad(x, y)
@@ -2039,9 +2039,10 @@ 
         if ui.verbose or not match.exact(f):
             ui.status(_('removing %s\n') % match.rel(f))
 
-    rejected = wctx.forget(forget, prefix)
-    bad.extend(f for f in rejected if f in match.files())
-    forgot.extend(f for f in forget if f not in rejected)
+    if not opts.get(r'dry_run'):
+        rejected = wctx.forget(forget, prefix)
+        bad.extend(f for f in rejected if f in match.files())
+        forgot.extend(f for f in forget if f not in rejected)
     return bad, forgot
 
 def files(ui, ctx, m, fm, fmt, subrepos):
diff -r 4c71a26a4009 -r a1be8989c015 mercurial/commands.py
--- a/mercurial/commands.py	Sun Mar 04 21:16:36 2018 -0500
+++ b/mercurial/commands.py	Sat Mar 10 12:33:19 2018 +0530
@@ -2036,7 +2036,11 @@ 
     with ui.formatter('files', opts) as fm:
         return cmdutil.files(ui, ctx, m, fm, fmt, opts.get('subrepos'))
 
-@command('^forget', walkopts, _('[OPTION]... FILE...'), inferrepo=True)
+@command(
+    '^forget',
+    [('', 'dry-run', None, _('only print output'))]
+    + walkopts,
+    _('[OPTION]... FILE...'), inferrepo=True)
 def forget(ui, repo, *pats, **opts):
     """forget the specified files on the next commit
 
@@ -2071,7 +2075,7 @@ 
         raise error.Abort(_('no files specified'))
 
     m = scmutil.match(repo[None], pats, opts)
-    rejected = cmdutil.forget(ui, repo, m, prefix="", explicitonly=False)[0]
+    rejected = cmdutil.forget(ui, repo, m, prefix="", explicitonly=False, **opts)[0]
     return rejected and 1 or 0
 
 @command(