Patchwork D8030: uncopy: add support for unmarking committed copies

login
register
mail settings
Submitter phabricator
Date Jan. 28, 2020, 11:54 p.m.
Message ID <differential-rev-PHID-DREV-trom3wfol3ivh26qyrtp-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/44722/
State Superseded
Headers show

Comments

phabricator - Jan. 28, 2020, 11:54 p.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The simplest way I'm aware of to unmark a file as copied after
  committing is this:
  
    hg uncommit --keep <dest>
    hg forget <dest>
    hg add <dest>
    hg amend
  
  This patch teaches `hg uncopy` a `-r` argument to simplify that into:
  
    hg uncopy -r . <dest>
  
  In addition to being simpler, it doesn't touch the working copy, so it
  can easily be used even if the destination file has been modified in
  the working copy.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D8030

AFFECTED FILES
  mercurial/cmdutil.py
  mercurial/commands.py
  mercurial/context.py
  relnotes/5.3
  relnotes/next
  tests/test-completion.t
  tests/test-copy.t
  tests/test-rename-after-merge.t

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 29, 2020, 10:43 p.m.
durin42 added a comment.


  I'm conflicted on rolling the `uncopy && amend` action into the uncopy command like this. Could users not (without this patch) do `hg uncopy foo && hg amend`?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8030/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8030

To: martinvonz, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - Jan. 29, 2020, 10:48 p.m.
martinvonz added a comment.


  In D8030#118480 <https://phab.mercurial-scm.org/D8030#118480>, @durin42 wrote:
  
  > I'm conflicted on rolling the `uncopy && amend` action into the uncopy command like this. Could users not (without this patch) do `hg uncopy foo && hg amend`?
  
  I don't think so, because it's not marked as a copy in the working copy, right?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8030/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8030

To: martinvonz, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - Jan. 29, 2020, 11:22 p.m.
durin42 added a comment.
durin42 accepted this revision as: durin42.


  In D8030#118489 <https://phab.mercurial-scm.org/D8030#118489>, @martinvonz wrote:
  
  > In D8030#118480 <https://phab.mercurial-scm.org/D8030#118480>, @durin42 wrote:
  >
  >> I'm conflicted on rolling the `uncopy && amend` action into the uncopy command like this. Could users not (without this patch) do `hg uncopy foo && hg amend`?
  >
  > I don't think so, because it's not marked as a copy in the working copy, right?
  
  Oh. Ew.
  
  I'm really not sure how I feel about that, but it does feel like this change is probably the best path forward.
  
  I'm out of time to review today (need to go make supper), but I'm at least +0 on this patch...

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8030/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8030

To: martinvonz, #hg-reviewers, durin42
Cc: durin42, mercurial-devel
phabricator - Jan. 29, 2020, 11:29 p.m.
martinvonz added a comment.


  In D8030#118542 <https://phab.mercurial-scm.org/D8030#118542>, @durin42 wrote:
  
  > In D8030#118489 <https://phab.mercurial-scm.org/D8030#118489>, @martinvonz wrote:
  >
  >> In D8030#118480 <https://phab.mercurial-scm.org/D8030#118480>, @durin42 wrote:
  >>
  >>> I'm conflicted on rolling the `uncopy && amend` action into the uncopy command like this. Could users not (without this patch) do `hg uncopy foo && hg amend`?
  >>
  >> I don't think so, because it's not marked as a copy in the working copy, right?
  >
  > Oh. Ew.
  > I'm really not sure how I feel about that, but it does feel like this change is probably the best path forward.
  > I'm out of time to review today (need to go make supper), but I'm at least +0 on this patch...
  
  I've started to think that the working copy should really be a commit and I find that helpful. If you think about it that way, it makes sense that `hg uncopy foo` (or more explicitly `hg uncopy -r 'wdir()' foo` unmarks copies in the working copy and `hg uncopy -r <some other commit> foo` unmarks copies in some other commit.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8030/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8030

To: martinvonz, #hg-reviewers, durin42
Cc: durin42, mercurial-devel
phabricator - Jan. 30, 2020, 2:24 p.m.
pulkit added a comment.


  I don't feel very good with this because this makes `hg uncopy` a history editing command. Something like `hg uncopy --after` which makes the changes in wdir which can be committed afterwards sounds best, but I am not sure if that's possible.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8030/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8030

