Patchwork [1,of,2] manifest: allow specifying the revlog filename

login
register
mail settings
Submitter Durham Goode
Date Feb. 24, 2017, 10:44 p.m.
Message ID <ed987b24e755a4c61c00.1487976241@dev111.prn1.facebook.com>
Download mbox | patch
Permalink /patch/18768/
State Superseded
Delegated to: Martin von Zweigbergk
Headers show

Comments

Durham Goode - Feb. 24, 2017, 10:44 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1487973639 28800
#      Fri Feb 24 14:00:39 2017 -0800
# Node ID ed987b24e755a4c61c006f7d98a4ff370229faad
# Parent  f01df5d2fe493376a67569756a9bc2ddffa5cd81
manifest: allow specifying the revlog filename

Previously we had hardcoded the manifest filename to be 00manifest.i. In our
external treemanifest extension, we want to allow writing a treemanifest side by
side with a flat manifest, so we need to be able to store the root revisions at
a different location (in our extension we use 00manifesttree.i).

This patches moves the revlog name to a parameter so we can adjust it.
via Mercurial-devel - Feb. 28, 2017, 1:10 a.m.
+mercurial-devel again (I had initially thought my comment was not
about the patch, but it turns out it was)

On Sat, Feb 25, 2017 at 3:23 PM, Durham Goode <durham@fb.com> wrote:
> On 2/24/17 10:00 PM, Martin von Zweigbergk wrote:
>>
>> -list
>>
>> On Fri, Feb 24, 2017, 14:44 Durham Goode <durham@fb.com
>> <mailto:durham@fb.com>> wrote:
>>
>>     # HG changeset patch
>>     # User Durham Goode <durham@fb.com <mailto:durham@fb.com>>
>>     # Date 1487973639 28800
>>     #      Fri Feb 24 14:00:39 2017 -0800
>>     # Node ID ed987b24e755a4c61c006f7d98a4ff370229faad
>>     # Parent  f01df5d2fe493376a67569756a9bc2ddffa5cd81
>>     manifest: allow specifying the revlog filename
>>
>>     Previously we had hardcoded the manifest filename to be
>>     00manifest.i. In our
>>     external treemanifest extension, we want to allow writing a
>>     treemanifest side by
>>     side with a flat manifest, so we need to be able to store the root
>>     revisions at
>>     a different location (in our extension we use 00manifesttree.i).
>>
>>
>> Why not meta/00manifest.i? That would be more consistent. I let them
>> (flat and tree) share a revlog only to allow for transition between
>> them. But if you don't care about that, meta/00manifest.i sounds like
>> the obvious choice.
>
>
> I'd be fine with that too, but I'm not sure how we'd go about that. I guess
> I could put an option on the opener that the manifest constructor checks
> for, like 'separatetreeroot=True' and then the constructor would know to put
> it in meta/.  Seem reasonable?

Or you could keep your new indexfile argument to the constructor and
use that if it was set and otherwise use the current way of
calculating it. Your extension could then pass 'meta/00manifest.i' for
dir=='' and None for all others. That gives more flexibility if you
want to use some other scheme too. Also, I'd like a comment explaining
that the parameter is for extensions, so the next person doesn't
remove it.
Durham Goode - Feb. 28, 2017, 5:49 p.m.
On 2/27/17 5:10 PM, Martin von Zweigbergk wrote:
> +mercurial-devel again (I had initially thought my comment was not
> about the patch, but it turns out it was)
>
> On Sat, Feb 25, 2017 at 3:23 PM, Durham Goode <durham@fb.com> wrote:
>> On 2/24/17 10:00 PM, Martin von Zweigbergk wrote:
>>>
>>> -list
>>>
>>> On Fri, Feb 24, 2017, 14:44 Durham Goode <durham@fb.com
>>> <mailto:durham@fb.com>> wrote:
>>>
>>>     # HG changeset patch
>>>     # User Durham Goode <durham@fb.com <mailto:durham@fb.com>>
>>>     # Date 1487973639 28800
>>>     #      Fri Feb 24 14:00:39 2017 -0800
>>>     # Node ID ed987b24e755a4c61c006f7d98a4ff370229faad
>>>     # Parent  f01df5d2fe493376a67569756a9bc2ddffa5cd81
>>>     manifest: allow specifying the revlog filename
>>>
>>>     Previously we had hardcoded the manifest filename to be
>>>     00manifest.i. In our
>>>     external treemanifest extension, we want to allow writing a
>>>     treemanifest side by
>>>     side with a flat manifest, so we need to be able to store the root
>>>     revisions at
>>>     a different location (in our extension we use 00manifesttree.i).
>>>
>>>
>>> Why not meta/00manifest.i? That would be more consistent. I let them
>>> (flat and tree) share a revlog only to allow for transition between
>>> them. But if you don't care about that, meta/00manifest.i sounds like
>>> the obvious choice.
>>
>>
>> I'd be fine with that too, but I'm not sure how we'd go about that. I guess
>> I could put an option on the opener that the manifest constructor checks
>> for, like 'separatetreeroot=True' and then the constructor would know to put
>> it in meta/.  Seem reasonable?
>
> Or you could keep your new indexfile argument to the constructor and
> use that if it was set and otherwise use the current way of
> calculating it. Your extension could then pass 'meta/00manifest.i' for
> dir=='' and None for all others. That gives more flexibility if you
> want to use some other scheme too. Also, I'd like a comment explaining
> that the parameter is for extensions, so the next person doesn't
> remove it.

