Patchwork [STABLE] record: fix record with change on moved file crashes (issue4619)

login
register
mail settings
Submitter Laurent Charignon
Date April 22, 2015, 9:57 p.m.
Message ID <827224ee690234eb78e3.1429739826@lcharignon-mbp.local>
Download mbox | patch
Permalink /patch/8766/
State Accepted
Commit edf907bd8144544cb2c00fc074c4591a112d698c
Headers show

Comments

Laurent Charignon - April 22, 2015, 9:57 p.m.
# HG changeset patch
# User Laurent Charignon <lcharignon@fb.com>
# Date 1429736190 25200
#      Wed Apr 22 13:56:30 2015 -0700
# Branch stable
# Node ID 827224ee690234eb78e3c12ddcc381879bfba014
# Parent  1f9127c9239b9c39c676bb09752db1e2ca6c26f7
record: fix record with change on moved file crashes (issue4619)

reverting 79fceed67676, add a test to prevent the issue from coming back.
Matt Mackall - April 23, 2015, 12:22 a.m.
On Wed, 2015-04-22 at 14:57 -0700, Laurent Charignon wrote:
> # HG changeset patch
> # User Laurent Charignon <lcharignon@fb.com>
> # Date 1429736190 25200
> #      Wed Apr 22 13:56:30 2015 -0700
> # Branch stable
> # Node ID 827224ee690234eb78e3c12ddcc381879bfba014
> # Parent  1f9127c9239b9c39c676bb09752db1e2ca6c26f7
> record: fix record with change on moved file crashes (issue4619)

> reverting 79fceed67676, add a test to prevent the issue from coming back.

So.. this means we lose the fix to the legitimate bug that was in that
change ("record: allow editing new files (issue4304)"), right? What's
the plan here?
Laurent Charignon - April 23, 2015, 5:11 p.m.
On 4/22/15, 5:22 PM, "Matt Mackall" <mpm@selenic.com> wrote:

>On Wed, 2015-04-22 at 14:57 -0700, Laurent Charignon wrote:
>> # HG changeset patch
>> # User Laurent Charignon <lcharignon@fb.com>
>> # Date 1429736190 25200
>> #      Wed Apr 22 13:56:30 2015 -0700
>> # Branch stable
>> # Node ID 827224ee690234eb78e3c12ddcc381879bfba014
>> # Parent  1f9127c9239b9c39c676bb09752db1e2ca6c26f7
>> record: fix record with change on moved file crashes (issue4619)
>
>> reverting 79fceed67676, add a test to prevent the issue from coming
>>back.
>
>So.. this means we lose the fix to the legitimate bug that was in that
>change ("record: allow editing new files (issue4304)"), right?
Correct
> What's
>the plan here?

Since this issue is a regression and I didn't feel confident getting the
right fix by the end of the week, I preferred rolling back the change that
caused it.
I will reopen issue4304 and work on the correct fix after, if that works
for you.

Thanks,

Laurent


>
>-- 
>Mathematics is the supreme nostalgia of our time.
>
>
Matt Mackall - April 23, 2015, 5:34 p.m.
On Thu, 2015-04-23 at 17:11 +0000, Laurent Charignon wrote:
> 
> On 4/22/15, 5:22 PM, "Matt Mackall" <mpm@selenic.com> wrote:
> 
> >On Wed, 2015-04-22 at 14:57 -0700, Laurent Charignon wrote:
> >> # HG changeset patch
> >> # User Laurent Charignon <lcharignon@fb.com>
> >> # Date 1429736190 25200
> >> #      Wed Apr 22 13:56:30 2015 -0700
> >> # Branch stable
> >> # Node ID 827224ee690234eb78e3c12ddcc381879bfba014
> >> # Parent  1f9127c9239b9c39c676bb09752db1e2ca6c26f7
> >> record: fix record with change on moved file crashes (issue4619)
> >
> >> reverting 79fceed67676, add a test to prevent the issue from coming
> >>back.
> >
> >So.. this means we lose the fix to the legitimate bug that was in that
> >change ("record: allow editing new files (issue4304)"), right?
> Correct
> > What's
> >the plan here?
> 
> Since this issue is a regression and I didn't feel confident getting the
> right fix by the end of the week, I preferred rolling back the change that
> caused it.
> I will reopen issue4304 and work on the correct fix after, if that works
> for you.