To: martinvonz, #hg-reviewers, durin42
Cc: pulkit, durin42, mercurial-devel
phabricator - Jan. 30, 2020, 2:33 p.m.
martinvonz added a comment.


  In D8030#118662 <https://phab.mercurial-scm.org/D8030#118662>, @pulkit wrote:
  
  > I don't feel very good with this because this makes `hg uncopy` a history editing command.
  
  That's a good thing in my opinion :)
  
  > Something like `hg uncopy --after` which makes the changes in wdir which can be committed afterwards sounds best, but I am not sure if that's possible.
  
  Yeah, not really possible (Augie had the same comment on one of these patches).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8030/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8030

To: martinvonz, #hg-reviewers, durin42
Cc: pulkit, durin42, mercurial-devel
phabricator - Feb. 7, 2020, 8:31 p.m.
marmoute added a comment.


  >> Something like `hg uncopy --after` which makes the changes in wdir which can be committed afterwards sounds best, but I am not sure if that's possible.
  >
  > Yeah, not really possible (Augie had the same comment on one of these patches).
  
  Can we update the dirstate to represent such copy related information ? (both for adding or removing a copy).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8030/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8030

To: martinvonz, #hg-reviewers, durin42
Cc: marmoute, pulkit, durin42, mercurial-devel
phabricator - Feb. 12, 2020, 7:43 p.m.
durin42 added a comment.
durin42 accepted this revision.


  I remain happy with this.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8030/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8030

To: martinvonz, #hg-reviewers, durin42, marmoute
Cc: marmoute, pulkit, durin42, mercurial-devel
phabricator - Feb. 13, 2020, 11:28 p.m.
This revision now requires changes to proceed.
marmoute added a comment.
marmoute requested changes to this revision.


  I had a longer discussion with @martinvonz on IRC on about this feature, he also pointed out to  D7631 <https://phab.mercurial-scm.org/D7631> doing similar work for `hg absorb`.
  
  From that discussion I understand there is a wider objective for many mercurial commands to gain the ability to use a specific revision in place of the working copy. (eg: hg amend, hg aborb, hg copy, etc…)
  
  The idea is quite interresting and can unlock powerfull capabilities. However, this is a rather big UX project, I think we need a step back and think about this idea on a large scale. I am thinking at least a Plan page with a wider study of the command that could be impacted how the UI will be consistent. This allows a wider set of the community to chim in on  such a big project.
  
  A concret issue I current have with the plan is the planned `--rev` flag usage. To me, and other people around me, something like `hg amend --rev X` would means "amend change from the working copy into revision `X`". likewise `hg absorb --rev REVSET` would absorb the working copy change into the `REVSET` set  of revisions. So the use of `--rev` is surprising to some and should be discussed. Maybe the `--change` flag (used, but `hg status` and `hg diff`) would be more suitable? Maybe a global flag --change-as-wdir that would work for any commands ?
  
  This is the kind of question I would like to see discussed more widely before we invest more in this idea (Idea, that I find interresting overall). I added this as a topic for the upcoming sprint.
  
  (Sorry for the delay this will involves, I wish the idea had explicitly surfaced earlier).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8030/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8030

To: martinvonz, #hg-reviewers, durin42, marmoute
Cc: marmoute, pulkit, durin42, mercurial-devel
phabricator - Feb. 13, 2020, 11:33 p.m.
martinvonz added a comment.


  In D8030#120771 <https://phab.mercurial-scm.org/D8030#120771>, @marmoute wrote:
  
  > I had a longer discussion with @martinvonz on IRC on about this feature, he also pointed out to  D7631 <https://phab.mercurial-scm.org/D7631> doing similar work for `hg absorb`.
  > From that discussion I understand there is a wider objective for many mercurial commands to gain the ability to use a specific revision in place of the working copy. (eg: hg amend, hg aborb, hg copy, etc…)
  > The idea is quite interresting and can unlock powerfull capabilities. However, this is a rather big UX project, I think we need a step back and think about this idea on a large scale. I am thinking at least a Plan page with a wider study of the command that could be impacted how the UI will be consistent. This allows a wider set of the community to chim in on  such a big project.
  > A concret issue I current have with the plan is the planned `--rev` flag usage. To me, and other people around me, something like `hg amend --rev X` would means "amend change from the working copy into revision `X`". likewise `hg absorb --rev REVSET` would absorb the working copy change into the `REVSET` set  of revisions. So the use of `--rev` is surprising to some and should be discussed. Maybe the `--change` flag (used, but `hg status` and `hg diff`) would be more suitable? Maybe a global flag --change-as-wdir that would work for any commands ?
  > This is the kind of question I would like to see discussed more widely before we invest more in this idea (Idea, that I find interresting overall). I added this as a topic for the upcoming sprint.
  > (Sorry for the delay this will involves, I wish the idea had explicitly surfaced earlier).
  
  I'm fine with marking the use of `-r` experimental. I don't like the idea of delaying this feature two months. Any problem with marking it experimental for now? As usual with experimental features, that will allow us to rename the flag (and change other behavior) before we take it out of experimental.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8030/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8030

