8:57 a.m.
repo: baserock:baserock/morph
ref: baserock/richarddale/config-messages-verbose-off
SHA1: 11824cadd2a2293ae96219bc1d76c7875506c1e5
Land: master
Add a wrapper function around app.runcmd() to temporarily turn off the
verbose option
Don't write 'git config' messages to the console in verbose mode from
the branch_and_merge_plugin
We discussed this patch earlier on IRC:
Richard Maw: "might I also suggest that instead of wrapping a function
and changing the global settings, add a parameter to runcmd that is
checked before the self.status is printed"
Richard Dale: "i thought about that, but i thought the way i've done it
is less intrusive"
8:57 a.m.
New subject: [PATCH] Add a wrapper function around app.runcmd() to temporarily turn off the verbose option
Don't write 'git config' messages to the console in verbose mode from
the branch_and_merge_plugin
---
morphlib/app.py | 8 ++++++++
morphlib/plugins/branch_and_merge_plugin.py | 8 ++++----
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/morphlib/app.py b/morphlib/app.py
index fcf5411..aa6d520 100644
--- a/morphlib/app.py
+++ b/morphlib/app.py
@@ -432,3 +432,11 @@ class Morph(cliapp.Application):
# run the command line
return cliapp.Application.runcmd(self, argv, *args, **kwargs)
+
+ def runcmd_verbose_off(self, argv, *args, **kwargs):
+ verbose = self.settings['verbose']
+ self.settings['verbose'] = False
+ res = self.runcmd(argv, *args, **kwargs)
+ self.settings['verbose'] = verbose
+ return res
+
diff --git a/morphlib/plugins/branch_and_merge_plugin.py
b/morphlib/plugins/branch_and_merge_plugin.py
index 38e9ccc..71feab3 100644
--- a/morphlib/plugins/branch_and_merge_plugin.py
+++ b/morphlib/plugins/branch_and_merge_plugin.py
@@ -180,18 +180,18 @@ class BranchAndMergePlugin(cliapp.Plugin):
def set_branch_config(self, branch_dir, option, value):
filename = os.path.join(branch_dir, '.morph-system-branch',
'config')
- self.app.runcmd(['git', 'config', '-f', filename, option,
value])
+ self.app.runcmd_verbose_off(['git', 'config', '-f',
filename, option, value])
def get_branch_config(self, branch_dir, option):
filename = os.path.join(branch_dir, '.morph-system-branch',
'config')
- value = self.app.runcmd(['git', 'config', '-f', filename,
option])
+ value = self.app.runcmd_verbose_off(['git', 'config',
'-f', filename, option])
return value.strip()
def set_repo_config(self, repo_dir, option, value):
- self.app.runcmd(['git', 'config', option, value], cwd=repo_dir)
+ self.app.runcmd_verbose_off(['git', 'config', option, value],
cwd=repo_dir)
def get_repo_config(self, repo_dir, option):
- value = self.app.runcmd(['git', 'config', option], cwd=repo_dir)
+ value = self.app.runcmd_verbose_off(['git', 'config', option],
cwd=repo_dir)
return value.strip()
def get_head(self, repo_path):
--
1.7.10.4
10:07 a.m.
New subject: [PATCH] Add a wrapper function around app.runcmd() to temporarily turn off the verbose option
On Tue, Jun 18, 2013 at 01:57:36PM +0100, Richard Dale wrote:
+ def runcmd_verbose_off(self, argv, *args, **kwargs):
+ verbose = self.settings['verbose']
+ self.settings['verbose'] = False
+ res = self.runcmd(argv, *args, **kwargs)
+ self.settings['verbose'] = verbose
+ return res
I don't agree with this way of implementing this: if self.runcmd
fails, the verbose setting will remain turned off. If the caller of
runcmd_verbose_off catches the exception runcmd raises, and then continues
the process instead of terminating, the changed verbosity setting will
be quite confusing to the user.
The right way to implement this is to not have a separate method, and to
add an optional keyword argument to runcmd that can turn off the verbose
output of what is being run.
--
http://www.codethink.co.uk/ http://wiki.baserock.org/ http://www.baserock.com/
9:57 a.m.
On 2013-06-18 13:57, Richard Dale wrote:
repo: baserock:baserock/morph
ref: baserock/richarddale/config-messages-verbose-off
SHA1: 11824cadd2a2293ae96219bc1d76c7875506c1e5
Land: master
Add a wrapper function around app.runcmd() to temporarily turn off
the
verbose option
Don't write 'git config' messages to the console in verbose mode from
the branch_and_merge_plugin
We discussed this patch earlier on IRC:
Richard Maw: "might I also suggest that instead of wrapping a
function
and changing the global settings, add a parameter to runcmd that is
checked before the self.status is printed"
Richard Dale: "i thought about that, but i thought the way i've done
it
is less intrusive"
_______________________________________________
baserock-dev mailing list
baserock-dev(a)baserock.org
http://vlists.pepperfish.net/cgi-bin/mailman/listinfo/baserock-dev-basero...
I'm not sure what you mean by "less intrusive" or if it is actually a
meaningful
goal here.
If we modify morphlib.app.runcmd to optionally disable verbose, then in
future we
can have other options simultaneously (e.g. a disable log option that
we might
want active at the same time as disabled verbose). Having separate
functions would
make this cumbersome.
10:41 a.m.
On 06/18/2013 02:57 PM, Jonathan Maw wrote:
On 2013-06-18 13:57, Richard Dale wrote:
> repo: baserock:baserock/morph
> ref: baserock/richarddale/config-messages-verbose-off
> SHA1: 11824cadd2a2293ae96219bc1d76c7875506c1e5
> Land: master
>
> Add a wrapper function around app.runcmd() to temporarily turn off the
> verbose option
>
> Don't write 'git config' messages to the console in verbose mode from
> the branch_and_merge_plugin
>
> We discussed this patch earlier on IRC:
>
> Richard Maw: "might I also suggest that instead of wrapping a function
> and changing the global settings, add a parameter to runcmd that is
> checked before the self.status is printed"
>
> Richard Dale: "i thought about that, but i thought the way i've done it
> is less intrusive"
>
>
> _______________________________________________
> baserock-dev mailing list
> baserock-dev(a)baserock.org
>
> http://vlists.pepperfish.net/cgi-bin/mailman/listinfo/baserock-dev-basero...
>
I'm not sure what you mean by "less intrusive" or if it is actually a
meaningful
goal here.
If we modify morphlib.app.runcmd to optionally disable verbose, then in
future we
can have other options simultaneously (e.g. a disable log option that we
might
want active at the same time as disabled verbose). Having separate
functions would
make this cumbersome.
I was thinking of this line in the status() function when deciding
whether or not to output a message to the console. It will now need
changing from:
ok = verbose or error or (not quiet and not chatty)
to:
ok = (verbose and not disable_verbose) or error or (not quiet and not
chatty)
I already found it a bit hard to read. Maybe a wrapper function isn't
very easy to read either and also doesn't scale as you point out.
I think Lars point about my wrapper function throwing an exception, and
leaving morph stuck with verbose mode switched off is very valid too.
I'll get rid of a wrapper function and add a 'disable_verbose' flag to
the args passed to runcmd().
10:47 a.m.
On 2013-06-18 15:41, Richard Dale wrote:
[snip]
> I was thinking of this line in the status() function when deciding
> whether or not to output a message to the console. It will now need
> changing from:
> ok = verbose or error or (not quiet and not chatty)
> to:
> ok = (verbose and not disable_verbose) or error or (not
quiet and not
> chatty)
> I already found it a bit hard to read. Maybe a wrapper
function isn't
> very easy to read either and also doesn't scale as you point out.
> I think Lars point about my wrapper function throwing an
exception,
> and leaving morph stuck with verbose mode switched off is very valid
> too.
> I'll get rid of a wrapper function and add a
'disable_verbose' flag
> to the args passed to runcmd().
I don't think that is what he meant. I think Richard Maw
referred to changing 'verbose' before calling self.status()
in runcmd, instead of modifying self.status() itself. I may
be wrong, of course.
10:58 a.m.
On Tue, Jun 18, 2013 at 03:47:10PM +0100, Jonathan Maw wrote:
I don't think that is what he meant. I think Richard Maw
referred to changing 'verbose' before calling self.status()
in runcmd, instead of modifying self.status() itself. I may
be wrong, of course.
I meant that the new flag to runcmd should be checked before calling
self.status(), since IIRC that's where the messages are coming from.
11:46 a.m.
On 06/18/2013 03:47 PM, Jonathan Maw wrote:
On 2013-06-18 15:41, Richard Dale wrote:
>
[snip]
> I was thinking of this line in the status() function when deciding
> whether or not to output a message to the console. It will now need
> changing from:
>
> ok = verbose or error or (not quiet and not chatty)
>
> to:
>
> ok = (verbose and not disable_verbose) or error or (not quiet and not
> chatty)
>
> I already found it a bit hard to read. Maybe a wrapper function isn't
> very easy to read either and also doesn't scale as you point out.
>
> I think Lars point about my wrapper function throwing an exception,
> and leaving morph stuck with verbose mode switched off is very valid
> too.
>
> I'll get rid of a wrapper function and add a 'disable_verbose' flag
> to the args passed to runcmd().
>
I don't think that is what he meant. I think Richard Maw
referred to changing 'verbose' before calling self.status()
in runcmd, instead of modifying self.status() itself. I may
be wrong, of course.
I see. So self.status() should use a verbose flag passed to it rather
than looking in app settings. I don't know if status() is called
elsewhere directly and not via runcmd().
-- Richard
11:54 a.m.
On 2013-06-18 16:46, Richard Dale wrote:
On 06/18/2013 03:47 PM, Jonathan Maw wrote:
> On 2013-06-18 15:41, Richard Dale wrote:
[snip]
>
> I don't think that is what he meant. I think Richard Maw
> referred to changing 'verbose' before calling self.status()
> in runcmd, instead of modifying self.status() itself. I may
> be wrong, of course.
I see. So self.status() should use a verbose flag passed to it rather
than looking in app settings. I don't know if status() is called
elsewhere directly and not via runcmd().
-- Richard
I'm not sure where you're getting that from. You might change status()
to take an optional arg, but you could just as easily temporarily
change
settings.verbose while calling status(), much like you did for calling
runcmd(), which I believe is what Richard Maw originally suggested.
noon
On Tue, Jun 18, 2013 at 04:54:48PM +0100, Jonathan Maw wrote:
I'm not sure where you're getting that from. You might change
status()
to take an optional arg, but you could just as easily temporarily
change
settings.verbose while calling status(), much like you did for calling
runcmd(), which I believe is what Richard Maw originally suggested.
I'm not understanding your interpretation of my meaning here.
I suggested that instead of changing any settings, you add a flag
or parameter to runcmd to suppress output. This flag would be tested
before running self.status, so rather than changing self.status to not
print anything, it doesn't get called at all.
12:07 p.m.
On 2013-06-18 17:00, Richard Maw wrote:
On Tue, Jun 18, 2013 at 04:54:48PM +0100, Jonathan Maw wrote:
> I'm not sure where you're getting that from. You might change
> status()
> to take an optional arg, but you could just as easily temporarily
> change
> settings.verbose while calling status(), much like you did for
> calling
> runcmd(), which I believe is what Richard Maw originally suggested.
I'm not understanding your interpretation of my meaning here.
I suggested that instead of changing any settings, you add a flag
or parameter to runcmd to suppress output. This flag would be tested
before running self.status, so rather than changing self.status to
not
print anything, it doesn't get called at all.
My one concern with that is that we lose logging output as well, if we
choose not to call self.status. I have no idea whether we should intend
to do that.
12:07 p.m.
On Tue, Jun 18, 2013 at 05:00:03PM +0100, Richard Maw wrote:
I suggested that instead of changing any settings, you add a flag
or parameter to runcmd to suppress output. This flag would be tested
before running self.status, so rather than changing self.status to not
print anything, it doesn't get called at all.
This is the right way to do it. Something like the below, which I have not
tested, and I hope Richard Dale will test it and fix it so it works, and
changing the call sites to disable the command printing when we don't want
them.
diff --git a/morphlib/app.py b/morphlib/app.py
index fcf5411..d72fa3c 100644
--- a/morphlib/app.py
+++ b/morphlib/app.py
@@ -412,6 +412,12 @@ class Morph(cliapp.Application):
if 'env' not in kwargs:
kwargs['env'] = dict(os.environ)
+ if 'print_command' in kwargs:
+ print_command = kwargs['print_command']
+ del kwargs['print_command']
+ else:
+ print_command = True
+
# convert the command line arguments into a string
commands = [argv] + list(args)
for command in commands:
@@ -421,9 +427,10 @@ class Morph(cliapp.Application):
commands = [' '.join(command) for command in commands]
# print the command line
- self.status(msg='# %(cmdline)s',
- cmdline=' | '.join(commands),
- chatty=True)
+ if print_command:
+ self.status(msg='# %(cmdline)s',
+ cmdline=' | '.join(commands),
+ chatty=True)
# Log the environment.
prev = getattr(self, 'prev_env', {})
--
http://www.codethink.co.uk/ http://wiki.baserock.org/ http://www.baserock.com/
4:47 a.m.
repo: baserock:baserock/morph
ref: baserock/richarddale/config-messages-verbose-off-2
SHA1: 6042be1f37c4d6e94ad292ad25e748aa73d68d93
Land: master
Inhibit writing git config messages as per Lars' suggestion
4:47 a.m.
New subject: [PATCH 1/2] Don't show git config messages for 'morph branch' in verbose mode
---
morphlib/app.py | 13 ++++++++++---
morphlib/git.py | 2 +-
morphlib/plugins/branch_and_merge_plugin.py | 8 ++++----
3 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/morphlib/app.py b/morphlib/app.py
index f82eae3..b8f1eb1 100644
--- a/morphlib/app.py
+++ b/morphlib/app.py
@@ -410,6 +410,12 @@ class Morph(cliapp.Application):
if 'env' not in kwargs:
kwargs['env'] = dict(os.environ)
+ if 'print_command' in kwargs:
+ print_command = kwargs['print_command']
+ del kwargs['print_command']
+ else:
+ print_command = True
+
# convert the command line arguments into a string
commands = [argv] + list(args)
for command in commands:
@@ -419,9 +425,10 @@ class Morph(cliapp.Application):
commands = [' '.join(command) for command in commands]
# print the command line
- self.status(msg='# %(cmdline)s',
- cmdline=' | '.join(commands),
- chatty=True)
+ if print_command:
+ self.status(msg='# %(cmdline)s',
+ cmdline=' | '.join(commands),
+ chatty=True)
# Log the environment.
prev = getattr(self, 'prev_env', {})
diff --git a/morphlib/git.py b/morphlib/git.py
index 9cdd0a8..3b9c424 100644
--- a/morphlib/git.py
+++ b/morphlib/git.py
@@ -214,7 +214,7 @@ def check_config_set(runcmd, keys, cwd='.'):
found = {}
for key in keys:
try:
- value = runcmd(['git', 'config', key], cwd=cwd).strip()
+ value = runcmd(['git', 'config', key], cwd=cwd,
print_command=False).strip()
found[key] = value
except cliapp.AppException:
missing.append(key)
diff --git a/morphlib/plugins/branch_and_merge_plugin.py
b/morphlib/plugins/branch_and_merge_plugin.py
index 38e9ccc..bad2e9d 100644
--- a/morphlib/plugins/branch_and_merge_plugin.py
+++ b/morphlib/plugins/branch_and_merge_plugin.py
@@ -180,18 +180,18 @@ class BranchAndMergePlugin(cliapp.Plugin):
def set_branch_config(self, branch_dir, option, value):
filename = os.path.join(branch_dir, '.morph-system-branch',
'config')
- self.app.runcmd(['git', 'config', '-f', filename, option,
value])
+ self.app.runcmd(['git', 'config', '-f', filename, option,
value], print_command=False)
def get_branch_config(self, branch_dir, option):
filename = os.path.join(branch_dir, '.morph-system-branch',
'config')
- value = self.app.runcmd(['git', 'config', '-f', filename,
option])
+ value = self.app.runcmd(['git', 'config', '-f', filename,
option], print_command=False)
return value.strip()
def set_repo_config(self, repo_dir, option, value):
- self.app.runcmd(['git', 'config', option, value], cwd=repo_dir)
+ self.app.runcmd(['git', 'config', option, value], cwd=repo_dir,
print_command=False)
def get_repo_config(self, repo_dir, option):
- value = self.app.runcmd(['git', 'config', option], cwd=repo_dir)
+ value = self.app.runcmd(['git', 'config', option], cwd=repo_dir,
print_command=False)
return value.strip()
def get_head(self, repo_path):
--
1.7.10.4
4:47 a.m.
New subject: [PATCH 2/2] Fix lines overflowing the 80 character limit
---
morphlib/git.py | 3 ++-
morphlib/plugins/branch_and_merge_plugin.py | 12 ++++++++----
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/morphlib/git.py b/morphlib/git.py
index 3b9c424..88c6dae 100644
--- a/morphlib/git.py
+++ b/morphlib/git.py
@@ -214,7 +214,8 @@ def check_config_set(runcmd, keys, cwd='.'):
found = {}
for key in keys:
try:
- value = runcmd(['git', 'config', key], cwd=cwd,
print_command=False).strip()
+ value = runcmd(['git', 'config', key], cwd=cwd,
+ print_command=False).strip()
found[key] = value
except cliapp.AppException:
missing.append(key)
diff --git a/morphlib/plugins/branch_and_merge_plugin.py
b/morphlib/plugins/branch_and_merge_plugin.py
index bad2e9d..f7109bc 100644
--- a/morphlib/plugins/branch_and_merge_plugin.py
+++ b/morphlib/plugins/branch_and_merge_plugin.py
@@ -180,18 +180,22 @@ class BranchAndMergePlugin(cliapp.Plugin):
def set_branch_config(self, branch_dir, option, value):
filename = os.path.join(branch_dir, '.morph-system-branch',
'config')
- self.app.runcmd(['git', 'config', '-f', filename, option,
value], print_command=False)
+ self.app.runcmd(['git', 'config', '-f', filename, option,
value],
+ print_command=False)
def get_branch_config(self, branch_dir, option):
filename = os.path.join(branch_dir, '.morph-system-branch',
'config')
- value = self.app.runcmd(['git', 'config', '-f', filename,
option], print_command=False)
+ value = self.app.runcmd(['git', 'config', '-f', filename,
option],
+ print_command=False)
return value.strip()
def set_repo_config(self, repo_dir, option, value):
- self.app.runcmd(['git', 'config', option, value], cwd=repo_dir,
print_command=False)
+ self.app.runcmd(['git', 'config', option, value], cwd=repo_dir,
+ print_command=False)
def get_repo_config(self, repo_dir, option):
- value = self.app.runcmd(['git', 'config', option], cwd=repo_dir,
print_command=False)
+ value = self.app.runcmd(['git', 'config', option], cwd=repo_dir,
+ print_command=False)
return value.strip()
def get_head(self, repo_path):
--
1.7.10.4
6:26 a.m.
On Tue, Jun 25, 2013 at 09:47:07AM +0100, Richard Dale wrote:
repo: baserock:baserock/morph
ref: baserock/richarddale/config-messages-verbose-off-2
SHA1: 6042be1f37c4d6e94ad292ad25e748aa73d68d93
Land: master
Merged, thanks.
--
http://www.codethink.co.uk/ http://wiki.baserock.org/ http://www.baserock.com/
3629
days inactive
3636
days old
15 comments
4 participants
participants (4)
-
Jonathan Maw -
Lars Wirzenius -
Richard Dale -
Richard Maw