diff mbox

[pacman-dev] Strip leading "/" from arguments to --overwrite

Message ID 20180529042829.6102-2-allan@archlinux.org
State Superseded, archived
Headers show

Commit Message

Allan McRae May 29, 2018, 4:28 a.m. UTC
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(-)

Comments

Joey Pabalinas June 1, 2018, 12:09 p.m. UTC | #1
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.
Andrew Gregory June 1, 2018, 9:13 p.m. UTC | #2
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;
>  		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 '/'?
Joey Pabalinas June 1, 2018, 9:41 p.m. UTC | #3
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.
Andrew Gregory June 1, 2018, 9:45 p.m. UTC | #4
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.
Joey Pabalinas June 1, 2018, 10:44 p.m. UTC | #5
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).
Andrew Gregory June 1, 2018, 10:56 p.m. UTC | #6
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.
Joey Pabalinas June 1, 2018, 11:01 p.m. UTC | #7
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?
Allan McRae June 4, 2018, 10:22 a.m. UTC | #8
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
diff mbox

Patch

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;