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 4, 2019, 23:49 UTC
Message-ID
<20190604234951.GB43275@comcast.net>
In-Reply-To
<20190604231418.GA12501@sigill.intra.peff.net>
On Tue, Jun 04, 2019 at 07:14:18PM -0400, Jeff King wrote:
Show 7 quoted lines
> Right, that has to be a real colon because it's syntactically
> significant (but a colon in the username _must_ be encoded). That strbuf
> function doesn't really understand whole URLs, and it's up to the caller
> to assemble the parts.
> 
> Anyway, we've veered off of your patch series enough. Yeah, it sounds
> like using strbuf's url-encoding is not quite what you want.

I tried to do it anyway :) I think this makes the strbuf API a bit easier to reason about, and strbuf.h is a bit more self-documenting. WDYT?

diff --git a/credential-store.c b/credential-store.c
index ac295420dd..c010497cb2 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -65,29 +65,30 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
 	parse_credential_file(fn, c, NULL, print_line);
 	if (commit_lock_file(&credential_lock) < 0)
 		die_errno("unable to write credential store");
 }
 
 static void store_credential_file(const char *fn, struct credential *c)
 {
 	struct strbuf buf = STRBUF_INIT;
 
 	strbuf_addf(&buf, "%s://", c->protocol);
-	strbuf_addstr_urlencode(&buf, c->username, 1);
+	strbuf_addstr_urlencode(&buf, c->username, is_rfc3986_unreserved);
 	strbuf_addch(&buf, ':');
-	strbuf_addstr_urlencode(&buf, c->password, 1);
+	strbuf_addstr_urlencode(&buf, c->password, is_rfc3986_unreserved);
 	strbuf_addch(&buf, '@');
 	if (c->host)
-		strbuf_addstr_urlencode(&buf, c->host, 1);
+		strbuf_addstr_urlencode(&buf, c->host, is_rfc3986_unreserved);
 	if (c->path) {
 		strbuf_addch(&buf, '/');
-		strbuf_addstr_urlencode(&buf, c->path, 0);
+		strbuf_addstr_urlencode(&buf, c->path,
+					is_rfc3986_reserved_or_unreserved);
 	}
 
 	rewrite_credential_file(fn, c, &buf);
 	strbuf_release(&buf);
 }
 
 static void store_credential(const struct string_list *fns, struct credential *c)
 {
 	struct string_list_item *fn;
 
diff --git a/http.c b/http.c
index 27aa0a3192..938b9e55af 100644
--- a/http.c
+++ b/http.c
@@ -506,23 +506,25 @@ static void var_override(const char **var, char *value)
 static void set_proxyauth_name_password(CURL *result)
 {
 #if LIBCURL_VERSION_NUM >= 0x071301
 		curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
 			proxy_auth.username);
 		curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
 			proxy_auth.password);
 #else
 		struct strbuf s = STRBUF_INIT;
 
-		strbuf_addstr_urlencode(&s, proxy_auth.username, 1);
+		strbuf_addstr_urlencode(&s, proxy_auth.username,
+					is_rfc3986_unreserved);
 		strbuf_addch(&s, ':');
-		strbuf_addstr_urlencode(&s, proxy_auth.password, 1);
+		strbuf_addstr_urlencode(&s, proxy_auth.password,
+					is_rfc3986_unreserved);
 		curl_proxyuserpwd = strbuf_detach(&s, NULL);
 		curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);
 #endif
 }
 
 static void init_curl_proxy_auth(CURL *result)
 {
 	if (proxy_auth.username) {
 		if (!proxy_auth.password)
 			credential_fill(&proxy_auth);
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 6b206dc58b..9a5677c2c8 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -167,30 +167,25 @@ static int parse_combine_filter(
 
 cleanup:
 	strbuf_list_free(subspecs);
 	if (result) {
 		list_objects_filter_release(filter_options);
 		memset(filter_options, 0, sizeof(*filter_options));
 	}
 	return result;
 }
 
-static void add_url_encoded(struct strbuf *dest, const char *s)
+static int allow_unencoded(char ch)
 {
-	while (*s) {
-		if (*s <= ' ' || strchr(RESERVED_NON_WS, *s) ||
-			*s == '%' || *s == '+')
-			strbuf_addf(dest, "%%%02X", (int)*s);
-		else
-			strbuf_addf(dest, "%c", *s);
-		s++;
-	}
+	if (ch <= ' ' || ch == '%' || ch == '+')
+		return 0;
+	return !strchr(RESERVED_NON_WS, ch);
 }
 
 /*
  * Changes filter_options into an equivalent LOFC_COMBINE filter options
  * instance. Does not do anything if filter_options is already LOFC_COMBINE.
  */
 static void transform_to_combine_type(
 	struct list_objects_filter_options *filter_options)
 {
 	assert(filter_options->choice);
@@ -202,22 +197,23 @@ static void transform_to_combine_type(
 			xcalloc(initial_sub_alloc, sizeof(*sub_array));
 		sub_array[0] = *filter_options;
 		memset(filter_options, 0, sizeof(*filter_options));
 		filter_options->sub = sub_array;
 		filter_options->sub_alloc = initial_sub_alloc;
 	}
 	filter_options->sub_nr = 1;
 	filter_options->choice = LOFC_COMBINE;
 	strbuf_init(&filter_options->filter_spec, 0);
 	strbuf_addstr(&filter_options->filter_spec, "combine:");
-	add_url_encoded(&filter_options->filter_spec,
-			filter_options->sub[0].filter_spec.buf);
+	strbuf_addstr_urlencode(&filter_options->filter_spec,
+				filter_options->sub[0].filter_spec.buf,
+				allow_unencoded);
 	/*
 	 * We don't need the filter_spec strings for subfilter specs, only the
 	 * top level.
 	 */
 	strbuf_release(&filter_options->sub[0].filter_spec);
 }
 
 void list_objects_filter_die_if_populated(
 	struct list_objects_filter_options *filter_options)
 {
@@ -239,21 +235,22 @@ void parse_list_objects_filter(
 		parse_error = gently_parse_list_objects_filter(
 			filter_options, arg, &errbuf);
 	} else {
 		/*
 		 * Make filter_options an LOFC_COMBINE spec so we can trivially
 		 * add subspecs to it.
 		 */
 		transform_to_combine_type(filter_options);
 
 		strbuf_addstr(&filter_options->filter_spec, "+");
-		add_url_encoded(&filter_options->filter_spec, arg);
+		strbuf_addstr_urlencode(&filter_options->filter_spec, arg,
+					allow_unencoded);
 		trace_printf("Generated composite filter-spec: %s\n",
 			     filter_options->filter_spec.buf);
 		ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,
 			      filter_options->sub_alloc);
 
 		parse_error = gently_parse_list_objects_filter(
 			&filter_options->sub[filter_options->sub_nr - 1], arg,
 			&errbuf);
 	}
 	if (parse_error)
diff --git a/strbuf.c b/strbuf.c
index 0e18b259ce..60ab5144f2 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -767,55 +767,56 @@ void strbuf_addstr_xml_quoted(struct strbuf *buf, const char *s)
 		case '&':
 			strbuf_addstr(buf, "&amp;");
 			break;
 		case 0:
 			return;
 		}
 		s++;
 	}
 }
 
