Patchwork copy: distinguish "file exists" cases and add a hint (BC)

login
register
mail settings
Submitter Augie Fackler
Date Sept. 26, 2016, 8:36 p.m.
Message ID <a1b65d0019f9ce96961c.1474922216@augie-macbookair2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/16789/
State Changes Requested
Headers show

Comments

Augie Fackler - Sept. 26, 2016, 8:36 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1474319739 14400
#      Mon Sep 19 17:15:39 2016 -0400
# Node ID a1b65d0019f9ce96961c5c9533a8658a92e5b9d4
# Parent  129e38a76f2ca43291ed3a12d92b5d4b7e3b7f2b
copy: distinguish "file exists" cases and add a hint (BC)

Users that want to add a copy record to an existing commit with 'hg
commit --amend' should be guided towards this workflow, rather than
reaching for some sort of uncommit-recommit flow. As part of this,
distinguish in the top-line error message whether the file merely
already exists (untracked) on disk or the file already exists in
history.
Pierre-Yves David - Sept. 27, 2016, 3:23 p.m.
On 09/26/2016 10:36 PM, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1474319739 14400
> #      Mon Sep 19 17:15:39 2016 -0400
> # Node ID a1b65d0019f9ce96961c5c9533a8658a92e5b9d4
> # Parent  129e38a76f2ca43291ed3a12d92b5d4b7e3b7f2b
> copy: distinguish "file exists" cases and add a hint (BC)

This appears to be a V3. I'm not sure why the V2 disappeared from 
patchwork before that (something looks fishy here).

> Users that want to add a copy record to an existing commit with 'hg
> commit --amend' should be guided towards this workflow, rather than
> reaching for some sort of uncommit-recommit flow. As part of this,
> distinguish in the top-line error message whether the file merely
> already exists (untracked) on disk or the file already exists in
> history.

Small side not. being able to record the copy after the fact is great 
and that is great that this changesets work toward making it more 
discoverable. Another important point would be to allow the same thing 
for move.

> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -640,8 +640,15 @@ def copy(ui, repo, pats, opts, rename=Fa
>
>          if not after and exists or after and state in 'mn':
>              if not opts['force']:
> -                ui.warn(_('%s: not overwriting - file exists\n') %
> -                        reltarget)
> +                if state in 'mn':
> +                    msg = _('%s: not overwriting - file already committed\n')
> +                    hint = _('(hg copy --after --force to replace the '
> +                             'file by recording a copy)\n')

This "replace" here is confusing. --after seems to point out that no 
actual replacement occurs, just a 'recording'. Why does this message 
mention 'replace' in addition to 'record'.

> +                else:
> +                    msg = _('%s: not overwriting - file exists\n')
> +                    hint = _('(hg copy --after to record the copy)\n')
> +                ui.warn(msg % reltarget)
> +                ui.warn(hint)
>                  return
>
>          if after:
> diff --git a/tests/test-copy.t b/tests/test-copy.t
> --- a/tests/test-copy.t
> +++ b/tests/test-copy.t
> @@ -226,11 +226,23 @@ foo was clean:
>    C foo
>  Trying to copy on top of an existing file fails,
>    $ hg copy -A bar foo
> -  foo: not overwriting - file exists
> +  foo: not overwriting - file already committed
> +  (hg copy --after --force to replace the file by recording a copy)
> +same error without the --after, so the user doesn't have to go through
> +two hints:
> +  $ hg copy bar foo
> +  foo: not overwriting - file already committed
> +  (hg copy --after --force to replace the file by recording a copy)

I'm not sure why pointing to --after here is correct? If the user did 
not used '--after' in the first place it is likely that they want to 
actually replace the destination content with the source content. Isn't it?

>  but it's considered modified after a copy --after --force
>    $ hg copy -Af bar foo
>    $ hg st -AC foo
>    M foo
>      bar
> +The hint for a file that exists but is not in file history doesn't
> +mention --force:
> +  $ touch xyzzy
> +  $ hg cp bar xyzzy
> +  xyzzy: not overwriting - file exists
> +  (hg copy --after to record the copy)
>
>    $ cd ..
> diff --git a/tests/test-rename.t b/tests/test-rename.t
> --- a/tests/test-rename.t
> +++ b/tests/test-rename.t
> @@ -265,7 +265,8 @@ move everything under directory d1 to ex
>  overwrite existing files (d2/b)
>
>    $ hg rename d1/* d2
> -  d2/b: not overwriting - file exists
> +  d2/b: not overwriting - file already committed
> +  (hg copy --after --force to replace the file by recording a copy)

This warning seems to be issued when 'rename' is used. Yet, it still 
mention 'hg copy'.
It also have an issue similar to the one pointed for copy previously. It 
point to --after while the user have not expressed any '--after' wish in 
the original command.

>    moving d1/d11/a1 to d2/d11/a1 (glob)
>    $ hg status -C
>    A d2/a
> @@ -370,6 +371,7 @@ attempt to overwrite an existing file
>    $ echo "ca" > d1/ca
>    $ hg rename d1/ba d1/ca
>    d1/ca: not overwriting - file exists
> +  (hg copy --after to record the copy)

ditto

>    $ hg status -C
>    ? d1/ca
>    $ hg update -C
> @@ -393,6 +395,7 @@ attempt to overwrite an existing broken
>    $ ln -s ba d1/ca
>    $ hg rename --traceback d1/ba d1/ca
>    d1/ca: not overwriting - file exists
> +  (hg copy --after to record the copy)

ditto.
Augie Fackler - Sept. 27, 2016, 8:37 p.m.
> On Sep 27, 2016, at 11:23, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> On 09/26/2016 10:36 PM, Augie Fackler wrote:
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1474319739 14400
>> #      Mon Sep 19 17:15:39 2016 -0400
>> # Node ID a1b65d0019f9ce96961c5c9533a8658a92e5b9d4
>> # Parent  129e38a76f2ca43291ed3a12d92b5d4b7e3b7f2b
>> copy: distinguish "file exists" cases and add a hint (BC)
> 

I'll get a v4 rolled tonight yet, assuming it doesn't turn out twisty and confusing to thread the action down into cmdutil.

>> Users that want to add a copy record to an existing commit with 'hg
>> commit --amend' should be guided towards this workflow, rather than
>> reaching for some sort of uncommit-recommit flow. As part of this,
>> distinguish in the top-line error message whether the file merely
>> already exists (untracked) on disk or the file already exists in
>> history.
> 
> Small side not. being able to record the copy after the fact is great and that is great that this changesets work toward making it more discoverable. Another important point would be to allow the same thing for move.
> 
>> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
>> --- a/mercurial/cmdutil.py
>> +++ b/mercurial/cmdutil.py
>> @@ -640,8 +640,15 @@ def copy(ui, repo, pats, opts, rename=Fa
>> 
>>         if not after and exists or after and state in 'mn':
>>             if not opts['force']:
>> -                ui.warn(_('%s: not overwriting - file exists\n') %
>> -                        reltarget)
>> +                if state in 'mn':
>> +                    msg = _('%s: not overwriting - file already committed\n')
>> +                    hint = _('(hg copy --after --force to replace the '
>> +                             'file by recording a copy)\n')
> 
> This "replace" here is confusing. --after seems to point out that no actual replacement occurs, just a 'recording'. Why does this message mention 'replace' in addition to 'record'.

Folks on irc were very insistent that the word replace matters, notably timeless. My algorithm is that timeless is right about these things pretty much by default, so I went with a tweak of his wording. I'm not going to change this in the next version unless I hear a lot about it from native speakers.

(I agree the word replace here is a little odd, but it's not strictly wrong data-model wise, and the hint text is very much something we can tweak if it's a problem in the real world.)

> 
>> +                else:
>> +                    msg = _('%s: not overwriting - file exists\n')
>> +                    hint = _('(hg copy --after to record the copy)\n')
>> +                ui.warn(msg % reltarget)
>> +                ui.warn(hint)
>>                 return
>> 
>>         if after:
>> diff --git a/tests/test-copy.t b/tests/test-copy.t
>> --- a/tests/test-copy.t
>> +++ b/tests/test-copy.t
>> @@ -226,11 +226,23 @@ foo was clean:
>>   C foo
>> Trying to copy on top of an existing file fails,
>>   $ hg copy -A bar foo
>> -  foo: not overwriting - file exists
>> +  foo: not overwriting - file already committed
>> +  (hg copy --after --force to replace the file by recording a copy)
>> +same error without the --after, so the user doesn't have to go through
>> +two hints:
>> +  $ hg copy bar foo
>> +  foo: not overwriting - file already committed
>> +  (hg copy --after --force to replace the file by recording a copy)
> 
> I'm not sure why pointing to --after here is correct? If the user did not used '--after' in the first place it is likely that they want to actually replace the destination content with the source content. Isn't it?

Aw, heck. There are actually 6 cases:

+-------------+------------+--------------+
|             |            |              |
|             | replace    |preserve      |
|             | dest.      |dest.         |
|             | contents   |contents      |
|             |            |              |
+-------------+------------+--------------+
|             |            |              |
| dest        |            |  --after     |
| doesn't     | [no flags] | [an error]   |
| exist       |            |              |
+-------------+------------+--------------+
|             |            |              |
| dest. exists|            |              |
| but is not  | --force    | --after      |
| tracked     |            |              |
|             |            |              |
+-------------+------------+--------------+
|             |            |              |
| dest exists |            |              |
| and is      | --force    | --after      |
| tracked     |            | --force      |
|             |            |              |
+-------------+------------+--------------+


> 
>> but it's considered modified after a copy --after --force
>>   $ hg copy -Af bar foo
>>   $ hg st -AC foo
>>   M foo
>>     bar
>> +The hint for a file that exists but is not in file history doesn't
>> +mention --force:
>> +  $ touch xyzzy
>> +  $ hg cp bar xyzzy
>> +  xyzzy: not overwriting - file exists
>> +  (hg copy --after to record the copy)
>> 
>>   $ cd ..
>> diff --git a/tests/test-rename.t b/tests/test-rename.t
>> --- a/tests/test-rename.t
>> +++ b/tests/test-rename.t
>> @@ -265,7 +265,8 @@ move everything under directory d1 to ex
>> overwrite existing files (d2/b)
>> 
>>   $ hg rename d1/* d2
>> -  d2/b: not overwriting - file exists
>> +  d2/b: not overwriting - file already committed
>> +  (hg copy --after --force to replace the file by recording a copy)
> 
> This warning seems to be issued when 'rename' is used. Yet, it still mention 'hg copy'.
> It also have an issue similar to the one pointed for copy previously. It point to --after while the user have not expressed any '--after' wish in the original command.

Sigh. Yep. I'll see if I can fix this too.

> 
>>   moving d1/d11/a1 to d2/d11/a1 (glob)
>>   $ hg status -C
>>   A d2/a
>> @@ -370,6 +371,7 @@ attempt to overwrite an existing file
>>   $ echo "ca" > d1/ca
>>   $ hg rename d1/ba d1/ca
>>   d1/ca: not overwriting - file exists
>> +  (hg copy --after to record the copy)
> 
> ditto
> 
>>   $ hg status -C
>>   ? d1/ca
>>   $ hg update -C
>> @@ -393,6 +395,7 @@ attempt to overwrite an existing broken
>>   $ ln -s ba d1/ca
>>   $ hg rename --traceback d1/ba d1/ca
>>   d1/ca: not overwriting - file exists
>> +  (hg copy --after to record the copy)
> 
> ditto.
> 
> -- 
> Pierre-Yves David
Pierre-Yves David - Sept. 28, 2016, 12:28 p.m.
On 09/27/2016 10:37 PM, Augie Fackler wrote:
>
>> On Sep 27, 2016, at 11:23, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>> On 09/26/2016 10:36 PM, Augie Fackler wrote:
>>> # HG changeset patch
>>> # User Augie Fackler <augie@google.com>
>>> # Date 1474319739 14400
>>> #      Mon Sep 19 17:15:39 2016 -0400
>>> # Node ID a1b65d0019f9ce96961c5c9533a8658a92e5b9d4
>>> # Parent  129e38a76f2ca43291ed3a12d92b5d4b7e3b7f2b
>>> copy: distinguish "file exists" cases and add a hint (BC)
>>
>
> I'll get a v4 rolled tonight yet, assuming it doesn't turn out twisty and confusing to thread the action down into cmdutil.
>
>>> Users that want to add a copy record to an existing commit with 'hg
>>> commit --amend' should be guided towards this workflow, rather than
>>> reaching for some sort of uncommit-recommit flow. As part of this,
>>> distinguish in the top-line error message whether the file merely
>>> already exists (untracked) on disk or the file already exists in
>>> history.
>>
>> Small side not. being able to record the copy after the fact is great and that is great that this changesets work toward making it more discoverable. Another important point would be to allow the same thing for move.
>>
>>> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
>>> --- a/mercurial/cmdutil.py
>>> +++ b/mercurial/cmdutil.py
>>> @@ -640,8 +640,15 @@ def copy(ui, repo, pats, opts, rename=Fa
>>>
>>>         if not after and exists or after and state in 'mn':
>>>             if not opts['force']:
>>> -                ui.warn(_('%s: not overwriting - file exists\n') %
>>> -                        reltarget)
>>> +                if state in 'mn':
>>> +                    msg = _('%s: not overwriting - file already committed\n')
>>> +                    hint = _('(hg copy --after --force to replace the '
>>> +                             'file by recording a copy)\n')
>>
>> This "replace" here is confusing. --after seems to point out that no actual replacement occurs, just a 'recording'. Why does this message mention 'replace' in addition to 'record'.
>
> Folks on irc were very insistent that the word replace matters, notably timeless. My algorithm is that timeless is right about these things pretty much by default, so I went with a tweak of his wording. I'm not going to change this in the next version unless I hear a lot about it from native speakers.
>
> (I agree the word replace here is a little odd, but it's not strictly wrong data-model wise, and the hint text is very much something we can tweak if it's a problem in the real world.)

I'm curious about the timeless reasoning around using "replace" for a 
pure book keeping operation that does not replace the file content. (But 
not challenging that timeless is most probably right). Timeless can you 
elaborate a bit ?
timeless - Sept. 28, 2016, 11:55 p.m.
Pierre-Yves David wrote:
> I'm curious about the timeless reasoning around using "replace" for a pure
> book keeping operation that does not replace the file content. (But not
> challenging that timeless is most probably right). Timeless can you
> elaborate a bit ?

It replaces the file history (what hg blame and friends show)

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -640,8 +640,15 @@  def copy(ui, repo, pats, opts, rename=Fa
 
         if not after and exists or after and state in 'mn':
             if not opts['force']:
-                ui.warn(_('%s: not overwriting - file exists\n') %
-                        reltarget)
+                if state in 'mn':
+                    msg = _('%s: not overwriting - file already committed\n')
+                    hint = _('(hg copy --after --force to replace the '
+                             'file by recording a copy)\n')
+                else:
+                    msg = _('%s: not overwriting - file exists\n')
+                    hint = _('(hg copy --after to record the copy)\n')
+                ui.warn(msg % reltarget)
+                ui.warn(hint)
                 return
 
         if after:
diff --git a/tests/test-copy.t b/tests/test-copy.t
--- a/tests/test-copy.t
+++ b/tests/test-copy.t
@@ -226,11 +226,23 @@  foo was clean:
   C foo
 Trying to copy on top of an existing file fails,
   $ hg copy -A bar foo
-  foo: not overwriting - file exists
+  foo: not overwriting - file already committed
+  (hg copy --after --force to replace the file by recording a copy)
+same error without the --after, so the user doesn't have to go through
+two hints:
+  $ hg copy bar foo
+  foo: not overwriting - file already committed
+  (hg copy --after --force to replace the file by recording a copy)
 but it's considered modified after a copy --after --force
   $ hg copy -Af bar foo
   $ hg st -AC foo
   M foo
     bar
+The hint for a file that exists but is not in file history doesn't
+mention --force:
+  $ touch xyzzy
+  $ hg cp bar xyzzy
+  xyzzy: not overwriting - file exists
+  (hg copy --after to record the copy)
 
   $ cd ..
diff --git a/tests/test-rename.t b/tests/test-rename.t
--- a/tests/test-rename.t
+++ b/tests/test-rename.t
@@ -265,7 +265,8 @@  move everything under directory d1 to ex
 overwrite existing files (d2/b)
 
   $ hg rename d1/* d2
-  d2/b: not overwriting - file exists
+  d2/b: not overwriting - file already committed
+  (hg copy --after --force to replace the file by recording a copy)
   moving d1/d11/a1 to d2/d11/a1 (glob)
   $ hg status -C
   A d2/a
@@ -370,6 +371,7 @@  attempt to overwrite an existing file
   $ echo "ca" > d1/ca
   $ hg rename d1/ba d1/ca
   d1/ca: not overwriting - file exists
+  (hg copy --after to record the copy)
   $ hg status -C
   ? d1/ca
   $ hg update -C
@@ -393,6 +395,7 @@  attempt to overwrite an existing broken 
   $ ln -s ba d1/ca
   $ hg rename --traceback d1/ba d1/ca
   d1/ca: not overwriting - file exists
+  (hg copy --after to record the copy)
   $ hg status -C
   ? d1/ca
   $ hg update -C