I can add a comment.  I think putting directories in the indexfile 
string might be a bit funky since I'm not sure if other parts of the 
code rely on self.indexfile to be just a filename.  I guess I can play 
with that after it lands.
via Mercurial-devel - Feb. 28, 2017, 5:53 p.m.
On Tue, Feb 28, 2017 at 9:49 AM, Durham Goode <durham@fb.com> wrote:
>
>
> On 2/27/17 5:10 PM, Martin von Zweigbergk wrote:
>>
>> +mercurial-devel again (I had initially thought my comment was not
>> about the patch, but it turns out it was)
>>
>> On Sat, Feb 25, 2017 at 3:23 PM, Durham Goode <durham@fb.com> wrote:
>>>
>>> On 2/24/17 10:00 PM, Martin von Zweigbergk wrote:
>>>>
>>>>
>>>> -list
>>>>
>>>> On Fri, Feb 24, 2017, 14:44 Durham Goode <durham@fb.com
>>>> <mailto:durham@fb.com>> wrote:
>>>>
>>>>     # HG changeset patch
>>>>     # User Durham Goode <durham@fb.com <mailto:durham@fb.com>>
>>>>     # Date 1487973639 28800
>>>>     #      Fri Feb 24 14:00:39 2017 -0800
>>>>     # Node ID ed987b24e755a4c61c006f7d98a4ff370229faad
>>>>     # Parent  f01df5d2fe493376a67569756a9bc2ddffa5cd81
>>>>     manifest: allow specifying the revlog filename
>>>>
>>>>     Previously we had hardcoded the manifest filename to be
>>>>     00manifest.i. In our
>>>>     external treemanifest extension, we want to allow writing a
>>>>     treemanifest side by
>>>>     side with a flat manifest, so we need to be able to store the root
>>>>     revisions at
>>>>     a different location (in our extension we use 00manifesttree.i).
>>>>
>>>>
>>>> Why not meta/00manifest.i? That would be more consistent. I let them
>>>> (flat and tree) share a revlog only to allow for transition between
>>>> them. But if you don't care about that, meta/00manifest.i sounds like
>>>> the obvious choice.
>>>
>>>
>>>
>>> I'd be fine with that too, but I'm not sure how we'd go about that. I
>>> guess
>>> I could put an option on the opener that the manifest constructor checks
>>> for, like 'separatetreeroot=True' and then the constructor would know to
>>> put
>>> it in meta/.  Seem reasonable?
>>
>>
>> Or you could keep your new indexfile argument to the constructor and
>> use that if it was set and otherwise use the current way of
>> calculating it. Your extension could then pass 'meta/00manifest.i' for
>> dir=='' and None for all others. That gives more flexibility if you
>> want to use some other scheme too. Also, I'd like a comment explaining
>> that the parameter is for extensions, so the next person doesn't
>> remove it.
>
>
> I can add a comment.  I think putting directories in the indexfile string
> might be a bit funky since I'm not sure if other parts of the code rely on
> self.indexfile to be just a filename.

I don't follow. You patch touches a line that says

  indexfile = "meta/" + dir + "00manifest.i"

so it already (potentially) has a directory in it. Or do you mean
something else?