Ok, sounds good. I just like to know what the plan is when we're trading
one fix for another. This is queued for stable.
Pierre-Yves David - April 23, 2015, 11:07 p.m.
On 04/23/2015 06:34 PM, Matt Mackall wrote:
> On Thu, 2015-04-23 at 17:11 +0000, Laurent Charignon wrote:
>>
>> On 4/22/15, 5:22 PM, "Matt Mackall" <mpm@selenic.com> wrote:
>>
>>> On Wed, 2015-04-22 at 14:57 -0700, Laurent Charignon wrote:
>>>> # HG changeset patch
>>>> # User Laurent Charignon <lcharignon@fb.com>
>>>> # Date 1429736190 25200
>>>> #      Wed Apr 22 13:56:30 2015 -0700
>>>> # Branch stable
>>>> # Node ID 827224ee690234eb78e3c12ddcc381879bfba014
>>>> # Parent  1f9127c9239b9c39c676bb09752db1e2ca6c26f7
>>>> record: fix record with change on moved file crashes (issue4619)
>>>
>>>> reverting 79fceed67676, add a test to prevent the issue from coming
>>>> back.
>>>
>>> So.. this means we lose the fix to the legitimate bug that was in that
>>> change ("record: allow editing new files (issue4304)"), right?
>> Correct
>>> What's
>>> the plan here?
>>
>> Since this issue is a regression and I didn't feel confident getting the
>> right fix by the end of the week, I preferred rolling back the change that
>> caused it.
>> I will reopen issue4304 and work on the correct fix after, if that works
>> for you.
>
> Ok, sounds good. I just like to know what the plan is when we're trading
> one fix for another. This is queued for stable.

If we "unfix" the new file issue, we should probably mark `commit -i` as 
"experimental" (DEPRECATED) because it is not ready for prime time 
without it.

>
Augie Fackler - April 27, 2015, 8:56 p.m.
On Fri, Apr 24, 2015 at 12:07:18AM +0100, Pierre-Yves David wrote:
>
>
> On 04/23/2015 06:34 PM, Matt Mackall wrote:
> >On Thu, 2015-04-23 at 17:11 +0000, Laurent Charignon wrote:
> >>
> >>On 4/22/15, 5:22 PM, "Matt Mackall" <mpm@selenic.com> wrote:
> >>
> >>>On Wed, 2015-04-22 at 14:57 -0700, Laurent Charignon wrote:
> >>>># HG changeset patch
> >>>># User Laurent Charignon <lcharignon@fb.com>
> >>>># Date 1429736190 25200
> >>>>#      Wed Apr 22 13:56:30 2015 -0700
> >>>># Branch stable
> >>>># Node ID 827224ee690234eb78e3c12ddcc381879bfba014
> >>>># Parent  1f9127c9239b9c39c676bb09752db1e2ca6c26f7
> >>>>record: fix record with change on moved file crashes (issue4619)
> >>>
> >>>>reverting 79fceed67676, add a test to prevent the issue from coming
> >>>>back.
> >>>
> >>>So.. this means we lose the fix to the legitimate bug that was in that
> >>>change ("record: allow editing new files (issue4304)"), right?
> >>Correct
> >>>What's
> >>>the plan here?
> >>
> >>Since this issue is a regression and I didn't feel confident getting the
> >>right fix by the end of the week, I preferred rolling back the change that
> >>caused it.
> >>I will reopen issue4304 and work on the correct fix after, if that works
> >>for you.
> >
> >Ok, sounds good. I just like to know what the plan is when we're trading
> >one fix for another. This is queued for stable.
>
> If we "unfix" the new file issue, we should probably mark `commit -i` as
> "experimental" (DEPRECATED) because it is not ready for prime time without
> it.

