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

[Pierre-Yves David]

On 12/18/2014 12:46 AM, FUJIWARA Katsunori wrote:
>
> 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).

There was some fuzz when applying the patch. This may have messed 
something up.

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

Okay, eagerly waiting for it.

-- 
Pierre-Yves David