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