>  I guess I can play with that after it
> lands.
Durham Goode - Feb. 28, 2017, 7:44 p.m.
On 2/28/17 9:53 AM, Martin von Zweigbergk wrote:
> On Tue, Feb 28, 2017 at 9:49 AM, Durham Goode <durham@fb.com> wrote:
>>
>>
>> On 2/27/17 5:10 PM, Martin von Zweigbergk wrote:
>>>
>>> +mercurial-devel again (I had initially thought my comment was not
>>> about the patch, but it turns out it was)
>>>
>>> On Sat, Feb 25, 2017 at 3:23 PM, Durham Goode <durham@fb.com> wrote:
>>>>
>>>> On 2/24/17 10:00 PM, Martin von Zweigbergk wrote:
>>>>>
>>>>>
>>>>> -list
>>>>>
>>>>> On Fri, Feb 24, 2017, 14:44 Durham Goode <durham@fb.com
>>>>> <mailto:durham@fb.com>> wrote:
>>>>>
>>>>>     # HG changeset patch
>>>>>     # User Durham Goode <durham@fb.com <mailto:durham@fb.com>>
>>>>>     # Date 1487973639 28800
>>>>>     #      Fri Feb 24 14:00:39 2017 -0800
>>>>>     # Node ID ed987b24e755a4c61c006f7d98a4ff370229faad
>>>>>     # Parent  f01df5d2fe493376a67569756a9bc2ddffa5cd81
>>>>>     manifest: allow specifying the revlog filename
>>>>>
>>>>>     Previously we had hardcoded the manifest filename to be
>>>>>     00manifest.i. In our
>>>>>     external treemanifest extension, we want to allow writing a
>>>>>     treemanifest side by
>>>>>     side with a flat manifest, so we need to be able to store the root
>>>>>     revisions at
>>>>>     a different location (in our extension we use 00manifesttree.i).
>>>>>
>>>>>
>>>>> Why not meta/00manifest.i? That would be more consistent. I let them
>>>>> (flat and tree) share a revlog only to allow for transition between
>>>>> them. But if you don't care about that, meta/00manifest.i sounds like
>>>>> the obvious choice.
>>>>
>>>>
>>>>
>>>> I'd be fine with that too, but I'm not sure how we'd go about that. I
>>>> guess
>>>> I could put an option on the opener that the manifest constructor checks
>>>> for, like 'separatetreeroot=True' and then the constructor would know to
>>>> put
>>>> it in meta/.  Seem reasonable?
>>>
>>>
>>> Or you could keep your new indexfile argument to the constructor and
>>> use that if it was set and otherwise use the current way of
>>> calculating it. Your extension could then pass 'meta/00manifest.i' for
>>> dir=='' and None for all others. That gives more flexibility if you
>>> want to use some other scheme too. Also, I'd like a comment explaining
>>> that the parameter is for extensions, so the next person doesn't
>>> remove it.
>>
>>
>> I can add a comment.  I think putting directories in the indexfile string
>> might be a bit funky since I'm not sure if other parts of the code rely on
>> self.indexfile to be just a filename.
>
> I don't follow. You patch touches a line that says
>
>   indexfile = "meta/" + dir + "00manifest.i"
>
> so it already (potentially) has a directory in it. Or do you mean
> something else?

You're right, so that's not a big deal I guess.

Patch

diff --git a/mercurial/manifest.py b/mercurial/manifest.py
--- a/mercurial/manifest.py
+++ b/mercurial/manifest.py
@@ -1132,7 +1132,8 @@  class manifestrevlog(revlog.revlog):
     '''A revlog that stores manifest texts. This is responsible for caching the
     full-text manifest contents.
     '''
-    def __init__(self, opener, dir='', dirlogcache=None):
+    def __init__(self, opener, dir='', dirlogcache=None,
+                 indexfile='00manifest.i'):
         # During normal operations, we expect to deal with not more than four
         # revs at a time (such as during commit --amend). When rebasing large
         # stacks of commits, the number can go up, hence the config knob below.
@@ -1150,12 +1151,11 @@  class manifestrevlog(revlog.revlog):
 
         self._fulltextcache = util.lrucachedict(cachesize)
 
-        indexfile = "00manifest.i"
         if dir:
             assert self._treeondisk, 'opts is %r' % opts
             if not dir.endswith('/'):
                 dir = dir + '/'
-            indexfile = "meta/" + dir + "00manifest.i"
+            indexfile = "meta/" + dir + indexfile
         self._dir = dir
         # The dirlogcache is kept on the root manifest log
         if dir: