git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v1 3/5] list-objects-filter: implement composite filters

From
Matthew DeVore <matvore@comcast.net>
Date
Jun 3, 2019, 22:22 UTC
Message-ID
<20190603222247.GG4641@comcast.net>
In-Reply-To
<20190603123435.GA18953@sigill.intra.peff.net>
On Mon, Jun 03, 2019 at 08:34:35AM -0400, Jeff King wrote:
> Great. We might want to stop there, but it's possible could reuse even
> more code. I didn't look closely before, but it seems this code is
> decoding a URL. We already have a url_decode() routine in url.c. Could
> it be reused?

Very nice. Here is an interdiff and the changes will be included in v3 of my patchset:

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index ed02c88eb6..0f135602a7 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -1,19 +1,20 @@
 #include "cache.h"
 #include "commit.h"
 #include "config.h"
 #include "revision.h"
 #include "argv-array.h"
 #include "list-objects.h"
 #include "list-objects-filter.h"
 #include "list-objects-filter-options.h"
 #include "trace.h"
+#include "url.h"
 
 static int parse_combine_filter(
 	struct list_objects_filter_options *filter_options,
 	const char *arg,
 	struct strbuf *errbuf);
 
 /*
  * Parse value of the argument to the "filter" keyword.
  * On the command line this looks like:
  *       --filter=<arg>
@@ -84,54 +85,20 @@ static int gently_parse_list_objects_filter(
 	 * Please update _git_fetch() in git-completion.bash when you
 	 * add new filters
 	 */
 
 	strbuf_addf(errbuf, "invalid filter-spec '%s'", arg);
 
 	memset(filter_options, 0, sizeof(*filter_options));
 	return 1;
 }
 
-static int url_decode(struct strbuf *s, struct strbuf *errbuf)
-{
-	char *dest = s->buf;
-	char *src = s->buf;
-	size_t new_len;
-
-	while (*src) {
-		if (src[0] != '%') {
-			*dest++ = *src++;
-			continue;
-		}
-
-		if (hex_to_bytes((unsigned char *)dest, src + 1, 1)) {
-			strbuf_addstr(errbuf,
-				      "error in filter-spec - "
-				      "invalid hex sequence after %");
-			return 1;
-		}
-
-		if (!*dest) {
-			strbuf_addstr(errbuf,
-				      "error in filter-spec - unexpected %00");
-			return 1;
-		}
-
-		src += 3;
-		dest++;
-	}
-	new_len = dest - s->buf;
-	strbuf_remove(s, new_len, s->len - new_len);
-
-	return 0;
-}
-
 static const char *RESERVED_NON_WS = "~`!@#$^&*()[]{}\\;'\",<>?";
 
 static int has_reserved_character(
 	struct strbuf *sub_spec, struct strbuf *errbuf)
 {
 	const char *c = sub_spec->buf;
 	while (*c) {
 		if (*c <= ' ' || strchr(RESERVED_NON_WS, *c)) {
 			strbuf_addf(errbuf,
 				    "must escape char in sub-filter-spec: '%c'",
@@ -147,56 +114,57 @@ static int has_reserved_character(
 static int parse_combine_subfilter(
 	struct list_objects_filter_options *filter_options,
 	struct strbuf *subspec,
 	struct strbuf *errbuf)
 {
 	size_t new_index = filter_options->sub_nr;
 
 	ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,
 		      filter_options->sub_alloc);
 
-	return has_reserved_character(subspec, errbuf) ||
-		url_decode(subspec, errbuf) ||
-		gently_parse_list_objects_filter(
-			&filter_options->sub[new_index], subspec->buf, errbuf);
+	decoded = url_percent_decode(subspec->buf);
+
+	result = gently_parse_list_objects_filter(
+		&filter_options->sub[new_index], decoded, errbuf);
+
+	free(decoded);
+	return result;
 }
 
 static int parse_combine_filter(
 	struct list_objects_filter_options *filter_options,
 	const char *arg,
 	struct strbuf *errbuf)
 {
 	struct strbuf **subspecs = strbuf_split_str(arg, '+', 0);
 	size_t sub;
-	int result;
+	int result = 0;
 
 	if (!subspecs[0]) {
 		strbuf_addf(errbuf,
 			    _("expected something after combine:"));
 		result = 1;
 		goto cleanup;
 	}
 
-	for (sub = 0; subspecs[sub]; sub++) {
+	for (sub = 0; subspecs[sub] && !result; sub++) {
 		if (subspecs[sub + 1]) {
 			/*
 			 * This is not the last subspec. Remove trailing "+" so
 			 * we can parse it.
 			 */
 			size_t last = subspecs[sub]->len - 1;
 			assert(subspecs[sub]->buf[last] == '+');
 			strbuf_remove(subspecs[sub], last, 1);
 		}
 		result = parse_combine_subfilter(
 			filter_options, subspecs[sub], errbuf);
-		if (result)
-			goto cleanup;
 	}
 
 	filter_options->choice = LOFC_COMBINE;
 
 cleanup:
 	strbuf_list_free(subspecs);
 	if (result) {
 		list_objects_filter_release(filter_options);
 		memset(filter_options, 0, sizeof(*filter_options));
 	}
diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
index 7fb5e50cde..e1bf3ed038 100755
--- a/t/t6112-rev-list-filters-objects.sh
+++ b/t/t6112-rev-list-filters-objects.sh
@@ -405,32 +405,20 @@ test_expect_success 'combine:... while URL-encoding things that should not be' '
 
 test_expect_success 'combine: with nothing after the :' '
 	expect_invalid_filter_spec combine: "expected something after combine:"
 '
 
 test_expect_success 'parse error in first sub-filter in combine:' '
 	expect_invalid_filter_spec combine:tree:asdf+blob:none \
 		"expected .tree:<depth>."
 '
 
-test_expect_success 'combine:... with invalid URL-encoded sequences' '
-	# Not enough hex chars
-	expect_invalid_filter_spec combine:tree:2+blob:non%a \
-		"error in filter-spec - invalid hex sequence after %" &&
-	# Non-hex digit after %
-	expect_invalid_filter_spec combine:tree:2+blob%G5none \
-		"error in filter-spec - invalid hex sequence after %" &&
-	# Null byte encoded by %
-	expect_invalid_filter_spec combine:tree:2+blob%00none \
-		"error in filter-spec - unexpected %00"
-'
-
 test_expect_success 'combine:... with non-encoded reserved chars' '
 	expect_invalid_filter_spec combine:tree:2+sparse:@xyz \
 		"must escape char in sub-filter-spec: .@." &&
 	expect_invalid_filter_spec combine:tree:2+sparse:\` \
 		"must escape char in sub-filter-spec: .\`." &&
 	expect_invalid_filter_spec combine:tree:2+sparse:~abc \
 		"must escape char in sub-filter-spec: .\~."
 '
 
 test_expect_success 'validate err msg for "combine:<valid-filter>+"' '
diff --git a/url.c b/url.c
index 25576c390b..bdede647bc 100644
--- a/url.c
+++ b/url.c
@@ -79,20 +79,26 @@ char *url_decode_mem(const char *url, int len)
 
 	/* Skip protocol part if present */
 	if (colon && url < colon) {
 		strbuf_add(&out, url, colon - url);
 		len -= colon - url;
 		url = colon;
 	}
 	return url_decode_internal(&url, len, NULL, &out, 0);
 }
 
+char *url_percent_decode(const char *encoded)
+{
+	struct strbuf out = STRBUF_INIT;
+	return url_decode_internal(&encoded, strlen(encoded), NULL, &out, 0);
+}
+
 char *url_decode_parameter_name(const char **query)
 {
 	struct strbuf out = STRBUF_INIT;
 	return url_decode_internal(query, -1, "&=", &out, 1);
 }
 
 char *url_decode_parameter_value(const char **query)
 {
 	struct strbuf out = STRBUF_INIT;
 	return url_decode_internal(query, -1, "&", &out, 1);
diff --git a/url.h b/url.h
index 00b7d58c33..2a27c34277 100644
--- a/url.h
+++ b/url.h
@@ -1,16 +1,24 @@
 #ifndef URL_H
 #define URL_H
 
 struct strbuf;
 
 int is_url(const char *url);
 int is_urlschemechar(int first_flag, int ch);
 char *url_decode(const char *url);
 char *url_decode_mem(const char *url, int len);
+
+/*
+ * Similar to the url_decode_{,mem} methods above, but doesn't assume there
+ * is a scheme followed by a : at the start of the string. Instead, %-sequences
+ * before any : are also parsed.
+ */
+char *url_percent_decode(const char *encoded);
+
 char *url_decode_parameter_name(const char **query);
 char *url_decode_parameter_value(const char **query);
 
 void end_url_with_slash(struct strbuf *buf, const char *url);
 void str_end_url_with_slash(const char *url, char **dest);
 
 #endif /* URL_H */
Previous: Jeff KingNext: Jeff King
Message 31 of 41 in “Filter combination”
  1. 0/5 Filter combinationMatthew DeVore, May 22, 2019
  2. 1/5 list-objects-filter: refactor into a context structMatthew DeVore, May 22, 2019
  3. Emily ShafferMay 24, 2019
  4. Matthew DeVoreMay 28, 2019
  5. list-objects-filter: merge filter data structsMatthew DeVore, May 28, 2019
  6. Junio C HamanoMay 29, 2019
  7. Jeff HostetlerMay 29, 2019
  8. Matthew DeVoreMay 29, 2019
  9. list-objects-filter: merge filter data structsMatthew DeVore, May 30, 2019
  10. Junio C HamanoMay 30, 2019
  11. Matthew DeVoreMay 30, 2019
  12. Matthew DeVoreMay 30, 2019
  13. 2/5 list-objects-filter-options: error is localizeableMatthew DeVore, May 22, 2019
  14. Emily ShafferMay 24, 2019
  15. Matthew DeVoreMay 28, 2019
  16. 3/5 list-objects-filter: implement composite filtersMatthew DeVore, May 22, 2019
  17. Jeff HostetlerMay 24, 2019
  18. Junio C HamanoMay 28, 2019
  19. Matthew DeVoreMay 29, 2019
  20. Jeff HostetlerMay 29, 2019
  21. Matthew DeVoreMay 29, 2019
  22. Jeff HostetlerMay 30, 2019
  23. Matthew DeVoreMay 31, 2019
  24. Jeff HostetlerJun 3, 2019
  25. Matthew DeVoreJun 1, 2019
  26. Emily ShafferMay 28, 2019
  27. Matthew DeVoreMay 31, 2019
  28. Jeff KingMay 31, 2019
  29. Matthew DeVoreJun 1, 2019
  30. Jeff KingJun 3, 2019
  31. Matthew DeVoreJun 3, 2019
  32. Jeff KingJun 4, 2019
  33. Matthew DeVoreJun 4, 2019
  34. Jeff KingJun 4, 2019
  35. Matthew DeVoreJun 4, 2019
  36. Jeff KingJun 4, 2019
  37. Matthew DeVoreJun 4, 2019
  38. Jeff KingJun 9, 2019
  39. 4/5 list-objects-filter-options: move error check upMatthew DeVore, May 22, 2019
  40. 5/5 list-objects-filter-options: allow mult. --filterMatthew DeVore, May 22, 2019
  41. Matthew DeVoreJun 6, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.