-static int is_rfc3986_reserved(char ch)
+int is_rfc3986_reserved_or_unreserved(char ch)
 {
+	if (is_rfc3986_unreserved(ch))
+		return 1;
 	switch (ch) {
 		case '!': case '*': case '\'': case '(': case ')': case ';':
 		case ':': case '@': case '&': case '=': case '+': case '$':
 		case ',': case '/': case '?': case '#': case '[': case ']':
 			return 1;
 	}
 	return 0;
 }
 
-static int is_rfc3986_unreserved(char ch)
+int is_rfc3986_unreserved(char ch)
 {
 	return isalnum(ch) ||
 		ch == '-' || ch == '_' || ch == '.' || ch == '~';
 }
 
 static void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len,
-				 int reserved)
+				 char_predicate allow_unencoded_fn)
 {
 	strbuf_grow(sb, len);
 	while (len--) {
 		char ch = *s++;
-		if (is_rfc3986_unreserved(ch) ||
-		    (!reserved && is_rfc3986_reserved(ch)))
+		if (allow_unencoded_fn(ch))
 			strbuf_addch(sb, ch);
 		else
 			strbuf_addf(sb, "%%%02x", (unsigned char)ch);
 	}
 }
 
 void strbuf_addstr_urlencode(struct strbuf *sb, const char *s,
-			     int reserved)
+			     char_predicate allow_unencoded_fn)
 {
-	strbuf_add_urlencode(sb, s, strlen(s), reserved);
+	strbuf_add_urlencode(sb, s, strlen(s), allow_unencoded_fn);
 }
 
 void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes)
 {
 	if (bytes > 1 << 30) {
 		strbuf_addf(buf, "%u.%2.2u GiB",
 			    (unsigned)(bytes >> 30),
 			    (unsigned)(bytes & ((1 << 30) - 1)) / 10737419);
 	} else if (bytes > 1 << 20) {
 		unsigned x = bytes + 5243;  /* for rounding */
diff --git a/strbuf.h b/strbuf.h
index c8d98dfb95..346d722492 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -659,22 +659,27 @@ void strbuf_branchname(struct strbuf *sb, const char *name,
 		       unsigned allowed);
 
 /*
  * Like strbuf_branchname() above, but confirm that the result is
  * syntactically valid to be used as a local branch name in refs/heads/.
  *
  * The return value is "0" if the result is valid, and "-1" otherwise.
  */
 int strbuf_check_branch_ref(struct strbuf *sb, const char *name);
 
+typedef int (*char_predicate)(char ch);
+
+int is_rfc3986_unreserved(char ch);
+int is_rfc3986_reserved_or_unreserved(char ch);
+
 void strbuf_addstr_urlencode(struct strbuf *sb, const char *name,
-			     int reserved);
+			     char_predicate allow_unencoded_fn);
 
 __attribute__((format (printf,1,2)))
 int printf_ln(const char *fmt, ...);
 __attribute__((format (printf,2,3)))
 int fprintf_ln(FILE *fp, const char *fmt, ...);
 
 char *xstrdup_tolower(const char *);
 char *xstrdup_toupper(const char *);
 
 /**
Previous: Jeff KingNext: Jeff King
Message 37 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.