[systemd-devel] [PATCH 3/4] systemctl: edit: further reword error messages

[Zbigniew Jędrzejewski-Szmek]

On Wed, Feb 04, 2015 at 03:35:47AM +0300, Ivan Shapovalov wrote:
> On 2015-01-05 at 17:08 +0100, Zbigniew Jędrzejewski-Szmek wrote:
> > On Fri, Dec 19, 2014 at 05:08:09PM +0300, Ivan Shapovalov wrote:
> > > ---
> > >  src/systemctl/systemctl.c | 22 +++++++++++-----------
> > >  1 file changed, 11 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
> > > index 658793e..20c367c 100644
> > > --- a/src/systemctl/systemctl.c
> > > +++ b/src/systemctl/systemctl.c
> > > @@ -4776,7 +4776,7 @@ static int init_home_and_lookup_paths(char **user_home, char **user_runtime, Loo
> > >  
> > >          r = lookup_paths_init_from_scope(lp, arg_scope, arg_root);
> > >          if (r < 0)
> > > -                return log_error_errno(r, "Failed to lookup unit lookup paths: %m");
> > > +                return log_error_errno(r, "Failed to get unit lookup paths: %m");
> > Maybe "query"? "get" is ugly.
> 
> OK.
> 
> > 
> > >          return 0;
> > >  }
> > > @@ -5900,11 +5900,11 @@ static int create_edit_temp_file(const char *new_path, const char *original_path
> > >  
> > >          r = tempfn_random(new_path, &t);
> > >          if (r < 0)
> > > -                return log_error_errno(r, "Failed to determine temporary filename for %s: %m", new_path);
> > > +                return log_error_errno(r, "Failed to determine temporary filename for \"%s\": %m", new_path);
> > >  
> > >          r = mkdir_parents(new_path, 0755);
> > >          if (r < 0) {
> > > -                log_error_errno(r, "Failed to create directories for %s: %m", new_path);
> > > +                log_error_errno(r, "Failed to create directories for \"%s\": %m", new_path);
> > >                  free(t);
> > >                  return r;
> > >          }
> > > @@ -5913,12 +5913,12 @@ static int create_edit_temp_file(const char *new_path, const char *original_path
> > >          if (r == -ENOENT) {
> > >                  r = touch(t);
> > >                  if (r < 0) {
> > > -                        log_error_errno(r, "Failed to create temporary file %s: %m", t);
> > > +                        log_error_errno(r, "Failed to create temporary file \"%s\": %m", t);
> > >                          free(t);
> > >                          return r;
> > >                  }
> > >          } else if (r < 0) {
> > > -                log_error_errno(r, "Failed to copy %s to %s: %m", original_path, t);
> > > +                log_error_errno(r, "Failed to copy \"%s\" to \"%s\": %m", original_path, t);
> > >                  free(t);
> > >                  return r;
> > >          }
> > > @@ -6026,7 +6026,7 @@ static int unit_file_create_copy(const char *unit_name,
> > >          if (!path_equal(fragment_path, tmp_new_path) && access(tmp_new_path, F_OK) == 0) {
> > >                  char response;
> > >  
> > > -                r = ask_char(&response, "yn", "%s already exists, are you sure to overwrite it with %s? [(y)es, (n)o] ", tmp_new_path, fragment_path);
> > > +                r = ask_char(&response, "yn", "\"%s\" already exists, are you sure to overwrite it with \"%s\"? [(y)es, (n)o] ", tmp_new_path, fragment_path);
> > 
> > This is not gramatically correct. I think
> > "\"%s\" already exists. Overwrite with \"%s\"?..." would be better.
> 
> OK.
> 
> > 
> > 
> > >                  if (r < 0) {
> > >                          free(tmp_new_path);
> > >                          return r;
> > > @@ -6040,7 +6040,7 @@ static int unit_file_create_copy(const char *unit_name,
> > >  
> > >          r = create_edit_temp_file(tmp_new_path, fragment_path, &tmp_tmp_path);
> > >          if (r < 0) {
> > > -                log_error_errno(r, "Failed to create temporary file for %s: %m", tmp_new_path);
> > > +                log_error_errno(r, "Failed to create temporary file for \"%s\": %m", tmp_new_path);
> > >                  free(tmp_new_path);
> > >                  return r;
> > >          }
> > > @@ -6176,12 +6176,12 @@ static int edit(sd_bus *bus, char **args) {
> > >          assert(args);
> > >  
> > >          if (!on_tty()) {
> > > -                log_error("Cannot edit units if we are not on a tty");
> > > +                log_error("Not on a tty, refusing.");
> > Why? Replacing a nice specific error message with a generic one seems to
> > be a step backwards.
> 
> I've tried to follow the existing pattern of most error messages in
> systemd: "Condition, action + -ing."
> 
> I'll reword again to make messages more specific... unless you tell me
> to stop bikeshedding and leave it as is ;)
Yeah, I think that that those messages are OK.

> > > -                        log_warning("Edition of %s canceled: temporary file empty", *original);
> > > +                        log_warning("Temporary file empty, not saving.");
> > And here.
> 
> Here? They are equally specific; "editing cancelled" == "file not
> saved".
The original one gives a hint of what the overall effect for the action is.
With you replacemnt, things are less explicit.

systemctl edit is something that may be used by relatively newbie
administrators, so some hand-holding is more useful than in other parts
of systemd.

Zbyszek