Patchwork [evolve-ext] uncommit: abort if an explicitly given file cannot be uncommitted (BC)

login
register
mail settings
Submitter Matt Harbison
Date April 3, 2019, 2:45 a.m.
Message ID <272e76e20c3c52fcae8a.1554259515@Envy>
Download mbox | patch
Permalink /patch/39458/
State Superseded
Headers show

Comments

Matt Harbison - April 3, 2019, 2:45 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1554259321 14400
#      Tue Apr 02 22:42:01 2019 -0400
# Node ID 272e76e20c3c52fcae8a4c2003f73d8db77e566b
# Parent  e8515ee7a7cc0281d905f762e26e8956ccfd7f74
uncommit: abort if an explicitly given file cannot be uncommitted (BC)

This corresponds to f4147ca63d39 in the core uncommit extension.
Matt Harbison - April 9, 2019, 12:48 a.m.
On Tue, 02 Apr 2019 22:45:15 -0400, Matt Harbison <mharbison72@gmail.com>  
wrote:

> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1554259321 14400
> #      Tue Apr 02 22:42:01 2019 -0400
> # Node ID 272e76e20c3c52fcae8a4c2003f73d8db77e566b
> # Parent  e8515ee7a7cc0281d905f762e26e8956ccfd7f74
> uncommit: abort if an explicitly given file cannot be uncommitted (BC)

Gentle ping on this (for evolve).

Is there a more preferred way to send patches for evolve, like PRs?
Pierre-Yves David - April 9, 2019, 2:41 p.m.
PAtch looks good overall, but has compatibility issue with older 
version. Can you have a look at my comment below?

On 4/3/19 4:45 AM, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1554259321 14400
> #      Tue Apr 02 22:42:01 2019 -0400
> # Node ID 272e76e20c3c52fcae8a4c2003f73d8db77e566b
> # Parent  e8515ee7a7cc0281d905f762e26e8956ccfd7f74
> uncommit: abort if an explicitly given file cannot be uncommitted (BC)
> 
> This corresponds to f4147ca63d39 in the core uncommit extension.
> 
> diff --git a/hgext3rd/evolve/cmdrewrite.py b/hgext3rd/evolve/cmdrewrite.py
> --- a/hgext3rd/evolve/cmdrewrite.py
> +++ b/hgext3rd/evolve/cmdrewrite.py
> @@ -520,17 +520,42 @@
>           if disallowunstable and not onahead:
>               raise error.Abort(_("cannot uncommit in the middle of a stack"))
>   
> +        match = scmutil.match(old, pats, opts)
> +
> +        # Check all explicitly given files; abort if there's a problem.
> +        if match.files():
> +            s = old.status(old.p1(), match, listclean=True)
> +            eligible = set(s.added) | set(s.modified) | set(s.removed)
> +
> +            badfiles = set(match.files()) - eligible
> +
> +            # Naming a parent directory of an eligible file is OK, even
> +            # if not everything tracked in that directory can be
> +            # uncommitted.
> +            if badfiles:
> +                badfiles -= set([f for f in util.dirs(eligible)])
> +
> +            for f in sorted(badfiles):
> +                if f in s.clean:
> +                    hint = _(b"file was not changed in working directory "
> +                             b"parent")
> +                elif repo.wvfs.exists(f):
> +                    hint = _(b"file was untracked in working directory parent")
> +                else:
> +                    hint = _(b"file does not exist")
> +
> +                raise error.Abort(_(b'cannot uncommit "%s"')
> +                                  % scmutil.getuipathfn(repo)(f), hint=hint)
> +

Evolve is compatible with version down to 4.4 and `getuipathfn` seems 
newer than that. Can you send a V2 compatible with the older version ?

Cheers,
via Mercurial-devel - April 10, 2019, 12:26 a.m.
On Tue, Apr 9, 2019 at 7:44 AM Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

