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

FUJIWARA Katsunori foozy at lares.dti.ne.jp
Wed Dec 17 05:52:16 CST 2014


At Wed, 17 Dec 2014 02:09:33 -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),
> - Foozy make memory status more correct.
> 
> If someone can confirm my diagnostic that no adjustment are needed I'll 
> happily push it.
> 
> -- 
> Pierre-Yves David

Core of Augie's patch is avoiding manifest hash calculation and
deleting a entry from manifest, if "self[file]" returns None (=
meaning "file is removed")

    ================
    +            fctx = self[f]
    +            if fctx is None:
    +                # removed file
    +                del man[f]
    +            else:
    +                man[f] = revlog.hash(fctx.data(), p1node, p2node)
    ================

On the other hand, my series achieves this in two steps:

  - calculate "memctx._status" correctly (patch #1/3)

    issue 4470 can't be resolved only with this change, but other
    patches rely on correctness of "memctx._status".


  - maintain manifest for "modified" and "removed" separately (patch #3/3)

    At first, manifest entries for "_status.modified" files are
    updated. This can avoid manifest hash calculation for removed
    files, because removed files shouldn't be listed up in
    "_status.modified" (and this also reduces loop cost for large
    manifest).

        ================
        -        for f, fnode in man.iteritems():
        +        for f in self._status.modified:
                     p1node = nullid
                     p2node = nullid
                      ....
                     man[f] = revlog.hash(self[f].data(), nullid, nullid)
        ================

    Then, manifest entries for "_status.removed" files are deleted.

        ================
        +        for f in self._status.removed:
        +            if f in man:
        +                del man[f]
        +
                 return man
        ================


In addition to it, "memctx._manifest" has another issue that newly
added files aren't included.

So, patch #2/3 adds manifest entries for "_status.added" files.

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

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


More information about the Mercurial-devel mailing list