[pacman-dev] [PATCH] Clarify remove error message.
Signed-off-by: Chantry Xavier <shiningxc@gmail.com> --- src/pacman/remove.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/src/pacman/remove.c b/src/pacman/remove.c index 56837fa..ee4e84a 100644 --- a/src/pacman/remove.c +++ b/src/pacman/remove.c @@ -108,8 +108,7 @@ int pacman_remove(alpm_list_t *targets) char *targ = alpm_list_getdata(i); if(alpm_trans_addtarget(targ) == -1) { printf("failed.\n"); - fprintf(stderr, _("error: failed to add target '%s' (%s)\n"), targ, - alpm_strerrorlast()); + fprintf(stderr, _("error: could not find '%s' to remove\n"), targ); remove_cleanup(); FREELIST(finaltargs); return(1); -- 1.5.4.rc4
On Feb 2, 2008 4:51 PM, Chantry Xavier <shiningxc@gmail.com> wrote:
Signed-off-by: Chantry Xavier <shiningxc@gmail.com> --- src/pacman/remove.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/src/pacman/remove.c b/src/pacman/remove.c index 56837fa..ee4e84a 100644 --- a/src/pacman/remove.c +++ b/src/pacman/remove.c @@ -108,8 +108,7 @@ int pacman_remove(alpm_list_t *targets) char *targ = alpm_list_getdata(i); if(alpm_trans_addtarget(targ) == -1) { printf("failed.\n"); - fprintf(stderr, _("error: failed to add target '%s' (%s)\n"), targ, - alpm_strerrorlast()); + fprintf(stderr, _("error: could not find '%s' to remove\n"), targ); remove_cleanup(); FREELIST(finaltargs); return(1);
I've actually been putting this one off for a reason- currently this error message, although not apparant to the normal user, appears all over when we can't add a target (package) to a package list (in this case a removal list). I'm hesitant to change the message in this one place because of the use of the word 'add'. Can we think of something that can be used everywhere (sync, add, remove)? "error: failed to find target '%s'" perhaps? -Dan
Dan McGee wrote:
On Feb 2, 2008 4:51 PM, Chantry Xavier<shiningxc@gmail.com> wrote:
Signed-off-by: Chantry Xavier<shiningxc@gmail.com> --- src/pacman/remove.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/src/pacman/remove.c b/src/pacman/remove.c index 56837fa..ee4e84a 100644 --- a/src/pacman/remove.c +++ b/src/pacman/remove.c @@ -108,8 +108,7 @@ int pacman_remove(alpm_list_t *targets) char *targ = alpm_list_getdata(i); if(alpm_trans_addtarget(targ) == -1) { printf("failed.\n"); - fprintf(stderr, _("error: failed to add target '%s' (%s)\n"), targ, - alpm_strerrorlast()); + fprintf(stderr, _("error: could not find '%s' to remove\n"), targ); remove_cleanup(); FREELIST(finaltargs); return(1);
I've actually been putting this one off for a reason- currently this error message, although not apparant to the normal user, appears all over when we can't add a target (package) to a package list (in this case a removal list). I'm hesitant to change the message in this one place because of the use of the word 'add'. Can we think of something that can be used everywhere (sync, add, remove)?
"error: failed to find target '%s'" perhaps?
I just noticed -S was different than -R and -U. It simply uses strerrorlast, which might be better. current behavior: bash-3.2$ sudo LANG=C pacman -S foo error: 'foo': not found in sync db bash-3.2$ sudo LANG=C pacman -U foo error: failed to add target 'foo' (cannot open package file)loading package data... bash-3.2$ bash-3.2$ sudo LANG=C pacman -R foo loading package data... failed. error: failed to add target 'foo' (could not find or read package) new behavior: bash-3.2$ sudo LANG=C src/pacman/pacman -S foo error: 'foo': not found in sync db bash-3.2$ sudo LANG=C src/pacman/pacman -U foo loading package data... failed. error: 'foo': cannot open package file bash-3.2$ sudo LANG=C src/pacman/pacman -R foo loading package data... failed. error: 'foo': could not find or read package A patch for this will follow.
Make the error message printed when addtarget fails consistent between add.c, remove.c and sync.c. The main problem was that the "failed to add target" in case of a removal operation could sound confusing. There was also a little output problem with -U ("failed" was missing). Signed-off-by: Chantry Xavier <shiningxc@gmail.com> --- src/pacman/add.c | 5 +++-- src/pacman/remove.c | 4 ++-- src/pacman/sync.c | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/pacman/add.c b/src/pacman/add.c index 975d26b..debe5c4 100644 --- a/src/pacman/add.c +++ b/src/pacman/add.c @@ -115,8 +115,9 @@ int pacman_add(alpm_list_t *targets) for(i = targets; i; i = alpm_list_next(i)) { char *targ = alpm_list_getdata(i); if(alpm_trans_addtarget(targ) == -1) { - fprintf(stderr, _("error: failed to add target '%s' (%s)"), targ, - alpm_strerrorlast()); + printf("failed.\n"); + fprintf(stderr, _("error: '%s': %s\n"), + targ, alpm_strerrorlast()); add_cleanup(); return(1); } diff --git a/src/pacman/remove.c b/src/pacman/remove.c index 56837fa..0722057 100644 --- a/src/pacman/remove.c +++ b/src/pacman/remove.c @@ -108,8 +108,8 @@ int pacman_remove(alpm_list_t *targets) char *targ = alpm_list_getdata(i); if(alpm_trans_addtarget(targ) == -1) { printf("failed.\n"); - fprintf(stderr, _("error: failed to add target '%s' (%s)\n"), targ, - alpm_strerrorlast()); + fprintf(stderr, _("error: '%s': %s\n"), + targ, alpm_strerrorlast()); remove_cleanup(); FREELIST(finaltargs); return(1); diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 582192a..27218d6 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -568,7 +568,7 @@ static int sync_trans(alpm_list_t *targets, int sync_only) } if(pm_errno != PM_ERR_PKG_NOT_FOUND) { fprintf(stderr, _("error: '%s': %s\n"), - (char *)i->data, alpm_strerrorlast()); + targ, alpm_strerrorlast()); retval = 1; goto cleanup; } -- 1.5.4
Hi! Well, this error message problem is not critical at all, but I don't like your preferred fix, because: - all "transaction steps" (init, release, addtarget, prepare, commit) use the same format now: "error: failed to foo transaction (ERRORLAST)\n" and the patch would break this simmetry. - I'm not sure that alpm_strerrorlast() is always informative and set properly - we can get the same ERRORLAST so foo can be helpful in debugging in these cases - Personally I liked this front-end + back-end error message "concatenation" So I prefer rephase this "failed to add target" message, Dan's suggestion seems OK to me: http://www.archlinux.org/pipermail/pacman-dev/2008-February/011045.html Bye ---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/
Nagy Gabor wrote:
Hi!
Well, this error message problem is not critical at all, but I don't like your preferred fix, because: - all "transaction steps" (init, release, addtarget, prepare, commit) use the same format now: "error: failed to foo transaction (ERRORLAST)\n" and the patch would break this simmetry.
I simply kept the sync.c message, and used the same for remove.c and add.c. So on this side, my patch makes it more consistent. Maybe sync.c wasn't consistent in the first place, but well :P
- I'm not sure that alpm_strerrorlast() is always informative and set properly
Then make it more informative :)
- we can get the same ERRORLAST so foo can be helpful in debugging in these cases - Personally I liked this front-end + back-end error message "concatenation"
So I prefer rephase this "failed to add target" message, Dan's suggestion seems OK to me: http://www.archlinux.org/pipermail/pacman-dev/2008-February/011045.html
So should I change in my patch from : fprintf(stderr, _("error: '%s': %s\n"), targ, alpm_strerrorlast()); to fprintf(stderr, _("error: failed to find target '%s' (%s)\n"), targ, alpm_strerrorlast()); for sync.c , remove.c and add.c ?
So should I change in my patch from : fprintf(stderr, _("error: '%s': %s\n"), targ, alpm_strerrorlast()); to fprintf(stderr, _("error: failed to find target '%s' (%s)\n"), targ, alpm_strerrorlast());
for sync.c , remove.c and add.c ?
my vote: yes [Note: you should patch line 632 in sync.c; and may rephrase the other error messages around this step1 part.] Bye ---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/
Nagy Gabor wrote:
So should I change in my patch from : fprintf(stderr, _("error: '%s': %s\n"), targ, alpm_strerrorlast()); to fprintf(stderr, _("error: failed to find target '%s' (%s)\n"), targ, alpm_strerrorlast());
for sync.c , remove.c and add.c ?
my vote: yes [Note: you should patch line 632 in sync.c; and may rephrase the other error messages around this step1 part.]
Actually, that message would not be acceptable in sync.c, line 570. 569 if(pm_errno != PM_ERR_PKG_NOT_FOUND) { 570 fprintf(stderr, _("error: failed to find target '%s'\n"), 571 (char *)i->data, alpm_strerrorlast()); This error is not displayed when the package is not found, but when something else happens. So I am not going to change my previous patch. I will let Dan choose whether he accepts it or not (he already pulled it in his maint branch : http://code.toofishes.net/gitweb.cgi?p=pacman.git;a=shortlog;h=maint). In any cases (whether my patch is applied or not), you are free to propose another patch, since you are apparently not satisfied.
participants (4)
-
Chantry Xavier
-
Dan McGee
-
Nagy Gabor
-
Xavier