Page MenuHomePhabricator

copy: add experimetal support for unmarking committed copies
ClosedPublic

Authored by martinvonz on Tue, Jan 28, 6:51 PM.

Details

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 copy --forget a -r argument to simplify that into:

hg copy --forget -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.

I'll teach hg copy without --forget to work with -r next.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

martinvonz created this revision.Tue, Jan 28, 6:51 PM
martinvonz updated this revision to Diff 19670.Tue, Jan 28, 6:55 PM

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'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?

durin42 accepted this revision as: durin42.Wed, Jan 29, 6:22 PM

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'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.

martinvonz updated this revision to Diff 19693.Wed, Jan 29, 6:37 PM
pulkit added a subscriber: pulkit.Thu, Jan 30, 9:24 AM

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.

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).

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).

marmoute requested changes to this revision.Fri, Feb 7, 3:47 PM

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).

Something that really bother me with this kind of command rewriting history is that it can result in an absurd amount of rewrite.

The usual way to change something in mercurial is:

  1. get on a clean revision
  2. make changes 1
  3. make changes 2
  4. make changes …
  5. make changes 42
  6. record the changes (commit, amend, etc…)

If command like hg copy start rewriting history, each smallest atomic operation result rewriting one (or multiple) changesets. This is a slippery slope I would rather not walk on.

I think we should explore being able to record this in dirstate and use regular amend to record the change.

mercurial/cmdutil.py
1765

Why can we do it for merge commit ?

1766

I prefer this kind of shortcut to be exceptional. Maybe this need to move out of cmdutil ?

1769

bonus point for using precheck ☺

This revision now requires changes to proceed.Fri, Feb 7, 3:47 PM
martinvonz marked 3 inline comments as done.Fri, Feb 7, 3:53 PM

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).

Something that really bother me with this kind of command rewriting history is that it can result in an absurd amount of rewrite.
The usual way to change something in mercurial is:

  1. get on a clean revision
  2. make changes 1
  3. make changes 2
  4. make changes …
  5. make changes 42
  6. record the changes (commit, amend, etc…)

If command like hg copy start rewriting history, each smallest atomic operation result rewriting one (or multiple) changesets. This is a slippery slope I would rather not walk on.
I think we should explore being able to record this in dirstate and use regular amend to record the change.

I completely disagree :P I think it's a good thing to allow it to work on other commits. Of course it's better if the user thinks of it before they commit, but when they don't, we should still make it as easy as possible.

mercurial/cmdutil.py
1765

Because I didn't feel like implementing support for it yet. Can be done in a follow-up, IMO.

1766

I agree (of course). There are already several cases in this file. I thought about the cycle for a few minutes and it wasn't clear how it should be broken. How do you suggest we resolve the cycle?

1769

Thanks :)

martinvonz marked 3 inline comments as done.Mon, Feb 10, 5:42 PM
martinvonz retitled this revision from uncopy: add support for unmarking committed copies to copy: add support for unmarking committed copies.
martinvonz edited the summary of this revision. (Show Details)
martinvonz updated this revision to Diff 20069.
martinvonz updated this revision to Diff 20082.Mon, Feb 10, 6:30 PM
durin42 accepted this revision.Wed, Feb 12, 2:43 PM

I remain happy with this.

martinvonz updated this revision to Diff 20192.Thu, Feb 13, 2:09 PM
martinvonz updated this revision to Diff 20200.Thu, Feb 13, 4:37 PM
martinvonz updated this revision to Diff 20205.Thu, Feb 13, 6:22 PM
marmoute requested changes to this revision.Thu, Feb 13, 6:28 PM

I had a longer discussion with @martinvonz on IRC on about this feature, he also pointed out to 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).

This revision now requires changes to proceed.Thu, Feb 13, 6:28 PM

I had a longer discussion with @martinvonz on IRC on about this feature, he also pointed out to 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.

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?

martinvonz retitled this revision from copy: add support for unmarking committed copies to copy: add experimetal support for unmarking committed copies.Thu, Feb 13, 6:53 PM
martinvonz updated this revision to Diff 20207.

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.

martinvonz added a comment.EditedFri, Feb 14, 9:35 AM

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?

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.

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, @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).

martinvonz updated this revision to Diff 20227.Fri, Feb 14, 4:38 PM
martinvonz updated this revision to Diff 20231.Fri, Feb 14, 6:19 PM

fixed stale uses of -r in commit message and transient code

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.