> PAtch looks good overall, but has compatibility issue with older
> version. Can you have a look at my comment below?
>
> On 4/3/19 4:45 AM, Matt Harbison wrote:
> > # HG changeset patch
> > # User Matt Harbison <matt_harbison@yahoo.com>
> > # Date 1554259321 14400
> > #      Tue Apr 02 22:42:01 2019 -0400
> > # Node ID 272e76e20c3c52fcae8a4c2003f73d8db77e566b
> > # Parent  e8515ee7a7cc0281d905f762e26e8956ccfd7f74
> > uncommit: abort if an explicitly given file cannot be uncommitted (BC)
> >
> > This corresponds to f4147ca63d39 in the core uncommit extension.
> >
> > diff --git a/hgext3rd/evolve/cmdrewrite.py
> b/hgext3rd/evolve/cmdrewrite.py
> > --- a/hgext3rd/evolve/cmdrewrite.py
> > +++ b/hgext3rd/evolve/cmdrewrite.py
> > @@ -520,17 +520,42 @@
> >           if disallowunstable and not onahead:
> >               raise error.Abort(_("cannot uncommit in the middle of a
> stack"))
> >
> > +        match = scmutil.match(old, pats, opts)
> > +
> > +        # Check all explicitly given files; abort if there's a problem.
> > +        if match.files():
> > +            s = old.status(old.p1(), match, listclean=True)
> > +            eligible = set(s.added) | set(s.modified) | set(s.removed)
> > +
> > +            badfiles = set(match.files()) - eligible
> > +
> > +            # Naming a parent directory of an eligible file is OK, even
> > +            # if not everything tracked in that directory can be
> > +            # uncommitted.
> > +            if badfiles:
> > +                badfiles -= set([f for f in util.dirs(eligible)])
> > +
> > +            for f in sorted(badfiles):
> > +                if f in s.clean:
> > +                    hint = _(b"file was not changed in working
> directory "
> > +                             b"parent")
> > +                elif repo.wvfs.exists(f):
> > +                    hint = _(b"file was untracked in working directory
> parent")
> > +                else:
> > +                    hint = _(b"file does not exist")
> > +
> > +                raise error.Abort(_(b'cannot uncommit "%s"')
> > +                                  % scmutil.getuipathfn(repo)(f),
> hint=hint)
> > +
>
> Evolve is compatible with version down to 4.4 and `getuipathfn` seems
> newer than that. Can you send a V2 compatible with the older version ?
>

Pierre-Yves, it may not be clear to Matt what exactly he's supposed to do
(it isn't to me anyway). Specifically, does `hg uncommit` try to use
absolute or relative paths? I'm thinking it's probably not a big deal for
now and an absolute path is okay? If that's the case, you could simply
replace the "scmutil.getuipathfn(repo)(f)" by "f" in flight if that's okay
with both of you.


>
> Cheers,
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Matt Harbison - April 10, 2019, 1:31 a.m.
On Tue, 09 Apr 2019 20:26:21 -0400, Martin von Zweigbergk  
<martinvonz@google.com> wrote:

> On Tue, Apr 9, 2019 at 7:44 AM Pierre-Yves David <
> pierre-yves.david@ens-lyon.org> wrote:
>
>> PAtch looks good overall, but has compatibility issue with older
>> version. Can you have a look at my comment below?
>>
>> On 4/3/19 4:45 AM, Matt Harbison wrote:
>> > # HG changeset patch
>> > # User Matt Harbison <matt_harbison@yahoo.com>
>> > # Date 1554259321 14400
>> > #      Tue Apr 02 22:42:01 2019 -0400
>> > # Node ID 272e76e20c3c52fcae8a4c2003f73d8db77e566b
>> > # Parent  e8515ee7a7cc0281d905f762e26e8956ccfd7f74
>> > uncommit: abort if an explicitly given file cannot be uncommitted (BC)
>> >
>> > This corresponds to f4147ca63d39 in the core uncommit extension.
>> >

>>
>> Evolve is compatible with version down to 4.4 and `getuipathfn` seems
>> newer than that. Can you send a V2 compatible with the older version ?
>>
>
> Pierre-Yves, it may not be clear to Matt what exactly he's supposed to do
> (it isn't to me anyway). Specifically, does `hg uncommit` try to use
> absolute or relative paths? I'm thinking it's probably not a big deal for
> now and an absolute path is okay? If that's the case, you could simply
> replace the "scmutil.getuipathfn(repo)(f)" by "f" in flight if that's  
> okay
> with both of you.

I'm OK with either relative or absolute output for now (though I thought  
most output other than status
was relative before your recent refactoring).  Maybe we just need to  
define a local `getuipathfn()` that either calls the scmutil function if  
available, otherwise calls matcher.rel()?  Diverging from the output of  
the bundled extension doesn't seem like a good idea.
via Mercurial-devel - April 10, 2019, 2:12 a.m.
On Tue, Apr 9, 2019, 18:31 Matt Harbison <mharbison72@gmail.com> wrote:

> On Tue, 09 Apr 2019 20:26:21 -0400, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
>
> > On Tue, Apr 9, 2019 at 7:44 AM Pierre-Yves David <
> > pierre-yves.david@ens-lyon.org> wrote:
> >
> >> PAtch looks good overall, but has compatibility issue with older
> >> version. Can you have a look at my comment below?
> >>
> >> On 4/3/19 4:45 AM, Matt Harbison wrote:
> >> > # HG changeset patch
> >> > # User Matt Harbison <matt_harbison@yahoo.com>
> >> > # Date 1554259321 14400
> >> > #      Tue Apr 02 22:42:01 2019 -0400
> >> > # Node ID 272e76e20c3c52fcae8a4c2003f73d8db77e566b
> >> > # Parent  e8515ee7a7cc0281d905f762e26e8956ccfd7f74
> >> > uncommit: abort if an explicitly given file cannot be uncommitted (BC)
> >> >
> >> > This corresponds to f4147ca63d39 in the core uncommit extension.
> >> >
>
> >>
> >> Evolve is compatible with version down to 4.4 and `getuipathfn` seems
> >> newer than that. Can you send a V2 compatible with the older version ?
> >>
> >
> > Pierre-Yves, it may not be clear to Matt what exactly he's supposed to do
> > (it isn't to me anyway). Specifically, does `hg uncommit` try to use
> > absolute or relative paths? I'm thinking it's probably not a big deal for
> > now and an absolute path is okay? If that's the case, you could simply
> > replace the "scmutil.getuipathfn(repo)(f)" by "f" in flight if that's
> > okay
> > with both of you.
>
> I'm OK with either relative or absolute output for now


So am I.

(though I thought
> most output other than status
> was relative before your recent refactoring).


I actually thought it was the other way around :)

Maybe we just need to
> define a local `getuipathfn()` that either calls the scmutil function if
> available, otherwise calls matcher.rel()?  Diverging from the output of
> the bundled extension doesn't seem like a good idea.
>

Seems like overkill to me, but it's up to Pierre-Yves. I'd understand it if
they wanted to do a similar overhaul in the entire evolve extension, but
doesn't seem worth it otherwise.

>
Pierre-Yves David - April 10, 2019, 10:37 a.m.
On 4/10/19 2:26 AM, Martin von Zweigbergk via Mercurial-devel wrote:
> 
> 
> On Tue, Apr 9, 2019 at 7:44 AM Pierre-Yves David 
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>> 
> wrote:
> 
>     PAtch looks good overall, but has compatibility issue with older
>     version. Can you have a look at my comment below?
> 
>     On 4/3/19 4:45 AM, Matt Harbison wrote:
>      > # HG changeset patch
>      > # User Matt Harbison <matt_harbison@yahoo.com
>     <mailto:matt_harbison@yahoo.com>>
>      > # Date 1554259321 14400
>      > #      Tue Apr 02 22:42:01 2019 -0400
>      > # Node ID 272e76e20c3c52fcae8a4c2003f73d8db77e566b
>      > # Parent  e8515ee7a7cc0281d905f762e26e8956ccfd7f74
>      > uncommit: abort if an explicitly given file cannot be uncommitted
>     (BC)
>      >
>      > This corresponds to f4147ca63d39 in the core uncommit extension.
>      >
>      > diff --git a/hgext3rd/evolve/cmdrewrite.py
>     b/hgext3rd/evolve/cmdrewrite.py
>      > --- a/hgext3rd/evolve/cmdrewrite.py
>      > +++ b/hgext3rd/evolve/cmdrewrite.py
>      > @@ -520,17 +520,42 @@
>      >           if disallowunstable and not onahead:
>      >               raise error.Abort(_("cannot uncommit in the middle
>     of a stack"))
>      >
>      > +        match = scmutil.match(old, pats, opts)
>      > +
>      > +        # Check all explicitly given files; abort if there's a
>     problem.
>      > +        if match.files():
>      > +            s = old.status(old.p1(), match, listclean=True)
>      > +            eligible = set(s.added) | set(s.modified) |
>     set(s.removed)
>      > +
>      > +            badfiles = set(match.files()) - eligible
>      > +
>      > +            # Naming a parent directory of an eligible file is
>     OK, even
>      > +            # if not everything tracked in that directory can be
>      > +            # uncommitted.
>      > +            if badfiles:
>      > +                badfiles -= set([f for f in util.dirs(eligible)])
>      > +
>      > +            for f in sorted(badfiles):
>      > +                if f in s.clean:
>      > +                    hint = _(b"file was not changed in working
>     directory "
>      > +                             b"parent")
>      > +                elif repo.wvfs.exists(f):
>      > +                    hint = _(b"file was untracked in working
>     directory parent")
>      > +                else:
>      > +                    hint = _(b"file does not exist")
>      > +
>      > +                raise error.Abort(_(b'cannot uncommit "%s"')
>      > +                                  %
>     scmutil.getuipathfn(repo)(f), hint=hint)
>      > +
> 
>     Evolve is compatible with version down to 4.4 and `getuipathfn` seems
>     newer than that. Can you send a V2 compatible with the older version ?
> 
> 
> Pierre-Yves, it may not be clear to Matt what exactly he's supposed to 
> do (it isn't to me anyway).

It is not to me either. The constraint is that we need a version 
compatible with older version. Any approach consistent with the rest of 
the evolve code base/behavior will work.

> Specifically, does `hg uncommit` try to use 
> absolute or relative paths? I'm thinking it's probably not a big deal 
> for now and an absolute path is okay? If that's the case, you could 
> simply replace the "scmutil.getuipathfn(repo)(f)" by "f" in flight if 
> that's okay with both of you.
> 
> 
>     Cheers,
> 
>     -- 
>     Pierre-Yves David
>     _______________________________________________
>     Mercurial-devel mailing list
>     Mercurial-devel@mercurial-scm.org
>     <mailto:Mercurial-devel@mercurial-scm.org>
>     https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> 
> 
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
via Mercurial-devel - April 10, 2019, 12:53 p.m.
On Wed, Apr 10, 2019, 03:37 Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 4/10/19 2:26 AM, Martin von Zweigbergk via Mercurial-devel wrote:
> >
> >
> > On Tue, Apr 9, 2019 at 7:44 AM Pierre-Yves David
> > <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>
> > wrote:
> >
> >     PAtch looks good overall, but has compatibility issue with older
> >     version. Can you have a look at my comment below?
> >
> >     On 4/3/19 4:45 AM, Matt Harbison wrote:
> >      > # HG changeset patch
> >      > # User Matt Harbison <matt_harbison@yahoo.com
> >     <mailto:matt_harbison@yahoo.com>>
> >      > # Date 1554259321 14400
> >      > #      Tue Apr 02 22:42:01 2019 -0400
> >      > # Node ID 272e76e20c3c52fcae8a4c2003f73d8db77e566b
> >      > # Parent  e8515ee7a7cc0281d905f762e26e8956ccfd7f74
> >      > uncommit: abort if an explicitly given file cannot be uncommitted
> >     (BC)
> >      >
> >      > This corresponds to f4147ca63d39 in the core uncommit extension.
> >      >
> >      > diff --git a/hgext3rd/evolve/cmdrewrite.py
> >     b/hgext3rd/evolve/cmdrewrite.py
> >      > --- a/hgext3rd/evolve/cmdrewrite.py
> >      > +++ b/hgext3rd/evolve/cmdrewrite.py
> >      > @@ -520,17 +520,42 @@
> >      >           if disallowunstable and not onahead:
> >      >               raise error.Abort(_("cannot uncommit in the middle
> >     of a stack"))
> >      >
> >      > +        match = scmutil.match(old, pats, opts)
> >      > +
> >      > +        # Check all explicitly given files; abort if there's a
> >     problem.
> >      > +        if match.files():
> >      > +            s = old.status(old.p1(), match, listclean=True)
> >      > +            eligible = set(s.added) | set(s.modified) |
> >     set(s.removed)
> >      > +
> >      > +            badfiles = set(match.files()) - eligible
> >      > +
> >      > +            # Naming a parent directory of an eligible file is
> >     OK, even
> >      > +            # if not everything tracked in that directory can be
> >      > +            # uncommitted.
> >      > +            if badfiles:
> >      > +                badfiles -= set([f for f in util.dirs(eligible)])
> >      > +
> >      > +            for f in sorted(badfiles):
> >      > +                if f in s.clean:
> >      > +                    hint = _(b"file was not changed in working
> >     directory "
> >      > +                             b"parent")
> >      > +                elif repo.wvfs.exists(f):
> >      > +                    hint = _(b"file was untracked in working
> >     directory parent")
> >      > +                else:
> >      > +                    hint = _(b"file does not exist")
> >      > +
> >      > +                raise error.Abort(_(b'cannot uncommit "%s"')
> >      > +                                  %
> >     scmutil.getuipathfn(repo)(f), hint=hint)
> >      > +
> >
> >     Evolve is compatible with version down to 4.4 and `getuipathfn` seems
> >     newer than that. Can you send a V2 compatible with the older version
> ?
> >
> >
> > Pierre-Yves, it may not be clear to Matt what exactly he's supposed to
> > do (it isn't to me anyway).
>
> It is not to me either. The constraint is that we need a version
> compatible with older version. Any approach consistent with the rest of
> the evolve code base/behavior will work.
>

Sure, I was just hoping that you knew the evolve code base better than we
do :P I tried to help Matt before I sent my previous email by trying to
figure out if uncommit seemed to use absolute paths or relative paths, but
I couldn't find an answer. If you also don't know the answer, I suppose
it's not very important which solution Matt picks (as I said before).


> > Specifically, does `hg uncommit` try to use
> > absolute or relative paths? I'm thinking it's probably not a big deal
> > for now and an absolute path is okay? If that's the case, you could
> > simply replace the "scmutil.getuipathfn(repo)(f)" by "f" in flight if
> > that's okay with both of you.
> >
> >
> >     Cheers,
> >
> >     --
> >     Pierre-Yves David
> >     _______________________________________________
> >     Mercurial-devel mailing list
> >     Mercurial-devel@mercurial-scm.org
> >     <mailto:Mercurial-devel@mercurial-scm.org>
> >     https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >
> >
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >
>
> --
> Pierre-Yves David
>

Patch

diff --git a/hgext3rd/evolve/cmdrewrite.py b/hgext3rd/evolve/cmdrewrite.py
--- a/hgext3rd/evolve/cmdrewrite.py
+++ b/hgext3rd/evolve/cmdrewrite.py
@@ -520,17 +520,42 @@ 
         if disallowunstable and not onahead:
             raise error.Abort(_("cannot uncommit in the middle of a stack"))
 
+        match = scmutil.match(old, pats, opts)
+
+        # Check all explicitly given files; abort if there's a problem.
+        if match.files():
+            s = old.status(old.p1(), match, listclean=True)
+            eligible = set(s.added) | set(s.modified) | set(s.removed)
+
+            badfiles = set(match.files()) - eligible
+
+            # Naming a parent directory of an eligible file is OK, even
+            # if not everything tracked in that directory can be
+            # uncommitted.
+            if badfiles:
+                badfiles -= set([f for f in util.dirs(eligible)])
+
+            for f in sorted(badfiles):
+                if f in s.clean:
+                    hint = _(b"file was not changed in working directory "
+                             b"parent")
+                elif repo.wvfs.exists(f):
+                    hint = _(b"file was untracked in working directory parent")
+                else:
+                    hint = _(b"file does not exist")
+
+                raise error.Abort(_(b'cannot uncommit "%s"')
+                                  % scmutil.getuipathfn(repo)(f), hint=hint)
+
         # Recommit the filtered changeset
         tr = repo.transaction('uncommit')
         if interactive:
             opts['all'] = True
-            match = scmutil.match(old, pats, opts)
             newid = _interactiveuncommit(ui, repo, old, match)
         else:
             newid = None
             includeorexclude = opts.get('include') or opts.get('exclude')
             if (pats or includeorexclude or opts.get('all')):
-                match = scmutil.match(old, pats, opts)
                 if not (opts['message'] or opts['logfile']):
                     opts['message'] = old.description()
                 message = cmdutil.logmessage(ui, opts)
diff --git a/tests/test-uncommit.t b/tests/test-uncommit.t
--- a/tests/test-uncommit.t
+++ b/tests/test-uncommit.t
@@ -514,3 +514,22 @@ 
   (use 'hg prune .' to remove it)
   $ hg status
   ? foo
+
+Bad explicit paths abort, like the bundled commit extension
+
+  $ hg uncommit foo
+  abort: cannot uncommit "foo"
+  (file was untracked in working directory parent)
+  [255]
+
+  $ hg ci -Aqm 'add foo'
+  $ hg uncommit bar
+  abort: cannot uncommit "bar"
+  (file does not exist)
+  [255]
+  $ hg uncommit d
+  abort: cannot uncommit "d"
+  (file was not changed in working directory parent)
+  [255]
+
+  $ hg status