On Sat, May 10, 2008 at 10:30 PM, Allan McRae <mcrae_allan@hotmail.com> wrote:
Tested using many easily generated error conditions. Also added "malloc failure" (conf.c) and "segmentation fault" (pacman.c) error messages for translation
Signed-off-by: Allan McRae <mcrae_allan@hotmail.com> This is great, thanks. I added a few comments inline and below, but I've pulled this patch into my working branch and made the fixes myself.
--- src/pacman/conf.c | 2 +- src/pacman/package.c | 7 +++---- src/pacman/pacman.c | 6 +++--- src/pacman/query.c | 15 ++++++++------- src/pacman/remove.c | 6 +++--- src/pacman/sync.c | 44 ++++++++++++++++++++++++++------------------ src/pacman/upgrade.c | 7 ++++--- src/pacman/util.c | 4 ++-- 8 files changed, 50 insertions(+), 41 deletions(-)
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index bf3a462..bf3909a 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -33,7 +33,7 @@ config_t *config_new(void) { config_t *newconfig = calloc(1, sizeof(config_t)); if(!newconfig) { - fprintf(stderr, "malloc failure: could not allocate %zd bytes\n", + pm_fprintf(stderr, PM_LOG_ERROR, _("malloc failure: could not allocate %zd bytes\n"), sizeof(config_t)); return(NULL); } conf.c spat a warning about not knowing the definition of pm_fprintf, which is because util.h hadn't been included. It probably correctly compiled for you because you aren't using the --enable-debug option, so a warning would have been spit here but it wouldn't have stopped your compile. Just something to keep in mind- when you are doing development, you probably want to keep --enable-debug turned on.
diff --git a/src/pacman/package.c b/src/pacman/package.c index 76e8e4f..7f587e0 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -195,8 +195,8 @@ void dump_pkg_backups(pmpkg_t *pkg) char *md5sum = alpm_get_md5sum(path);
if(md5sum == NULL) { - fprintf(stderr, _("error: could not calculate checksums for %s\n"), - path); + pm_fprintf(stderr, PM_LOG_ERROR, + _("could not calculate checksums for %s\n"), path); free(str); continue; } @@ -245,8 +245,7 @@ void dump_pkg_changelog(pmpkg_t *pkg) void *fp = NULL;
if((fp = alpm_pkg_changelog_open(pkg)) == NULL) { - /* TODO after string freeze use pm_fprintf */ - fprintf(stderr, _("error: no changelog available for '%s'.\n"), + pm_fprintf(stderr, PM_LOG_ERROR, _("no changelog available for '%s'.\n"), alpm_pkg_get_name(pkg)); return; } else { diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 9468d51..53a96cf 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -213,9 +213,9 @@ static RETSIGTYPE handler(int signum) if(signum==SIGSEGV) { /* write a log message and write to stderr */ - pm_printf(PM_LOG_ERROR, "segmentation fault\n"); - pm_fprintf(stderr, PM_LOG_ERROR, "Internal pacman error: Segmentation fault.\n" - "Please submit a full bug report with --debug if appropriate.\n"); + pm_printf(PM_LOG_ERROR, _("segmentation fault\n")); + pm_fprintf(stderr, PM_LOG_ERROR, _("Internal pacman error: Segmentation fault.\n") + _("Please submit a full bug report with --debug if appropriate.\n")); I'm not sure if you did this after the fact and didn't compile it, but it didn't quite work. :)
Instead, I've changed it to something like the following: pm_fprintf(stderr, PM_LOG_ERROR, _("line one of text\n" "line two of text\n")); The C compiler takes strings that are on two lines and concatinates them automatically- so it worked in the previous (untranslated) case, but once you changed the two strings to function calls, which is what the _() wrapper really is, it failed because it can't concatinate non-static things. Instead, we can wrap this giant string up in one gettext call.
exit(signum); } else if((signum == SIGINT)) { if(alpm_trans_interrupt() == 0) { diff --git a/src/pacman/query.c b/src/pacman/query.c index 0b6ce0c..74d3ff2 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -62,7 +62,7 @@ static int query_fileowner(alpm_list_t *targets)
/* This code is here for safety only */ if(targets == NULL) { - fprintf(stderr, _("error: no file was specified for --owns\n")); + pm_fprintf(stderr, PM_LOG_ERROR, _("no file was specified for --owns\n")); return(1); }
@@ -75,14 +75,15 @@ static int query_fileowner(alpm_list_t *targets) alpm_list_t *i, *j;
if(stat(filename, &buf) == -1) { - fprintf(stderr, _("error: failed to read file '%s': %s\n"), + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to read file '%s': %s\n"), filename, strerror(errno)); ret++; continue; }
if(S_ISDIR(buf.st_mode)) { - fprintf(stderr, _("error: cannot determine ownership of a directory\n")); + pm_fprintf(stderr, PM_LOG_ERROR, + _("cannot determine ownership of a directory\n")); ret++; continue; } @@ -93,7 +94,7 @@ static int query_fileowner(alpm_list_t *targets) free(dname);
if(!rpath) { - fprintf(stderr, _("error: cannot determine real path for '%s': %s\n"), + pm_fprintf(stderr, PM_LOG_ERROR, _("cannot determine real path for '%s': %s\n"), filename, strerror(errno)); free(rpath); ret++; @@ -133,7 +134,7 @@ static int query_fileowner(alpm_list_t *targets) } } if(!found) { - fprintf(stderr, _("error: No package owns %s\n"), filename); + pm_fprintf(stderr, PM_LOG_ERROR, _("No package owns %s\n"), filename); ret++; } free(rpath); @@ -237,7 +238,7 @@ static int query_group(alpm_list_t *targets) printf("%s %s\n", grpname, alpm_pkg_get_name(alpm_list_getdata(p))); } } else { - fprintf(stderr, _("error: group \"%s\" was not found\n"), grpname); + pm_fprintf(stderr, PM_LOG_ERROR, _("group \"%s\" was not found\n"), grpname); ret++; } } @@ -415,7 +416,7 @@ int pacman_query(alpm_list_t *targets) }
if(pkg == NULL) { - fprintf(stderr, _("error: package \"%s\" not found\n"), strname); + pm_fprintf(stderr, PM_LOG_ERROR, _("package \"%s\" not found\n"), strname); ret++; continue; } diff --git a/src/pacman/remove.c b/src/pacman/remove.c index a2209ac..4fe9bc8 100644 --- a/src/pacman/remove.c +++ b/src/pacman/remove.c @@ -86,7 +86,7 @@ int pacman_remove(alpm_list_t *targets) for(i = finaltargs; i; i = alpm_list_next(i)) { char *targ = alpm_list_getdata(i); if(alpm_trans_addtarget(targ) == -1) { - fprintf(stderr, _("error: '%s': %s\n"), + pm_fprintf(stderr, PM_LOG_ERROR, "'%s': %s\n", targ, alpm_strerrorlast()); trans_release(); FREELIST(finaltargs); @@ -96,7 +96,7 @@ int pacman_remove(alpm_list_t *targets)
/* Step 2: prepare the transaction based on its type, targets and flags */ if(alpm_trans_prepare(&data) == -1) { - fprintf(stderr, _("error: failed to prepare transaction (%s)\n"), + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to prepare transaction (%s)\n"), alpm_strerrorlast()); switch(pm_errno) { case PM_ERR_UNSATISFIED_DEPS: @@ -142,7 +142,7 @@ int pacman_remove(alpm_list_t *targets)
/* Step 3: actually perform the removal */ if(alpm_trans_commit(NULL) == -1) { - fprintf(stderr, _("error: failed to commit transaction (%s)\n"), + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to commit transaction (%s)\n"), alpm_strerrorlast()); trans_release(); FREELIST(finaltargs); diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 3c6dfd5..a41b79e 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -46,7 +46,7 @@ static int sync_cleandb(const char *dbpath, int keep_used) {
dir = opendir(dbpath); if(dir == NULL) { - fprintf(stderr, _("error: could not access database directory\n")); + pm_fprintf(stderr, PM_LOG_ERROR, _("could not access database directory\n")); return(1); }
@@ -90,7 +90,8 @@ static int sync_cleandb(const char *dbpath, int keep_used) { }
if(rmrf(path)) { - fprintf(stderr, _("error: could not remove repository directory\n")); + pm_fprintf(stderr, PM_LOG_ERROR, + _("could not remove repository directory\n")); return(1); } } @@ -151,7 +152,7 @@ static int sync_cleancache(int level)
dir = opendir(cachedir); if(dir == NULL) { - fprintf(stderr, _("error: could not access cache directory\n")); + pm_fprintf(stderr, PM_LOG_ERROR, _("could not access cache directory\n")); return(1); }
@@ -220,12 +221,12 @@ static int sync_cleancache(int level) printf(_("removing all packages from cache... "));
if(rmrf(cachedir)) { - fprintf(stderr, _("error: could not remove cache directory\n")); + pm_fprintf(stderr, PM_LOG_ERROR, _("could not remove cache directory\n")); return(1); }
if(makepath(cachedir)) { - fprintf(stderr, _("error: could not create new cache directory\n")); + pm_fprintf(stderr, PM_LOG_ERROR, _("could not create new cache directory\n")); return(1); } printf(_("done.\n")); @@ -248,7 +249,7 @@ static int sync_synctree(int level, alpm_list_t *syncs)
ret = alpm_db_update((level < 2 ? 0 : 1), db); if(ret < 0) { - fprintf(stderr, _("error: failed to update %s (%s)\n"), + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to update %s (%s)\n"), alpm_db_get_name(db), alpm_strerrorlast()); } else if(ret == 1) { printf(_(" %s is up to date\n"), alpm_db_get_name(db)); @@ -266,7 +267,7 @@ static int sync_synctree(int level, alpm_list_t *syncs) * expected */ if(!success) { - fprintf(stderr, _("error: failed to synchronize any databases\n")); + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to synchronize any databases\n")); } return(success > 0); } @@ -418,7 +419,8 @@ static int sync_info(alpm_list_t *syncs, alpm_list_t *targets) }
if(!db) { - fprintf(stderr, _("error: repository '%s' does not exist\n"), repo); + pm_fprintf(stderr, PM_LOG_ERROR, + _("repository '%s' does not exist\n"), repo); return(1); }
@@ -433,7 +435,8 @@ static int sync_info(alpm_list_t *syncs, alpm_list_t *targets) }
if(!foundpkg) { - fprintf(stderr, _("error: package '%s' was not found in repository '%s'\n"), pkgstr, repo); + pm_fprintf(stderr, PM_LOG_ERROR, + _("package '%s' was not found in repository '%s'\n"), pkgstr, repo); ret++; } } else { @@ -453,7 +456,8 @@ static int sync_info(alpm_list_t *syncs, alpm_list_t *targets) } } if(!foundpkg) { - fprintf(stderr, _("error: package '%s' was not found\n"), pkgstr); + pm_fprintf(stderr, PM_LOG_ERROR, + _("package '%s' was not found\n"), pkgstr); ret++; } } @@ -490,7 +494,8 @@ static int sync_list(alpm_list_t *syncs, alpm_list_t *targets) }
if(db == NULL) { - fprintf(stderr, _("error: repository \"%s\" was not found.\n"),repo); + pm_fprintf(stderr, PM_LOG_ERROR, + _("repository \"%s\" was not found.\n"),repo); alpm_list_free(ls); return(1); } @@ -538,7 +543,7 @@ static int sync_trans(alpm_list_t *targets) printf(_(":: Starting full system upgrade...\n")); alpm_logaction("starting full system upgrade\n"); if(alpm_trans_sysupgrade() == -1) { - fprintf(stderr, _("error: %s\n"), alpm_strerrorlast()); + pm_fprintf(stderr, PM_LOG_ERROR, "%s\n", alpm_strerrorlast()); retval = 1; goto cleanup; } @@ -567,7 +572,8 @@ static int sync_trans(alpm_list_t *targets) return(1); } if(alpm_trans_addtarget("pacman") == -1) { - fprintf(stderr, _("error: pacman: %s\n"), alpm_strerrorlast()); + pm_fprintf(stderr, PM_LOG_ERROR, _("pacman: %s\n"), + alpm_strerrorlast()); return(1); } break; @@ -591,7 +597,7 @@ static int sync_trans(alpm_list_t *targets) continue; } if(pm_errno != PM_ERR_PKG_NOT_FOUND) { - fprintf(stderr, _("error: '%s': %s\n"), + pm_fprintf(stderr, PM_LOG_ERROR, "'%s': %s\n", targ, alpm_strerrorlast()); retval = 1; goto cleanup; @@ -647,7 +653,8 @@ static int sync_trans(alpm_list_t *targets) targets = alpm_list_add(targets, strdup(pname)); } else { alpm_list_t *k; - fprintf(stderr, _("error: several packages provide %s, please specify one :\n"), targ); + pm_fprintf(stderr, PM_LOG_ERROR, + _("several packages provide %s, please specify one :\n"), targ); for(k = prov; k; k = alpm_list_next(k)) { pmpkg_t *pkg = alpm_list_getdata(k); printf("%s ", alpm_pkg_get_name(pkg)); @@ -658,7 +665,8 @@ static int sync_trans(alpm_list_t *targets) goto cleanup; } } else { - fprintf(stderr, _("error: '%s': not found in sync db\n"), targ); + pm_fprintf(stderr, PM_LOG_ERROR, + _("'%s': not found in sync db\n"), targ); retval = 1; goto cleanup; } @@ -669,7 +677,7 @@ static int sync_trans(alpm_list_t *targets)
/* Step 2: "compute" the transaction based on targets and flags */ if(alpm_trans_prepare(&data) == -1) { - fprintf(stderr, _("error: failed to prepare transaction (%s)\n"), + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to prepare transaction (%s)\n"), alpm_strerrorlast()); switch(pm_errno) { alpm_list_t *i; @@ -722,7 +730,7 @@ static int sync_trans(alpm_list_t *targets)
/* Step 3: actually perform the installation */ if(alpm_trans_commit(&data) == -1) { - fprintf(stderr, _("error: failed to commit transaction (%s)\n"), + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to commit transaction (%s)\n"), alpm_strerrorlast()); switch(pm_errno) { alpm_list_t *i; diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index 589970e..927f1f4 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -73,7 +73,7 @@ int pacman_upgrade(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: '%s': %s\n"), + pm_fprintf(stderr, PM_LOG_ERROR, "'%s': %s\n", targ, alpm_strerrorlast()); trans_release(); return(1); @@ -83,7 +83,7 @@ int pacman_upgrade(alpm_list_t *targets) /* Step 2: "compute" the transaction based on targets and flags */ /* TODO: No, compute nothing. This is stupid. */ if(alpm_trans_prepare(&data) == -1) { - fprintf(stderr, _("error: failed to prepare transaction (%s)\n"), + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to prepare transaction (%s)\n"), alpm_strerrorlast()); switch(pm_errno) { case PM_ERR_UNSATISFIED_DEPS: @@ -137,7 +137,8 @@ int pacman_upgrade(alpm_list_t *targets)
/* Step 3: perform the installation */ if(alpm_trans_commit(NULL) == -1) { - fprintf(stderr, _("error: failed to commit transaction (%s)\n"), alpm_strerrorlast()); + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to commit transaction (%s)\n"), + alpm_strerrorlast()); trans_release(); return(1); } diff --git a/src/pacman/util.c b/src/pacman/util.c index f1f098f..2e4ee86 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -49,7 +49,7 @@ int trans_init(pmtranstype_t type, pmtransflag_t flags) { if(alpm_trans_init(type, flags, cb_trans_evt, cb_trans_conv, cb_trans_progress) == -1) { - fprintf(stderr, _("error: failed to init transaction (%s)\n"), + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to init transaction (%s)\n"), alpm_strerrorlast()); if(pm_errno == PM_ERR_HANDLE_LOCK) { fprintf(stderr, _(" if you're sure a package manager is not already\n" @@ -63,7 +63,7 @@ int trans_init(pmtranstype_t type, pmtransflag_t flags) int trans_release() { if(alpm_trans_release() == -1) { - fprintf(stderr, _("error: failed to release transaction (%s)\n"), + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to release transaction (%s)\n"), alpm_strerrorlast()); return(-1); } --
The rest was great, minus a few trailing whitespace errors: $ git am -3 -s < /tmp/pm-fprintf-fixes.patch Applying Make all error messages use pm_fprintf .dotest/patch:35: trailing whitespace. pm_fprintf(stderr, PM_LOG_ERROR, .dotest/patch:183: trailing whitespace. pm_fprintf(stderr, PM_LOG_ERROR, .dotest/patch:235: trailing whitespace. pm_fprintf(stderr, PM_LOG_ERROR, .dotest/patch:245: trailing whitespace. pm_fprintf(stderr, PM_LOG_ERROR, .dotest/patch:255: trailing whitespace. pm_fprintf(stderr, PM_LOG_ERROR, warning: squelched 5 whitespace errors warning: 10 lines add whitespace errors. To get this output, I have +x set on .git/hooks/pre-commit, which catches some stupid mistakes you may make when you commit. I would recommend most people do the following: chmod +x .git/hooks/{applypatch-msg,commit-message,pre-commit,pre-rebase} -Dan