To: martinvonz, #hg-reviewers, durin42, marmoute
Cc: marmoute, pulkit, durin42, mercurial-devel
phabricator - Feb. 13, 2020, 11:46 p.m.
marmoute added a comment.


  In D8030#120781 <https://phab.mercurial-scm.org/D8030#120781>, @martinvonz wrote:
  
  > I don't like the idea of delaying this feature two months.
  
  I believe we need wider discussion about this before going further, this is an interresting change but we cannot rush it. This not necessarly result in a to a 1.5 month delay if a proper community discussion happens before that. Can you create and fill a Plan page and start a community discussion around this?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8030/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8030

To: martinvonz, #hg-reviewers, durin42, marmoute
Cc: marmoute, pulkit, durin42, mercurial-devel
phabricator - Feb. 14, 2020, 9:09 a.m.
Alphare added a comment.


  > I'm fine with marking the use of `-r` experimental. I don't like the idea of delaying this feature two months. Any problem with marking it experimental for now? As usual with experimental features, that will allow us to rename the flag (and change other behavior) before we take it out of experimental.
  
  Aside from (obviously) @marmoute, both Jordi and I had an instant reaction of "what?" when we learned what `--rev` meant.
  
  I think that this goes against the intuition of at least a few people that are very accustomed to `hg`. While being able to operate on any particular rev could be a good - even great - idea in general, I am very uneasy about using `--rev` to qualify that target. I don't know if `--target-rev` is an option, but something similar would be much clearer that an already *extremely* common flag. This would shift the semantics of `--rev` in some commands and not in others.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8030/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8030

To: martinvonz, #hg-reviewers, durin42, marmoute
Cc: Alphare, marmoute, pulkit, durin42, mercurial-devel
phabricator - Feb. 14, 2020, 2:35 p.m.
martinvonz added a comment.


  In D8030#120786 <https://phab.mercurial-scm.org/D8030#120786>, @marmoute wrote:
  
  > In D8030#120781 <https://phab.mercurial-scm.org/D8030#120781>, @martinvonz wrote:
  >
  >> I don't like the idea of delaying this feature two months.
  >
  > I believe we need wider discussion about this before going further, this is an interresting change but we cannot rush it. This not necessarly result in a to a 1.5 month delay if a proper community discussion happens before that. Can you create and fill a Plan page and start a community discussion around this?
  
  
  
  In D8030#120861 <https://phab.mercurial-scm.org/D8030#120861>, @Alphare wrote:
  
  >> I'm fine with marking the use of `-r` experimental. I don't like the idea of delaying this feature two months. Any problem with marking it experimental for now? As usual with experimental features, that will allow us to rename the flag (and change other behavior) before we take it out of experimental.
  >
  > Aside from (obviously) @marmoute, both Jordi and I had an instant reaction of "what?" when we learned what `--rev` meant. 
  > I think that this goes against the intuition of at least a few people that are very accustomed to `hg`. While being able to operate on any particular rev could be a good - even great - idea in general, I am very uneasy about using `--rev` to qualify that target. I don't know if `--target-rev` is an option, but something similar would be much clearer that an already *extremely* common flag. This would shift the semantics of `--rev` in some commands and not in others.
  
  I'm fine with `--target-rev if that's

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8030/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8030

