[PATCH] memctx: fix manifest for removed files (issue4470)

FUJIWARA Katsunori foozy at lares.dti.ne.jp
Thu Dec 18 02:46:04 CST 2014


At Wed, 17 Dec 2014 11:44:38 -0800,
Pierre-Yves David wrote:
> 
> 
> 
> On 12/17/2014 08:38 AM, Augie Fackler wrote:
> > On Wed, Dec 17, 2014 at 02:09:33AM -0800, Pierre-Yves David wrote:
> >>
> >>
> >> On 12/16/2014 10:58 PM, Sean Farley wrote:
> >>>
> >>> FUJIWARA Katsunori writes:
> >>>
> >>>> At Tue, 16 Dec 2014 15:33:11 -0800,
> >>>> Pierre-Yves David wrote:
> >>>>>
> >>>>> On 12/16/2014 11:39 AM, Jordi GutiƩrrez Hermoso wrote:
> >>>>>> On Tue, 2014-12-16 at 10:43 -0500, Augie Fackler wrote:
> >>>>>>> # HG changeset patch
> >>>>>>> # User Augie Fackler <augie at google.com>
> >>>>>>> # Date 1418673654 18000
> >>>>>>> #      Mon Dec 15 15:00:54 2014 -0500
> >>>>>>> # Node ID 40bd85091fb37fc992e3af2128a8ed31f31a47ff
> >>>>>>> # Parent  65c854f92d6ba8861414ff3191182fba28777a82
> >>>>>>> memctx: fix manifest for removed files (issue4470)
> >>>>>>
> >>>>>> Nice! Thanks for the fix!
> >>>>>>
> >>>>>>> diff --git a/tests/test-commit.t b/tests/test-commit.t
> >>>>>>> --- a/tests/test-commit.t
> >>>>>>> +++ b/tests/test-commit.t
> >>>>>>> @@ -429,6 +429,68 @@ specific template keywords work well
> >>>>>> [snip]
> >>>>>>> +  $ cd ..
> >>>>>>> +  $ rm -r lol
> >>>>>>
> >>>>>> Maybe remove this last `rm -r` from the test? I had it in the original
> >>>>>> repro, but it seems out of place in the test. It's useful to keep the
> >>>>>> repos in the tests when doing run-tests.py --keep-tmpdir.
> >>>>>>
> >>>>>> Also, perhaps a better name than "lol" should be used here? I'm kind
> >>>>>> of cavalier about my metasyntactic variables.
> >>>>>
> >>>>> I've renamed "lol" to issue4470, dropped the rm and pushted the result
> >>>>> to the clowncopter.
> >>>>
> >>>> Oh! I also just finished patches to fix this issue !
> >>>>
> >>>> Even though it may be too late, I post my patches for reference :-)
> >>>>
> >>>> In addition to fixing issue4470, my series also fixes "missing newly
> >>>> added files in the manifest" issue of memctx (by #2 of series)
> >>>
> >>> Wow, this is much more robust and helps with issues when trying to use
> >>> memctx in the merge machinery (I have a half-baked version of what
> >>> Katsunori wrote). Please queue this one if not too late.
> >>
> >> The foozy series looks really good and I would be very happy to queue it.
> >> But I wonder how it does related with the Augie one? I feel like they are
> >> orthogonal as:
> >> - Augie make a magic value explicit (which is good),
> >
> > I'm not doing much of that in my code?
> >
> >> - Foozy make memory status more correct.
> >
> > ...and then uses it to avoid iterating over the entire manifest, which
> > is a huge win.
> >
> > My feeling is that foozy's change may be a bit big for stable, but
> > that it's the better long-term fix. If we want a fix for this on
> > stable (might be nice, but I didn't consider it), mine may be more
> > suitable. I defer to you and mpm on that.
> >
> > foozy's patches should definitely replace mine for default. It's a
> > better solution by far.
> It seem to me that you patch are otogonal because foozy does nto replace 
> your patch. And you patch still sound a net positive.
> 
> So I've pushed you patch on stable and foozy's on default.

According to current clowncopter log, the 1st hunk below of my patch
#3/3 was dropped, (maybe, to avoid conflict against Augie's work).

    ====================
             pctx = self._parents[0]
             man = pctx.manifest().copy()
     
    -        for f, fnode in man.iteritems():
    +        for f in self._status.modified:
                 p1node = nullid
                 p2node = nullid
                 p = pctx[f].parents() # if file isn't in pctx, check p2?
    ====================

But, with Augie's work, the 2nd hunk below itself doesn't do anything
for improvement and is redundant, because:

  - "removed" files should be included in the manifest of the parent, and

  - they are scanned and dropped from the manifest in the 1st loop (by
    Augie's work), then

  - they aren't included in the manifest at the loop below

    ====================
             for f in self._status.added:
                 man[f] = revlog.hash(self[f].data(), nullid, nullid)
     
    +        for f in self._status.removed:
    +            if f in man:
    +                del man[f]
    +
             return man
    ====================

Please drop #3/3 of my series from clowncopter. I'll post revised one
suitable for Augie's.


> -- 
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy at lares.dti.ne.jp


More information about the Mercurial-devel mailing list