[pacman-dev] [PATCH] Apparently we live in the future!
Signed-off-by: Allan McRae
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
Signed-off-by: Allan McRae
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. -- Cheers, Joey Pabalinas
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
--- 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; -- 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. -- Cheers, Joey Pabalinas
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). -- Cheers, Joey Pabalinas
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? -- Cheers, Joey Pabalinas
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
participants (3)
-
Allan McRae
-
Andrew Gregory
-
Joey Pabalinas