To: martinvonz, #hg-reviewers, durin42, marmoute
Cc: Alphare, marmoute, pulkit, durin42, mercurial-devel
phabricator - Feb. 14, 2020, 4:59 p.m.
martinvonz added a comment.


  In D8030#120899 <https://phab.mercurial-scm.org/D8030#120899>, @martinvonz wrote:
  
  > In D8030#120786 <https://phab.mercurial-scm.org/D8030#120786>, @marmoute wrote:
  >
  >> In D8030#120781 <https://phab.mercurial-scm.org/D8030#120781>, @martinvonz wrote:
  >>
  >>> I don't like the idea of delaying this feature two months.
  >>
  >> I believe we need wider discussion about this before going further, this is an interresting change but we cannot rush it. This not necessarly result in a to a 1.5 month delay if a proper community discussion happens before that. Can you create and fill a Plan page and start a community discussion around this?
  >>
  >>> ! In D8030#120861 <https://phab.mercurial-scm.org/D8030#120861>, @Alphare wrote:
  >>>  I'm fine with marking the use of `-r` experimental. I don't like the idea of delaying this feature two months. Any problem with marking it experimental for now? As usual with experimental features, that will allow us to rename the flag (and change other behavior) before we take it out of experimental.
  >>
  >> Aside from (obviously) @marmoute, both Jordi and I had an instant reaction of "what?" when we learned what `--rev` meant. 
  >> I think that this goes against the intuition of at least a few people that are very accustomed to `hg`. While being able to operate on any particular rev could be a good - even great - idea in general, I am very uneasy about using `--rev` to qualify that target. I don't know if `--target-rev` is an option, but something similar would be much clearer that an already *extremely* common flag. This would shift the semantics of `--rev` in some commands and not in others.
  >
  > I'm fine with `--target-rev if that's what people prefer. I find it a bit weird to not use `--rev` but that may just be because I'm been thinking of the working copy as a commit for longer than most.
  
  Actually, after thinking more about it, I'd like to keep `--rev` (still experimental) as the name. We already have `hg files`, `hg grep`, and `hg parents`, which have the same behavior as the `--rev` I'm using here (i.e. default to the working copy but allow the user to choose a commit to work on instead).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8030/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8030

To: martinvonz, #hg-reviewers, durin42, marmoute
Cc: Alphare, marmoute, pulkit, durin42, mercurial-devel

Patch

diff --git a/tests/test-rename-after-merge.t b/tests/test-rename-after-merge.t
--- a/tests/test-rename-after-merge.t
+++ b/tests/test-rename-after-merge.t
@@ -120,4 +120,10 @@ 
   $ hg log -r tip -C -v | grep copies
   copies:      b2 (b1)
 
+Test unmarking copies in merge commit
+
+  $ hg uncopy -r . b2
+  abort: cannot unmark copy in merge commit
+  [255]
+
   $ cd ..
diff --git a/tests/test-copy.t b/tests/test-copy.t
--- a/tests/test-copy.t
+++ b/tests/test-copy.t
@@ -319,5 +319,56 @@ 
   A dir2/bar
   A dir2/foo
   ? dir2/untracked
+# Clean up for next test
+  $ hg forget dir2
+  removing dir2/bar
+  removing dir2/foo
+  $ rm -r dir2
+
+Test uncopy on committed copies
+
+# Commit some copies
+  $ hg cp bar baz
+  $ hg cp bar qux
+  $ hg ci -m copies
+  $ hg st -C --change .
+  A baz
+    bar
+  A qux
+    bar
+  $ base=$(hg log -r '.^' -T '{rev}')
+  $ hg log -G -T '{rev}:{node|short} {desc}\n' -r $base:
+  @  5:a612dc2edfda copies
+  |
+  o  4:4800b1f1f38e add dir/
+  |
+  ~
+# Add a dirty change on top to show that it's unaffected
+  $ echo dirty >> baz
+  $ hg st
+  M baz
+  $ cat baz
+  bleah
+  dirty
+  $ hg uncopy -r . baz
+  saved backup bundle to $TESTTMP/part2/.hg/strip-backup/a612dc2edfda-e36b4448-uncopy.hg
+# The unwanted copy is no longer recorded, but the unrelated one is
+  $ hg st -C --change .
+  A baz
+  A qux
+    bar
+# The old commit is gone and we have updated to the new commit
+  $ hg log -G -T '{rev}:{node|short} {desc}\n' -r $base:
+  @  5:c45090e5effe copies
+  |
+  o  4:4800b1f1f38e add dir/
+  |
+  ~
+# Working copy still has the uncommitted change
+  $ hg st
+  M baz
+  $ cat baz
+  bleah
+  dirty
 
   $ cd ..
