| Submitter | Phillip Cohen |
|---|---|
| Date | June 27, 2017, 6:03 a.m. |
| Message ID | <016ce3d8fee0968dd308.1498543397@phillco-mbp.dhcp.thefacebook.com> |
| Download | mbox | patch |
| Permalink | /patch/21758/ |
| State | Accepted |
| Headers | show |
Comments
> I discussed with Sidd about just having the getters raise RuntimeErrors after a mutator has been called, but we actually call isabsent() in merge.py after running the internal merge tools. It looks like in the test suite, we only call isabsent() after calling remove(). So perhaps a reasonable option is to make calling write() on an absentfilectx marked it as non-absent (and cause flags(), size(), data(), e.g. to pass-through to the underlying filectx). Calling remove() would mark it as absent, if it wasn't. It would default to absent. Then, we can do the writes on the absentcontext instance (which feels better than going around it and writing directly to the working copy) without breaking any existing behavior. (Also, hooray, I reinvented Optional[] :P) On Mon, Jun 26, 2017 at 11:03 PM, Phil Cohen <phillco@fb.com> wrote: > # HG changeset patch > # User Phil Cohen <phillco@fb.com> > # Date 1498542735 25200 > # Mon Jun 26 22:52:15 2017 -0700 > # Node ID 016ce3d8fee0968dd30894bf78c3812a19a240d9 > # Parent d2f2b5a60476e18e69fdcd76ac296d37bb69b112 > filemerge: convert a couple of wvfs calls in internal mergetools to contexts > > One hitch is that sometimes fcd is actually an absentfilectx which does not > expose any mutator functions. In order to still use the context functions, > we look up the underlying workingfilectx to perform the write there. > > One alternate way would be to put the write functions on the absentfilectx and > have them pass-through. While this makes the callsites cleaner, we would need > to decide what its getter functions would return after this point, since > returning None for `data` (and True for `isabsent()`) might no longer be > correct after a write. I discussed with Sidd about just having the getters > raise RuntimeErrors after a mutator has been called, but we actually call > isabsent() in merge.py after running the internal merge tools. > > diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py > --- a/mercurial/filemerge.py > +++ b/mercurial/filemerge.py > @@ -298,10 +298,10 @@ > """Uses the other `p2()` version of files as the merged version.""" > if fco.isabsent(): > # local changed, remote deleted -- 'deleted' picked > - repo.wvfs.unlinkpath(fcd.path()) > + _underlyingfctxifabsent(fcd).remove() > deleted = True > else: > - repo.wwrite(fcd.path(), fco.data(), fco.flags()) > + _underlyingfctxifabsent(fcd).write(fco.data(), fco.flags()) > deleted = False > return 0, deleted > > @@ -313,9 +313,19 @@ > used to resolve these conflicts.""" > # for change/delete conflicts write out the changed version, then fail > if fcd.isabsent(): > - repo.wwrite(fcd.path(), fco.data(), fco.flags()) > + _underlyingfctxifabsent(fcd).write(fco.data(), fco.flags()) > return 1, False > > +def _underlyingfctxifabsent(filectx): > + """Sometimes when resolving, our fcd is actually an absentfilectx, but > + we want to write to it (to do the resolve). This helper returns the > + underyling workingfilectx in that case. > + """ > + if filectx.isabsent(): > + return filectx.changectx()[filectx.path()] > + else: > + return filectx > + > def _premerge(repo, fcd, fco, fca, toolconf, files, labels=None): > tool, toolpath, binary, symlink = toolconf > if symlink or fcd.isabsent() or fco.isabsent(): > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> It looks like in the test suite, we only call isabsent() after calling > remove(). I was wrong. The existing merge code relies on isabsent() still returning True after writing to that path in the working copy. So, I think this patch is probably the best we can do for now without over-complicating things. On Mon, Jun 26, 2017 at 11:12 PM, Phillip Cohen <phillip@phillip.io> wrote: >> I discussed with Sidd about just having the getters > raise RuntimeErrors after a mutator has been called, but we actually call > isabsent() in merge.py after running the internal merge tools. > > It looks like in the test suite, we only call isabsent() after calling > remove(). So perhaps a reasonable option is to make calling write() on > an absentfilectx marked it as non-absent (and cause flags(), size(), > data(), e.g. to pass-through to the underlying filectx). Calling > remove() would mark it as absent, if it wasn't. It would default to > absent. Then, we can do the writes on the absentcontext instance > (which feels better than going around it and writing directly to the > working copy) without breaking any existing behavior. > > (Also, hooray, I reinvented Optional[] :P) > > On Mon, Jun 26, 2017 at 11:03 PM, Phil Cohen <phillco@fb.com> wrote: >> # HG changeset patch >> # User Phil Cohen <phillco@fb.com> >> # Date 1498542735 25200 >> # Mon Jun 26 22:52:15 2017 -0700 >> # Node ID 016ce3d8fee0968dd30894bf78c3812a19a240d9 >> # Parent d2f2b5a60476e18e69fdcd76ac296d37bb69b112 >> filemerge: convert a couple of wvfs calls in internal mergetools to contexts >> >> One hitch is that sometimes fcd is actually an absentfilectx which does not >> expose any mutator functions. In order to still use the context functions, >> we look up the underlying workingfilectx to perform the write there. >> >> One alternate way would be to put the write functions on the absentfilectx and >> have them pass-through. While this makes the callsites cleaner, we would need >> to decide what its getter functions would return after this point, since >> returning None for `data` (and True for `isabsent()`) might no longer be >> correct after a write. I discussed with Sidd about just having the getters >> raise RuntimeErrors after a mutator has been called, but we actually call >> isabsent() in merge.py after running the internal merge tools. >> >> diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py >> --- a/mercurial/filemerge.py >> +++ b/mercurial/filemerge.py >> @@ -298,10 +298,10 @@ >> """Uses the other `p2()` version of files as the merged version.""" >> if fco.isabsent(): >> # local changed, remote deleted -- 'deleted' picked >> - repo.wvfs.unlinkpath(fcd.path()) >> + _underlyingfctxifabsent(fcd).remove() >> deleted = True >> else: >> - repo.wwrite(fcd.path(), fco.data(), fco.flags()) >> + _underlyingfctxifabsent(fcd).write(fco.data(), fco.flags()) >> deleted = False >> return 0, deleted >> >> @@ -313,9 +313,19 @@ >> used to resolve these conflicts.""" >> # for change/delete conflicts write out the changed version, then fail >> if fcd.isabsent(): >> - repo.wwrite(fcd.path(), fco.data(), fco.flags()) >> + _underlyingfctxifabsent(fcd).write(fco.data(), fco.flags()) >> return 1, False >> >> +def _underlyingfctxifabsent(filectx): >> + """Sometimes when resolving, our fcd is actually an absentfilectx, but >> + we want to write to it (to do the resolve). This helper returns the >> + underyling workingfilectx in that case. >> + """ >> + if filectx.isabsent(): >> + return filectx.changectx()[filectx.path()] >> + else: >> + return filectx >> + >> def _premerge(repo, fcd, fco, fca, toolconf, files, labels=None): >> tool, toolpath, binary, symlink = toolconf >> if symlink or fcd.isabsent() or fco.isabsent(): >> _______________________________________________ >> Mercurial-devel mailing list >> Mercurial-devel@mercurial-scm.org >> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
On Mon, 26 Jun 2017 23:03:17 -0700, Phil Cohen wrote: > # HG changeset patch > # User Phil Cohen <phillco@fb.com> > # Date 1498542735 25200 > # Mon Jun 26 22:52:15 2017 -0700 > # Node ID 016ce3d8fee0968dd30894bf78c3812a19a240d9 > # Parent d2f2b5a60476e18e69fdcd76ac296d37bb69b112 > filemerge: convert a couple of wvfs calls in internal mergetools to contexts [...] > +def _underlyingfctxifabsent(filectx): > + """Sometimes when resolving, our fcd is actually an absentfilectx, but > + we want to write to it (to do the resolve). This helper returns the > + underyling workingfilectx in that case. > + """ > + if filectx.isabsent(): > + return filectx.changectx()[filectx.path()] > + else: > + return filectx I think lesser abstraction is good if we have no better alternative. Queued, thanks.
Patch
diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py --- a/mercurial/filemerge.py +++ b/mercurial/filemerge.py @@ -298,10 +298,10 @@ """Uses the other `p2()` version of files as the merged version.""" if fco.isabsent(): # local changed, remote deleted -- 'deleted' picked - repo.wvfs.unlinkpath(fcd.path()) + _underlyingfctxifabsent(fcd).remove() deleted = True else: - repo.wwrite(fcd.path(), fco.data(), fco.flags()) + _underlyingfctxifabsent(fcd).write(fco.data(), fco.flags()) deleted = False return 0, deleted @@ -313,9 +313,19 @@ used to resolve these conflicts.""" # for change/delete conflicts write out the changed version, then fail if fcd.isabsent(): - repo.wwrite(fcd.path(), fco.data(), fco.flags()) + _underlyingfctxifabsent(fcd).write(fco.data(), fco.flags()) return 1, False +def _underlyingfctxifabsent(filectx): + """Sometimes when resolving, our fcd is actually an absentfilectx, but + we want to write to it (to do the resolve). This helper returns the + underyling workingfilectx in that case. + """ + if filectx.isabsent(): + return filectx.changectx()[filectx.path()] + else: + return filectx + def _premerge(repo, fcd, fco, fca, toolconf, files, labels=None): tool, toolpath, binary, symlink = toolconf if symlink or fcd.isabsent() or fco.isabsent():