This makes sense to me.

>
> >
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - April 27, 2015, 10:41 p.m.
On 04/27/2015 01:56 PM, Augie Fackler wrote:
> On Fri, Apr 24, 2015 at 12:07:18AM +0100, Pierre-Yves David wrote:
>>
>>
>> On 04/23/2015 06:34 PM, Matt Mackall wrote:
>>> On Thu, 2015-04-23 at 17:11 +0000, Laurent Charignon wrote:
>>>>
>>>> On 4/22/15, 5:22 PM, "Matt Mackall" <mpm@selenic.com> wrote:
>>>>
>>>>> On Wed, 2015-04-22 at 14:57 -0700, Laurent Charignon wrote:
>>>>>> # HG changeset patch
>>>>>> # User Laurent Charignon <lcharignon@fb.com>
>>>>>> # Date 1429736190 25200
>>>>>> #      Wed Apr 22 13:56:30 2015 -0700
>>>>>> # Branch stable
>>>>>> # Node ID 827224ee690234eb78e3c12ddcc381879bfba014
>>>>>> # Parent  1f9127c9239b9c39c676bb09752db1e2ca6c26f7
>>>>>> record: fix record with change on moved file crashes (issue4619)
>>>>>
>>>>>> reverting 79fceed67676, add a test to prevent the issue from coming
>>>>>> back.
>>>>>
>>>>> So.. this means we lose the fix to the legitimate bug that was in that
>>>>> change ("record: allow editing new files (issue4304)"), right?
>>>> Correct
>>>>> What's
>>>>> the plan here?
>>>>
>>>> Since this issue is a regression and I didn't feel confident getting the
>>>> right fix by the end of the week, I preferred rolling back the change that
>>>> caused it.
>>>> I will reopen issue4304 and work on the correct fix after, if that works
>>>> for you.
>>>
>>> Ok, sounds good. I just like to know what the plan is when we're trading
>>> one fix for another. This is queued for stable.
>>
>> If we "unfix" the new file issue, we should probably mark `commit -i` as
>> "experimental" (DEPRECATED) because it is not ready for prime time without
>> it.
>
> This makes sense to me

The unfixed bug have apparently been refixed.

.--
Pierre-Yves David

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -59,8 +59,6 @@ 
 def dorecord(ui, repo, commitfunc, cmdsuggest, backupall,
             filterfn, *pats, **opts):
     import merge as mergemod