diff --git a/tests/test-completion.t b/tests/test-completion.t
--- a/tests/test-completion.t
+++ b/tests/test-completion.t
@@ -357,7 +357,7 @@ 
   tags: template
   tip: patch, git, style, template
   unbundle: update
-  uncopy: include, exclude
+  uncopy: rev, include, exclude
   unshelve: abort, continue, interactive, keep, name, tool, date
   update: clean, check, merge, date, rev, tool
   verify: full
diff --git a/relnotes/next b/relnotes/next
--- a/relnotes/next
+++ b/relnotes/next
@@ -1,6 +1,7 @@ 
 == New Features ==
 
- * `hg uncopy` can be used to unmark a file as copied.
+ * `hg uncopy` can be used to unmark a file as copied. Use `hg uncopy -r REV`
+   to unmark already committed copies.
 
 
 == New Experimental Features ==
diff --git a/relnotes/5.3 b/relnotes/5.3
--- a/relnotes/5.3
+++ b/relnotes/5.3
@@ -2,7 +2,8 @@ 
 
  * Windows will process hgrc files in %PROGRAMDATA%\Mercurial\hgrc.d.
 
- * `hg uncopy` can be used to unmark a file as copied.
+ * `hg uncopy` can be used to unmark a file as copied. Use `hg uncopy -r REV`
+   to unmark already committed copies.
 
 == New Experimental Features ==
 
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -2488,6 +2488,17 @@ 
             editor=editor,
         )
 
+    def tomemctx_for_amend(self, precursor):
+        extra = precursor.extra().copy()
+        extra[b'amend_source'] = precursor.hex()
+        return self.tomemctx(
+            text=precursor.description(),
+            branch=precursor.branch(),
+            extra=extra,
+            date=precursor.date(),
+            user=precursor.user(),
+        )
+
     def isdirty(self, path):
         return path in self._cache
 
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -7493,7 +7493,8 @@ 
 
 @command(
     b'uncopy',
-    walkopts,
+    [(b'r', b'rev', b'', _(b'unmark copies in the given revision'), _(b'REV'))]
+    + walkopts,
     _(b'[OPTION]... DEST...'),
     helpcategory=command.CATEGORY_FILE_CONTENTS,
 )
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -1695,7 +1695,23 @@ 
 
 
 def uncopy(ui, repo, pats, opts):
-    ctx = repo[None]
+    rev = opts[b'rev']
+    if rev:
+        ctx = scmutil.revsingle(repo, rev)
+    else:
+        ctx = repo[None]
+    if ctx.rev() is None:
+        new_ctx = ctx
+    else:
+        if len(ctx.parents()) > 1:
+            raise error.Abort(_(b'cannot unmark copy in merge commit'))
+        # avoid cycle context -> subrepo -> cmdutil
+        from . import context
+
+        rewriteutil.precheck(repo, [ctx.rev()], b'uncopy')
+        new_ctx = context.overlayworkingctx(repo)
+        new_ctx.setbase(ctx.p1())
+        mergemod.graft(repo, ctx, wctx=new_ctx)
 
     match = scmutil.match(ctx, pats, opts)
 
@@ -1705,13 +1721,24 @@ 
     uipathfn = scmutil.getuipathfn(repo)
     for f in ctx.walk(match):
         if f in current_copies:
-            ctx[f].markcopied(None)
+            new_ctx[f].markcopied(None)
         elif match.exact(f):
             ui.warn(
                 _(b'%s: not uncopying - file is not marked as copied\n')
                 % uipathfn(f)
             )
 
+    if ctx.rev() is not None:
+        with repo.lock():
+            mem_ctx = new_ctx.tomemctx_for_amend(ctx)
+            new_node = mem_ctx.commit()
+
+            if repo.dirstate.p1() == ctx.node():
+                with repo.dirstate.parentchange():
+                    scmutil.movedirstate(repo, repo[new_node])
+            replacements = {ctx.node(): [new_node]}
+            scmutil.cleanupnodes(repo, replacements, b'uncopy', fixphase=True)
+
 
 ## facility to let extension process additional data into an import patch
 # list of identifier to be executed in order