Signed-off-by: Allan McRae allan@archlinux.org --- doc/index.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/doc/index.asciidoc b/doc/index.asciidoc index 7d30e11d..415027df 100644 --- a/doc/index.asciidoc +++ b/doc/index.asciidoc @@ -75,7 +75,7 @@ Releases [frame="topbot",grid="none",options="header,autowidth"] !====== !Version !Date -!5.1.0 !2017-05-28 +!5.1.0 !2018-05-28 !5.0.1 !2016-02-23 !5.0.0 !2016-01-30 !4.2.1 !2015-02-20
The arguments for the --overwrite option requried the user to strip the leading "/" from the path. It is more intuative to provide the whole path and have pacman strip the leading "/" before passing to the backend.
Signed-off-by: Allan McRae allan@archlinux.org --- src/pacman/pacman.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index fe54793e..d90a9f6c 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -723,7 +723,16 @@ static int parsearg_upgrade(int opt) config->flags |= ALPM_TRANS_FLAG_FORCE; break; case OP_OVERWRITE_FILES: - parsearg_util_addlist(&(config->overwrite_files)); + { + char *i, *save = NULL; + for(i = strtok_r(optarg, ",", &save); i; i = strtok_r(NULL, ",", &save)) { + /* strip leading "/" before adding to option list */ + while(i[0] == '/') { + i = i + 1; + } + config->overwrite_files = alpm_list_add(config->overwrite_files, strdup(i)); + } + } break; case OP_ASDEPS: config->flags |= ALPM_TRANS_FLAG_ALLDEPS;
On Tue May 29 04:28:28 UTC 2018, Allan McRae wrote:
The arguments for the --overwrite option requried the user to strip the leading "/" from the path. It is more intuative to provide the whole path and have pacman strip the leading "/" before passing to the backend.
...
- while(i[0] == '/') {
i = i + 1;- }
Eli pointed out that you had already submitted a patch for this and mine touched a bit too much, and I agree.
I do like your approach more, but I am still of the opinion that something like:
i += strspn(i, "/")
would be much simpler (and maintain equivalent semantics) in place of that loop.
On 05/29/18 at 02:28pm, Allan McRae wrote:
The arguments for the --overwrite option requried the user to strip the leading "/" from the path. It is more intuative to provide the whole path and have pacman strip the leading "/" before passing to the backend.
Signed-off-by: Allan McRae allan@archlinux.org
src/pacman/pacman.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index fe54793e..d90a9f6c 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -723,7 +723,16 @@ static int parsearg_upgrade(int opt) config->flags |= ALPM_TRANS_FLAG_FORCE; break; case OP_OVERWRITE_FILES:
parsearg_util_addlist(&(config->overwrite_files));
{char *i, *save = NULL;for(i = strtok_r(optarg, ",", &save); i; i = strtok_r(NULL, ",", &save)) {/* strip leading "/" before adding to option list */while(i[0] == '/') {i = i + 1;}config->overwrite_files = alpm_list_add(config->overwrite_files, strdup(i));}} break;-- 2.17.0
If we're going to allow absolute paths, should we not be removing the full root, not just '/'?
On Fri, Jun 01, 2018 at 05:13:37PM -0400, Andrew Gregory wrote:
If we're going to allow absolute paths, should we not be removing the full root, not just '/'?
Did you mean strip out the sysroot in the event it happens to not be "/"? In my opinion, although doable, the logic you'd need to add would make this far more complex than just stripping out leading "/", and how scarce that particular situation is in reality make this not really seem like a worthwhile trade-off to me.
On 06/01/18 at 11:41am, Joey Pabalinas wrote:
On Fri, Jun 01, 2018 at 05:13:37PM -0400, Andrew Gregory wrote:
If we're going to allow absolute paths, should we not be removing the full root, not just '/'?
Did you mean strip out the sysroot in the event it happens to not be "/"? In my opinion, although doable, the logic you'd need to add would make this far more complex than just stripping out leading "/", and how scarce that particular situation is in reality make this not really seem like a worthwhile trade-off to me.
Not the sysroot, rootdir. I don't see how that would be overly complex, we do it in other places already.
On Fri, Jun 01, 2018 at 05:45:11PM -0400, Andrew Gregory wrote:
Not the sysroot, rootdir. I don't see how that would be overly complex, we do it in other places already.
Isn't --root deprecated in favor of --sysroot though? (according to the manpage at least).
On 06/01/18 at 12:44pm, Joey Pabalinas wrote:
On Fri, Jun 01, 2018 at 05:45:11PM -0400, Andrew Gregory wrote:
Not the sysroot, rootdir. I don't see how that would be overly complex, we do it in other places already.
Isn't --root deprecated in favor of --sysroot though? (according to the manpage at least).
Sort of. The current --root option is a confusing mess that nobody actually understands, so it will go away at some point. The underlying libalpm rootdir setting isn't going anywhere though, and, in the future, will be configured with a new --rootdir option.
On Fri, Jun 01, 2018 at 06:56:13PM -0400, Andrew Gregory wrote:
Sort of. The current --root option is a confusing mess that nobody actually understands, so it will go away at some point. The underlying libalpm rootdir setting isn't going anywhere though, and, in the future, will be configured with a new --rootdir option.
Well looking at the code it actually isn't as complicated as I thought it would be.
How about something like this:
case OP_OVERWRITE_FILES: { char *i, *root = config->rootdir, *save = NULL; for(i = strtok_r(optarg, ",", &save); i; i = strtok_r(NULL, ",", &save)) { /* strip rootdir if applicable */ if (root && !memcmp(i, root, strlen(root))) i += strlen(root); /* strip remaining leading "/" before adding to option list */ i += strspn(i, "/"); config->overwrite_files = alpm_list_add(config->overwrite_files, strdup(i)); } }
which would strip the rootdir and then the leading "/". Although I am not 100% certain config->rootdir would be NULL if no argument is passed; could you confirm that part?
On 02/06/18 09:01, Joey Pabalinas wrote:
On Fri, Jun 01, 2018 at 06:56:13PM -0400, Andrew Gregory wrote:
Sort of. The current --root option is a confusing mess that nobody actually understands, so it will go away at some point. The underlying libalpm rootdir setting isn't going anywhere though, and, in the future, will be configured with a new --rootdir option.
Well looking at the code it actually isn't as complicated as I thought it would be.
How about something like this:
case OP_OVERWRITE_FILES: { char *i, *root = config->rootdir, *save = NULL; for(i = strtok_r(optarg, ",", &save); i; i = strtok_r(NULL, ",", &save)) { /* strip rootdir if applicable */ if (root && !memcmp(i, root, strlen(root))) i += strlen(root); /* strip remaining leading "/" before adding to option list */ i += strspn(i, "/"); config->overwrite_files = alpm_list_add(config->overwrite_files, strdup(i)); } }
which would strip the rootdir and then the leading "/". Although I am not 100% certain config->rootdir would be NULL if no argument is passed; could you confirm that part?
This will not work - we need to handle this after the config file is read as the root dir may be set there.
A
Signed-off-by: Allan McRae allan@archlinux.org --- lib/libalpm/alpm.h | 3 +-- lib/libalpm/conflict.c | 3 +-- src/pacman/pacman.c | 5 ----- src/pacman/sync.c | 3 --- test/pacman/tests/TESTS | 5 ----- test/pacman/tests/upgrade012.py | 14 -------------- test/pacman/tests/upgrade014.py | 23 ----------------------- test/pacman/tests/upgrade015.py | 14 -------------- test/pacman/tests/upgrade016.py | 15 --------------- test/pacman/tests/upgrade046.py | 31 ------------------------------- 10 files changed, 2 insertions(+), 114 deletions(-) delete mode 100644 test/pacman/tests/upgrade012.py delete mode 100644 test/pacman/tests/upgrade014.py delete mode 100644 test/pacman/tests/upgrade015.py delete mode 100644 test/pacman/tests/upgrade016.py delete mode 100644 test/pacman/tests/upgrade046.py
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 57226ebd..2d3d198a 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -1461,8 +1461,7 @@ alpm_pkg_t *alpm_sync_newversion(alpm_pkg_t *pkg, alpm_list_t *dbs_sync); typedef enum _alpm_transflag_t { /** Ignore dependency checks. */ ALPM_TRANS_FLAG_NODEPS = 1, - /** Ignore file conflicts and overwrite files. */ - ALPM_TRANS_FLAG_FORCE = (1 << 1), + /* (1 << 1) flag can go here */ /** Delete files even if they are tagged as backup. */ ALPM_TRANS_FLAG_NOSAVE = (1 << 2), /** Ignore version numbers when checking dependencies. */ diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index 35946de5..d871631a 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -401,8 +401,7 @@ static alpm_pkg_t *_alpm_find_file_owner(alpm_handle_t *handle, const char *path
static int _alpm_can_overwrite_file(alpm_handle_t *handle, const char *path) { - return handle->trans->flags & ALPM_TRANS_FLAG_FORCE - || _alpm_fnmatch_patterns(handle->overwrite_files, path) == 0; + return _alpm_fnmatch_patterns(handle->overwrite_files, path) == 0; }
/** diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index d90a9f6c..8e41e441 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -717,11 +717,6 @@ static int parsearg_upgrade(int opt) return 0; } switch(opt) { - case OP_FORCE: - pm_printf(ALPM_LOG_WARNING, - _("option --force is deprecated; use --overwrite instead\n")); - config->flags |= ALPM_TRANS_FLAG_FORCE; - break; case OP_OVERWRITE_FILES: { char *i, *save = NULL; diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 3c6be89d..ef8faedf 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -813,9 +813,6 @@ int sync_prepare_execute(void) alpm_strerror(err)); switch(err) { case ALPM_ERR_FILE_CONFLICTS: - if(config->flags & ALPM_TRANS_FLAG_FORCE) { - printf(_("unable to %s directory-file conflicts\n"), "--force"); - } for(i = data; i; i = alpm_list_next(i)) { alpm_fileconflict_t *conflict = i->data; switch(conflict->type) { diff --git a/test/pacman/tests/TESTS b/test/pacman/tests/TESTS index a9b4288c..b11cb511 100644 --- a/test/pacman/tests/TESTS +++ b/test/pacman/tests/TESTS @@ -276,11 +276,7 @@ TESTS += test/pacman/tests/upgrade005.py TESTS += test/pacman/tests/upgrade006.py TESTS += test/pacman/tests/upgrade010.py TESTS += test/pacman/tests/upgrade011.py -TESTS += test/pacman/tests/upgrade012.py TESTS += test/pacman/tests/upgrade013.py -TESTS += test/pacman/tests/upgrade014.py -TESTS += test/pacman/tests/upgrade015.py -TESTS += test/pacman/tests/upgrade016.py TESTS += test/pacman/tests/upgrade020.py TESTS += test/pacman/tests/upgrade021.py TESTS += test/pacman/tests/upgrade022.py @@ -299,7 +295,6 @@ TESTS += test/pacman/tests/upgrade041.py TESTS += test/pacman/tests/upgrade042.py TESTS += test/pacman/tests/upgrade043.py TESTS += test/pacman/tests/upgrade045.py -TESTS += test/pacman/tests/upgrade046.py TESTS += test/pacman/tests/upgrade050.py TESTS += test/pacman/tests/upgrade051.py TESTS += test/pacman/tests/upgrade052.py diff --git a/test/pacman/tests/upgrade012.py b/test/pacman/tests/upgrade012.py deleted file mode 100644 index 4d9f0cd1..00000000 --- a/test/pacman/tests/upgrade012.py +++ /dev/null @@ -1,14 +0,0 @@ -self.description = "Install a package with a filesystem conflict (--force)" - -p = pmpkg("dummy") -p.files = ["bin/dummy", "usr/man/man1/dummy.1"] -self.addpkg(p) - -self.filesystem = ["bin/dummy"] - -self.args = "-U --force %s" % p.filename() - -self.addrule("PACMAN_RETCODE=0") -self.addrule("PKG_EXIST=dummy") -self.addrule("FILE_MODIFIED=bin/dummy") -self.addrule("FILE_EXIST=usr/man/man1/dummy.1") diff --git a/test/pacman/tests/upgrade014.py b/test/pacman/tests/upgrade014.py deleted file mode 100644 index 2ef759d1..00000000 --- a/test/pacman/tests/upgrade014.py +++ /dev/null @@ -1,23 +0,0 @@ -self.description = "Install two packages with a conflicting file (--force)" - -p1 = pmpkg("dummy") -p1.files = ["bin/dummy", - "usr/man/man1/dummy.1", - "usr/common"] - -p2 = pmpkg("foobar") -p2.files = ["bin/foobar", - "usr/man/man1/foobar.1", - "usr/common"] - -for p in p1, p2: - self.addpkg(p) - -self.args = "-U --force %s" % " ".join([p.filename() for p in (p1, p2)]) - -self.addrule("PACMAN_RETCODE=0") -for p in p1, p2: - self.addrule("PKG_EXIST=%s" % p.name) - self.addrule("PKG_FILES=%s|usr/common" % p.name) - for f in p.files: - self.addrule("FILE_EXIST=%s" % f) diff --git a/test/pacman/tests/upgrade015.py b/test/pacman/tests/upgrade015.py deleted file mode 100644 index 64fe2813..00000000 --- a/test/pacman/tests/upgrade015.py +++ /dev/null @@ -1,14 +0,0 @@ -self.description = "Install a package with an existing file (--force)" - -p = pmpkg("dummy") -p.files = ["etc/dummy.conf"] -self.addpkg(p) - -self.filesystem = ["etc/dummy.conf"] - -self.args = "-U --force %s" % p.filename() - -self.addrule("PACMAN_RETCODE=0") -self.addrule("PKG_EXIST=dummy") -self.addrule("FILE_MODIFIED=etc/dummy.conf") -self.addrule("!FILE_PACNEW=etc/dummy.conf") diff --git a/test/pacman/tests/upgrade016.py b/test/pacman/tests/upgrade016.py deleted file mode 100644 index ddf57e8c..00000000 --- a/test/pacman/tests/upgrade016.py +++ /dev/null @@ -1,15 +0,0 @@ -self.description = "Install a package with an existing file (--force, new modified)" - -p = pmpkg("dummy") -p.files = ["etc/dummy.conf*"] -p.backup = ["etc/dummy.conf"] -self.addpkg(p) - -self.filesystem = ["etc/dummy.conf"] - -self.args = "-U --force %s" % p.filename() - -self.addrule("PACMAN_RETCODE=0") -self.addrule("PKG_EXIST=dummy") -self.addrule("!FILE_MODIFIED=etc/dummy.conf") -self.addrule("FILE_PACNEW=etc/dummy.conf") diff --git a/test/pacman/tests/upgrade046.py b/test/pacman/tests/upgrade046.py deleted file mode 100644 index 0b59ca66..00000000 --- a/test/pacman/tests/upgrade046.py +++ /dev/null @@ -1,31 +0,0 @@ -self.description = "File relocation between two packages (reverse order, --force)" - -lp1 = pmpkg("dummy") -lp1.files = ["bin/dummy"] - -lp2 = pmpkg("foobar") -lp2.files = ["bin/foobar", - "usr/share/file"] - -for p in lp1, lp2: - self.addpkg2db("local", p) - -p1 = pmpkg("dummy") -p1.files = ["bin/dummy", - "usr/share/file"] - -p2 = pmpkg("foobar") -p2.files = ["bin/foobar"] - -for p in p1, p2: - self.addpkg(p) - -self.args = "-U --force %s" % " ".join([p.filename() for p in (p1, p2)]) - -self.addrule("PACMAN_RETCODE=0") -for p in p1, p2: - self.addrule("PKG_EXIST=%s" % p.name) -self.addrule("FILE_MODIFIED=bin/dummy") -self.addrule("FILE_MODIFIED=bin/foobar") -self.addrule("FILE_EXIST=usr/share/file") -self.addrule("FILE_MODIFIED=usr/share/file")
pacman-dev@lists.archlinux.org