[pacman-dev] [PATCH] Clarify remove error message.

Chantry Xavier

2 Feb 2008 2 Feb '08

10:51 p.m.

Signed-off-by: Chantry Xavier --- 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

Show replies by date

Dan McGee

2 Feb 2 Feb

11:01 p.m.

On Feb 2, 2008 4:51 PM, Chantry Xavier wrote:

...

Signed-off-by: Chantry Xavier --- 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

Xavier

3 Feb 3 Feb

12:12 a.m.

Dan McGee wrote:

...

On Feb 2, 2008 4:51 PM, Chantry Xavier wrote:

...
Signed-off-by: Chantry Xavier --- 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.

Chantry Xavier

12:18 a.m.

New subject: [pacman-dev] [PATCH] Clarify the "failed to add target" errors.

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

Nagy Gabor

4 Feb 4 Feb

4 p.m.

New subject: [pacman-dev] [PATCH] Clarify the "failed to add target" errors.

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/

Xavier

6:03 p.m.

New subject: [pacman-dev] [PATCH] Clarify the "failed to add target" errors.

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 ?

Nagy Gabor

5 Feb 5 Feb

1:21 p.m.

New subject: [pacman-dev] [PATCH] Clarify the "failed to add target" errors.

...

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/

Xavier

1:41 p.m.

New subject: [pacman-dev] [PATCH] Clarify the "failed to add target" errors.

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.

5933

Age (days ago)

5936

Last active (days ago)

List overview

Download

7 comments

4 participants

participants (4)

Chantry Xavier
Dan McGee
Nagy Gabor
Xavier