Re: [gentoo-portage-dev] [PATCH] Add --sync-submodule <glsa|news|profiles> (534070)

[Zac Medico <zmedico@gentoo.org>]

On 01/06/2015 08:53 PM, Brian Dolbec wrote:
> On Wed, 31 Dec 2014 00:07:19 -0800
> Zac Medico <zmedico@gentoo.org> wrote:
> 
>> This adds support for a new --sync-submodule option to both emerge and
>> emaint. When this option is used with the sync action, only the
>> selected submodules are synced. Each submodule is referenced using an
>> abstract identifier, which serves to hide the implementation details
>> involving the precise locations of specific submodules within each
>> repository.
>>
>> Currently, --sync-submodule has no effect for sync protocols other
>> than rsync, but the new SyncBase._get_submodule_paths() method will
>> be useful for implementing support in other SyncBase subclasses.
>>
>> X-Gentoo-Bug: 534070
>> X-Gentoo-Bug-URL: https://bugs.gentoo.org/show_bug.cgi?id=534070
>> ---
>>
>> portage.process.spawn(command, diff --git
>> a/pym/portage/sync/syncbase.py b/pym/portage/sync/syncbase.py index
>> 94d4aab..fcde51f 100644 --- a/pym/portage/sync/syncbase.py
>> +++ b/pym/portage/sync/syncbase.py
>> @@ -12,6 +12,11 @@ import os
>>  import portage
>>  from portage.util import writemsg_level
>>  
>> +_SUBMODULE_PATH_MAP = {
>> +	'glsa': 'metadata/glsa',
>> +	'news': 'metadata/news',
>> +	'profiles': 'profiles',
>> +}
>>  
>>
> 
> It mostly looks good, but...
> 
> I don't like the idea of hard-coding this in the base class.
> 
> I think this should be configurable per-repo or at least per repo type.

The paths should be the same as long as the repo conforms to existing
conventions. If the repo doesn't conform to existing conventions, then
existing tools won't know where to look for the glsa, news, and profiles
directories.

> This patch clears the way for other repo types to have sub-modules, but
> will they be the same mapping for a git tree with those as
> sub-modules?  Maybe I'm just not following how the system works
> properly...

Yes, they should use the same mappings, as explained above.

> Alternatively...if the above is wrong/not feasible.  In the
> sync/__init__.py you have the choices hard-coded there independently of
> these definitions above.  I would prefer that one get it's options from
> the other. Since syncbase.py is not gauranteed to be loaded, I would
> think that the above be declared in the sync module's __init__.py and
> imported or passed in here.  Then the:
> 
> "choices": ("glsa", "news", "profiles"),
> 
> could become:
> 
> "choices": list(_SUBMODULE_PATH_MAP)
> 
> Then there is only one place to edit as options or code change.

That's a good idea. I'll make a patch for that.
-- 
Thanks,
Zac