diff mbox series

[pacman-dev,v2] pacman: print error when -Fx is given invalid regex

Message ID 20191127174519.267931-1-morganamilo@archlinux.org
State Accepted, archived
Headers show
Series [pacman-dev,v2] pacman: print error when -Fx is given invalid regex | expand

Commit Message

morganamilo Nov. 27, 2019, 5:45 p.m. UTC 
When processing the targets for -Fx, compile all the regex ahead of
    time, printing an error for each that failed to compile. Then, if they all
    compiled successfully, continue with printing files.

    Signed-off-by: morganamilo <morganamilo@archlinux.org>

    ---

    v2:
            Add comment about why we don't free targ
            Fix whitespace error

    I have vim set to display trialing whitespace so normally I catch it.
    Not sure why I didn't this time.

    Also I agree that running all the regex against each target before
    moving on to the next is a good improvement. This patch is already kinda
    long though, so I'll do it another time.

Comments

Allan McRae Dec. 2, 2019, 3:49 a.m. UTC | #1 
On 28/11/19 3:45 am, morganamilo wrote:
>     When processing the targets for -Fx, compile all the regex ahead of
>     time, printing an error for each that failed to compile. Then, if they all
>     compiled successfully, continue with printing files.
> 
>     Signed-off-by: morganamilo <morganamilo@archlinux.org>
> 
>     ---
> 
>     v2:
>             Add comment about why we don't free targ
>             Fix whitespace error
> 
>     I have vim set to display trialing whitespace so normally I catch it.
>     Not sure why I didn't this time.
> 
>     Also I agree that running all the regex against each target before
>     moving on to the next is a good improvement. This patch is already kinda
>     long though, so I'll do it another time.
> 
> diff --git a/src/pacman/files.c b/src/pacman/files.c
> index 3b6dc23b..6fcc6e9b 100644
> --- a/src/pacman/files.c
> +++ b/src/pacman/files.c
> @@ -94,17 +94,27 @@ static void print_match(alpm_list_t *match, alpm_db_t *repo, alpm_pkg_t *pkg, in
>  	}
>  }
>  
> +struct filetarget {
> +	char *targ;
> +	int exact_file;
> +	regex_t reg;
> +};
> +
> +static void filetarget_free(struct filetarget *ftarg) {
> +	regfree(&ftarg->reg);
> +	/* we don't own ftarg->targ so don't free it */

Patch looks good.

I changed that comment to:

do not free ftarg->targ as it is owned by the caller of files_search

This does a few useful things:
1) gets the "do not free" right at the start of the comment
2) specifies where the memory is owned
3) does not use contractions - apparently the use of "not" make the
negative clearer for non-native speakers, although I am not sure what
sort of evidence is behind that is claim...

Allan
diff mbox series

Patch

diff --git a/src/pacman/files.c b/src/pacman/files.c
index 3b6dc23b..6fcc6e9b 100644
--- a/src/pacman/files.c
+++ b/src/pacman/files.c
@@ -94,17 +94,27 @@  static void print_match(alpm_list_t *match, alpm_db_t *repo, alpm_pkg_t *pkg, in
 	}
 }
 
+struct filetarget {
+	char *targ;
+	int exact_file;
+	regex_t reg;
+};
+
+static void filetarget_free(struct filetarget *ftarg) {
+	regfree(&ftarg->reg);
+	/* we don't own ftarg->targ so don't free it */
+	free(ftarg);
+}
+
 static int files_search(alpm_list_t *syncs, alpm_list_t *targets, int regex) {
 	int ret = 0;
-	alpm_list_t *t;
+	alpm_list_t *t, *filetargs = NULL;
 
 	for(t = targets; t; t = alpm_list_next(t)) {
 		char *targ = t->data;
-		alpm_list_t *s;
-		int found = 0;
-		regex_t reg;
 		size_t len = strlen(targ);
 		int exact_file = strchr(targ, '/') != NULL;
+		regex_t reg;
 
 		if(exact_file) {
 			while(len > 1 && targ[0] == '/') {
@@ -115,11 +125,33 @@  static int files_search(alpm_list_t *syncs, alpm_list_t *targets, int regex) {
 
 		if(regex) {
 			if(regcomp(&reg, targ, REG_EXTENDED | REG_NOSUB | REG_ICASE | REG_NEWLINE) != 0) {
-				/* TODO: error message */
-				goto notfound;
+				pm_printf(ALPM_LOG_ERROR,
+						_("invalid regular expression '%s'\n"), targ);
+				ret = 1;
+				continue;
 			}
 		}
 
+		struct filetarget *ftarg = malloc(sizeof(struct filetarget));
+		ftarg->targ = targ;
+		ftarg->exact_file = exact_file;
+		ftarg->reg = reg;
+
+		filetargs = alpm_list_add(filetargs, ftarg);
+	}
+
+	if(ret != 0) {
+		goto cleanup;
+	}
+
+	for(t = filetargs; t; t = alpm_list_next(t)) {
+		struct filetarget *ftarg = t->data;
+		char *targ = ftarg->targ;
+		regex_t *reg = &ftarg->reg;
+		int exact_file = ftarg->exact_file;
+		alpm_list_t *s;
+		int found = 0;
+
 		for(s = syncs; s; s = alpm_list_next(s)) {
 			alpm_list_t *p;
 			alpm_db_t *repo = s->data;
@@ -135,7 +167,7 @@  static int files_search(alpm_list_t *syncs, alpm_list_t *targets, int regex) {
 					if (regex) {
 						for(size_t f = 0; f < files->count; f++) {
 							char *c = files->files[f].name;
-							if(regexec(&reg, c, 0, 0, 0) == 0) {
+							if(regexec(reg, c, 0, 0, 0) == 0) {
 								match = alpm_list_add(match, files->files[f].name);
 								found = 1;
 							}
@@ -151,7 +183,7 @@  static int files_search(alpm_list_t *syncs, alpm_list_t *targets, int regex) {
 						char *c = strrchr(files->files[f].name, '/');
 						if(c && *(c + 1)) {
 							if(regex) {
-								m = regexec(&reg, (c + 1), 0, 0, 0);
+								m = regexec(reg, (c + 1), 0, 0, 0);
 							} else {
 								m = strcmp(c + 1, targ);
 							}
@@ -170,16 +202,15 @@  static int files_search(alpm_list_t *syncs, alpm_list_t *targets, int regex) {
 			}
 		}
 
-		if(regex) {
-			regfree(&reg);
-		}
-
-notfound:
 		if(!found) {
 			ret = 1;
 		}
 	}
 
+cleanup:
+	alpm_list_free_inner(filetargs, (alpm_list_fn_free) filetarget_free);
+	alpm_list_free(filetargs);
+
 	return ret;
 }