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

[Ivan Shapovalov]

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 ;)

> 
> >                  return -EINVAL;
> >          }
> >  
> >          if (arg_transport != BUS_TRANSPORT_LOCAL) {
> > -                log_error("Cannot remotely edit units");
> > +                log_error("Remote operation requested, refusing.");
> And the same here.
> 
> >                  return -EINVAL;
> >          }
> >  
> > @@ -6205,12 +6205,12 @@ static int edit(sd_bus *bus, char **args) {
> >                   * It's useful if the user wants to cancel its modification
> >                   */
> >                  if (null_or_empty_path(*tmp)) {
> > -                        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".

-- 
Ivan Shapovalov / intelfx /

> 
> >                          continue;
> >                  }
> >                  r = rename(*tmp, *original);
> >                  if (r < 0) {
> > -                        r = log_error_errno(errno, "Failed to rename %s to %s: %m", *tmp, *original);
> > +                        r = log_error_errno(errno, "Failed to rename \"%s\" to \"%s\": %m", *tmp, *original);
> >                          goto end;
> >                  }
> >          }
> 
> Zbyszek

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 213 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20150204/79fc5038/attachment-0001.sig>