On 21/1/20 6:03 am, Hamza Mogni wrote:

I found this TODO while browsing pacman's source code, tried fixing it by checking returned errors while deleting packages files and logging accordingly.

This is my first time editing code and submitting a patch; SORRY if I did anything dumb!

Signed-off-by: Hamza Mogni <hamzamogni5@gmail.com>

Hi Hazma, Thanks for your patch and sorry for the delay in review. Firstly, the commit message should be used to describe the change you are doing. What you have written can be added to the patch...

---

... under this line!

lib/libalpm/remove.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index 9030bfee..4a19c718 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -707,7 +707,12 @@ int _alpm_remove_single_package(alpm_handle_t *handle,

if(!(handle->trans->flags & ALPM_TRANS_FLAG_DBONLY)) { /* TODO check returned errors if any */ - remove_package_files(handle, oldpkg, newpkg, targ_count, pkg_count); + int err = remove_package_files(handle, oldpkg, newpkg, targ_count, pkg_count); + if (err == -1) { + _alpm_log(handle, ALPM_LOG_ERROR, "alpm lacks permission to delete all files");

Good that you identified both error conditions. This TODO is actually a lot bigger than it seems! It was more about what should we do in this situation. We have failed to remove the old package, printing an error message and trying to install the updated version (if any) is not ideal. We also should not continue through this function with removing it from the local package database. So overall, not an ideal starting place for a first contribution, and a lot more would need done here. FYI, output strings need to be translated. So surround in _("output")

+ } else if (err > 0) { + _alpm_log(handle, ALPM_LOG_ERROR, "failed to delete %d files", err);

This string is more fun... "1 files" does not make sense, so we can provide multiple versions of the string. Look for _n("one", "many") style output strings.

+ } }

if(!newpkg) {

Cheers, Allan