Patchwork memctx: fix manifest for removed files (issue4470)

login
register
mail settings
Submitter Augie Fackler
Date Dec. 16, 2014, 3:43 p.m.
Message ID <40bd85091fb37fc992e3.1418744601@augie-macbookair2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/7123/
State Accepted
Delegated to: Pierre-Yves David
Headers show

Comments

Augie Fackler - Dec. 16, 2014, 3:43 p.m.
# HG changeset patch
# User Augie Fackler <augie@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)

filectxfn returns None for removed files, so we have to check for None
before computing the new file content hash for the manifest.

Includes a test that proves this works, by demonstrating that we can
show the diff of an amended commit in the committemplate.
Jordi Gutiérrez Hermoso - Dec. 16, 2014, 7:39 p.m.
On Tue, 2014-12-16 at 10:43 -0500, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@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.
Pierre-Yves David - Dec. 16, 2014, 11:33 p.m.
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@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.
Katsunori FUJIWARA - Dec. 17, 2014, 6:14 a.m.
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@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)


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

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Sean Farley - Dec. 17, 2014, 6:58 a.m.
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@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.
Pierre-Yves David - Dec. 17, 2014, 10:09 a.m.
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@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.
Katsunori FUJIWARA - Dec. 17, 2014, 11:52 a.m.
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@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@lares.dti.ne.jp
Augie Fackler - Dec. 17, 2014, 4:38 p.m.
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@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.

>
> If someone can confirm my diagnostic that no adjustment are needed I'll
> happily push it.
>
> --
> Pierre-Yves David
Pierre-Yves David - Dec. 17, 2014, 7:44 p.m.
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@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.
Katsunori FUJIWARA - Dec. 18, 2014, 8:46 a.m.
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@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@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Pierre-Yves David - Dec. 18, 2014, 8:47 a.m.
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@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.

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -1630,9 +1630,10 @@  class memctx(committablectx):
 
         # keep this simple for now; just worry about p1
         pctx = self._parents[0]
+        pman = pctx.manifest()
         man = pctx.manifest().copy()
 
-        for f, fnode in man.iteritems():
+        for f, fnode in pman.iteritems():
             p1node = nullid
             p2node = nullid
             p = pctx[f].parents() # if file isn't in pctx, check p2?
@@ -1640,7 +1641,12 @@  class memctx(committablectx):
                 p1node = p[0].node()
                 if len(p) > 1:
                     p2node = p[1].node()
-            man[f] = revlog.hash(self[f].data(), p1node, p2node)
+            fctx = self[f]
+            if fctx is None:
+                # removed file
+                del man[f]
+            else:
+                man[f] = revlog.hash(fctx.data(), p1node, p2node)
 
         return man
 
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
   abort: empty commit message
   [255]
 
+prove that we can show a diff of an amend using committemplate:
+
+  $ hg init lol
+  $ cd lol
+  $ cat >> .hg/hgrc <<EOF
+  > [committemplate]
+  > changeset = {desc}\n\n
+  >      HG: {extramsg}
+  >      HG: user: {author}\n{ifeq(p2rev, "-1", "",
+  >     "HG: branch merge\n")
+  >     }HG: branch '{branch}'\n{if(currentbookmark,
+  >     "HG: bookmark '{currentbookmark}'\n")  }{subrepos %
+  >     "HG: subrepo {subrepo}\n"              }
+  >     {splitlines(diff()) % 'HG: {line}\n'}
+  > EOF
+  $ echo a > a
+  $ echo b > b
+  $ hg addr
+  adding a
+  adding b
+  $ hg ci -m 'init'
+  $ hg rm b
+  $ hg ci -m 'rm b'
+  $ hg export .
+  # HG changeset patch
+  # User test
+  # Date 0 0
+  #      Thu Jan 01 00:00:00 1970 +0000
+  # Node ID 88d0ffa85e7a92ccc7c9cc187f9b17858bd206a7
+  # Parent  9118d25c26b1ca5cab5683b02100e7eb2c0d9471
+  rm b
+  
+  diff -r 9118d25c26b1 -r 88d0ffa85e7a b
+  --- a/b	Thu Jan 01 00:00:00 1970 +0000
+  +++ /dev/null	Thu Jan 01 00:00:00 1970 +0000
+  @@ -1,1 +0,0 @@
+  -b
+  $ echo a >> a
+  $ HGEDITOR=cat hg commit --amend
+  rm b
+  
+  
+  HG: Leave message empty to abort commit.
+  HG: user: test
+  HG: branch 'default'
+  
+  HG: diff -r 9118d25c26b1 a
+  HG: --- a/a	Thu Jan 01 00:00:00 1970 +0000
+  HG: +++ b/a	Thu Jan 01 00:00:00 1970 +0000
+  HG: @@ -1,1 +1,2 @@
+  HG:  a
+  HG: +a
+  HG: diff -r 9118d25c26b1 b
+  HG: --- a/b	Thu Jan 01 00:00:00 1970 +0000
+  HG: +++ /dev/null	Thu Jan 01 00:00:00 1970 +0000
+  HG: @@ -1,1 +0,0 @@
+  HG: -b
+  saved backup bundle to $TESTTMP/*/*-amend-backup.hg (glob)
+  $ cd ..
+  $ rm -r lol
+
+cleanup
   $ cat >> .hg/hgrc <<EOF
   > # disable customizing for subsequent tests
   > [committemplate]