-    hunkclasses = (crecordmod.uihunk, patch.recordhunk)
-    ishunk = lambda x: isinstance(x, hunkclasses)
 
     if not ui.interactive():
         raise util.Abort(_('running non-interactively, use %s instead') %
@@ -117,12 +115,6 @@ 
             ui.status(_('no changes to record\n'))
             return 0
 
-        newandmodifiedfiles = set()
-        for h in chunks:
-            isnew = h.filename() in status.added
-            if ishunk(h) and isnew and not h in originalchunks:
-                newandmodifiedfiles.add(h.filename())
-
         modified = set(status.modified)
 
         # 2. backup changed files, so we can restore them in the end
@@ -130,8 +122,7 @@ 
         if backupall:
             tobackup = changed
         else:
-            tobackup = [f for f in newfiles
-                        if f in modified or f in newandmodifiedfiles]
+            tobackup = [f for f in newfiles if f in modified]
 
         backups = {}
         if tobackup:
@@ -155,13 +146,11 @@ 
             fp = cStringIO.StringIO()
             for c in chunks:
                 fname = c.filename()
-                if fname in backups or fname in newandmodifiedfiles:
+                if fname in backups:
                     c.write(fp)
             dopatch = fp.tell()
             fp.seek(0)
 
-            [os.unlink(c) for c in newandmodifiedfiles]
-
             # 3a. apply filtered patch to clean repo  (clean)
             if backups:
                 # Equivalent to hg.revert
diff --git a/mercurial/patch.py b/mercurial/patch.py
--- a/mercurial/patch.py
+++ b/mercurial/patch.py
@@ -820,7 +820,7 @@ 
     """
     diffgit_re = re.compile('diff --git a/(.*) b/(.*)$')
     diff_re = re.compile('diff -r .* (.*)$')
-    allhunks_re = re.compile('(?:index|deleted file) ')
+    allhunks_re = re.compile('(?:index|new file|deleted file) ')
     pretty_re = re.compile('(?:new file|deleted file) ')
     special_re = re.compile('(?:index|new|deleted|copy|rename) ')
 
diff --git a/tests/test-commit-interactive-curses.t b/tests/test-commit-interactive-curses.t
--- a/tests/test-commit-interactive-curses.t
+++ b/tests/test-commit-interactive-curses.t
@@ -143,48 +143,10 @@ 
   10
   y
 
-Editing patch of newly added file
-
-  $ cat > editor.sh << '__EOF__'
-  > cat "$1"  | sed "s/first/very/g"  > tt
-  > mv tt  "$1"
-  > __EOF__
-  $ cat > newfile << '__EOF__'
-  > This is the first line
-  > This is the second line
-  > This is the third line
-  > __EOF__
-  $ hg add newfile
-  $ cat <<EOF >testModeCommands
-  > f
-  > KEY_DOWN
-  > KEY_DOWN
-  > KEY_DOWN
-  > e
-  > X
-  > EOF
-  $ HGEDITOR="\"sh\" \"`pwd`/editor.sh\"" hg commit -i -d '23 0' -medit-patch-new
-  $ hg tip
-  changeset:   4:6a0a43e9eff5
-  tag:         tip
-  user:        test
-  date:        Thu Jan 01 00:00:23 1970 +0000
-  summary:     edit-patch-new
-  
-  $ hg cat -r tip newfile
-  This is the very line
-  This is the second line
-  This is the third line
-
-  $ cat newfile
-  This is the first line
-  This is the second line
-  This is the third line
-
 Newly added files can be selected with the curses interface
 
   $ hg update -C .
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ echo "hello" > x
   $ hg add x
   $ cat <<EOF >testModeCommands
@@ -194,10 +156,8 @@ 
   > EOF
   $ hg st
   A x
-  ? editor.sh
   ? testModeCommands
   $ hg commit -i  -m "newly added file" -d "0 0"
   $ hg st
-  ? editor.sh
   ? testModeCommands
 
diff --git a/tests/test-commit-interactive.t b/tests/test-commit-interactive.t
--- a/tests/test-commit-interactive.t
+++ b/tests/test-commit-interactive.t
@@ -229,25 +229,11 @@ 
   $ hg add plain
   $ hg commit -i -d '7 0' -m plain plain<<EOF
   > y
-  > y
   > EOF
   diff --git a/plain b/plain
   new file mode 100644
   examine changes to 'plain'? [Ynesfdaq?] y
   
-  @@ -0,0 +1,10 @@
-  +1
-  +2
-  +3
-  +4
-  +5
-  +6
-  +7
-  +8
-  +9
-  +10
-  record this change to 'plain'? [Ynesfdaq?] y
-  
   $ hg tip -p
   changeset:   7:11fb457c1be4
   tag:         tip
@@ -348,10 +334,6 @@ 
   new file mode 100644
   examine changes to 'plain2'? [Ynesfdaq?] y
   
-  @@ -0,0 +1,1 @@
-  +1
-  record change 2/2 to 'plain2'? [Ynesfdaq?] y
-  
 Modify beginning, trim end, record both, add another file to test
 changes numbering
 
@@ -1395,40 +1377,34 @@ 
   $ export HGUSER
 
 
-Editing patch of newly added file
+Moving files
 
-  $ cat > editor.sh << '__EOF__'
-  > cat "$1"  | sed "s/first/very/g"  > tt
-  > mv tt  "$1"
-  > __EOF__
-  $ cat > newfile << '__EOF__'
-  > This is the first line
-  > This is the second line
-  > This is the third line
-  > __EOF__
-  $ hg add newfile
-  $ HGEDITOR="\"sh\" \"`pwd`/editor.sh\"" hg commit -i -d '23 0' -medit-patch-new <<EOF
+  $ hg update -C .
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg mv plain plain3
+  $ echo somechange >> plain3
+  $ hg commit -i -d '23 0' -mmoving_files << EOF
   > y
-  > e
+  > y
   > EOF
-  diff --git a/newfile b/newfile
-  new file mode 100644
-  examine changes to 'newfile'? [Ynesfdaq?] y
+  diff --git a/plain b/plain3
+  rename from plain
+  rename to plain3
+  1 hunks, 1 lines changed
+  examine changes to 'plain' and 'plain3'? [Ynesfdaq?] y
   
-  @@ -0,0 +1,3 @@
-  +This is the first line
-  +This is the second line
-  +This is the third line
-  record this change to 'newfile'? [Ynesfdaq?] e
+  @@ -11,3 +11,4 @@
+   9
+   10
+   11
+  +somechange
+  record this change to 'plain3'? [Ynesfdaq?] y
   
-  $ hg cat -r tip newfile
-  This is the very line
-  This is the second line
-  This is the third line
-
-  $ cat newfile
-  This is the first line
-  This is the second line
-  This is the third line
-
+  $ hg tip
+  changeset:   30:542e1f362a22
+  tag:         tip
+  user:        test
+  date:        Thu Jan 01 00:00:23 1970 +0000
+  summary:     moving_files
+  
   $ cd ..
diff --git a/tests/test-keyword.t b/tests/test-keyword.t
--- a/tests/test-keyword.t
+++ b/tests/test-keyword.t
@@ -479,12 +479,6 @@ 
   new file mode 100644
   examine changes to 'r'? [Ynesfdaq?] y
   
-  @@ -0,0 +1,1 @@
-  +$Id$
-  record this change to 'r'? [Ynesfdaq?] y
-  
-  resolving manifests
-  patching file r
   committing files:
   r
   committing manifest
@@ -513,12 +507,6 @@ 
   new file mode 100644
   examine changes to 'i'? [Ynesfdaq?] y
   
-  @@ -0,0 +1,1 @@
-  +$Id$
-  record this change to 'i'? [Ynesfdaq?] y
-  
-  resolving manifests
-  patching file i
   committing files:
   i
   committing manifest
diff --git a/tests/test-mq-subrepo.t b/tests/test-mq-subrepo.t
--- a/tests/test-mq-subrepo.t
+++ b/tests/test-mq-subrepo.t
@@ -295,11 +295,6 @@ 
   new file mode 100644
   examine changes to '.hgsub'? [Ynesfdaq?] y
   
-  @@ -0,0 +1,1 @@
-  +sub = sub
-  record this change to '.hgsub'? [Ynesfdaq?] y
-  
-  warning: subrepo spec file '.hgsub' not found
   abort: uncommitted changes in subrepository 'sub'
   [255]
   % update substate when adding .hgsub w/clean updated subrepo
@@ -309,11 +304,6 @@ 
   new file mode 100644
   examine changes to '.hgsub'? [Ynesfdaq?] y
   
-  @@ -0,0 +1,1 @@
-  +sub = sub
-  record this change to '.hgsub'? [Ynesfdaq?] y
-  
-  warning: subrepo spec file '.hgsub' not found
   path sub
    source   sub
    revision b2fdb12cd82b021c3b7053d67802e77b6eeaee31