Re: [gentoo-catalyst] [PATCH 2/4] Remove self.mounts and self.mountmap's use of paths for keys and paths.

[Matt Turner <mattst88@gentoo.org>]

On Sat, Dec 14, 2013 at 10:05 AM, W. Trevor King <wking@tremily.us> wrote:
>> On Sat, Dec 14, 2013 at 03:15:42AM -0800, Brian Dolbec wrote:
> And here's the request-pull:
>
>   $ git request-pull origin/master wking wking/dolsen-rewrite-part-1
>   The following changes since commit 1c86c64113491885b159529dacb452ce6a3e5f4b:
>
>     catalyst 2.0.15 (2013-11-13 13:59:25 -0800)
>
>   are available in the git repository at:
>
>     git://tremily.us/catalyst.git dolsen-rewrite-part-1
>
>   for you to fetch changes up to 907c35fb1e1cae75cc78d45b054c3c43ac9deb48:
>
>     cleanup long lines, improve usage() output formatting slightly (2013-12-14 09:48:22 -0800)
>
>   ----------------------------------------------------------------
>   Brian Dolbec (20):
>         modules/tinderbox_target.py: Use 'portdir' instead of hard-coding '/usr/portage'
>         modules/generic_stage_target.py: Use 'portdir' instead of hard-coding '/usr/portage'
>         modules/generic_stage_target.py: Use 'portdir' instead of hard-coding '/usr/portage'
>         modules/generic_stage_target.py: Use 'distdir' instead of hard-coding '${PORTAGE}/distfiles'
>         modules/generic_stage_target.py: Use a 'local_overlay' setting instead of hard-coding '/usr/local/portage'
>         catalyst: Split confdefaults into line-per-entry
>         catalyst: Add 'repo_name' default
>         catalyst: Add 'snapshot_name' default
>         catalyst: Add 'packagedir' default instead of hard-coding '/usr/portage/packages'
>         catalyst: Add 'port_tmpdir' default instead of hard-coding '/var/tmp/portage'
>         modules/generic_stage_target.py: Don't use paths as mountmap keys
>         modules/generic_stage_target.py: Use 'proc' instead of '/proc' as the mountmap key
>         modules/generic_stage_target.py: Use 'dev' instead of '/dev' as the mountmap key
>         modules/generic_stage_target.py: Use 'distdir' instead of '/usr/portage/distfiles' as the mountmap key
>         modules/generic_stage_target.py: Use 'port_tmpdir' instead of '/var/tmp/portage' as the mountmap key
>         modules/generic_stage_target.py: Use 'devpts' instead of '/dev/pts' as the mountmap key
>         modules/generic_stage_target.py: Use 'packagedir' instead of '/usr/portage/packages' as the mountmap key
>         modules/generic_stage_target.py: Use 'kerncache' instead of '/tmp/kerncache' as the mountmap key
>         modules/generic_stage_target.py: Use 'ccache' instead of '/var/tmp/ccache' as the mountmap key
>         cleanup long lines, improve usage() output formatting slightly
>
>    catalyst                        | 101 ++++++++++++++++++++++++----------------
>    modules/generic_stage_target.py |  92 ++++++++++++++++++------------------
>    modules/snapshot_target.py      |  14 ++++--
>    modules/tinderbox_target.py     |   4 +-
>    4 files changed, 119 insertions(+), 92 deletions(-)
>
> I can submit them to the list via `git send-email` if you'd like, just
> let me know.
>
> Cheers,
> Trevor

Awesome, thanks for doing this. This is exactly how this patch series
should look and was super easy to review. I feel they're basically
ready to go, but I want everyone to be in the habit of sending patches
to the list, so please do.

Preemptive review: I saw just a few minor things, almost entirely
dealing with lack of whitespace around operators. git log -p --reverse
origin/master..wking/dolsen-rewrite-part-1 and then search for ^\+.*[^
]\+[^ ] (less regex) finds a couple.

I'd opt to just put parenthesis around prints where we're going to be
line wrapping its argument. It accomplishes the same thing as the
trailing slash, but moves us slightly in the direction of python3. So,
fix up lines you think it possible that match ^\+.*\\$

The other thing: I think we should drop most of the last patch for
now. Wrapping long lines before we switch from 8-space tabs to PEP8
4-space style seems premature. The other half of that commit that
replaces a series of prints with a single one looks good though.

You may want to put your Signed-off-by on the patches to note that you
refactored them heavily.

Thanks again,
Matt