diff mbox series

[v2,1/3] refs: expose 'for_each_fullref_in_prefixes'

Message ID bda314fe7ae1629ba068a0c4ada9b6adc20576eb.1611158549.git.me@ttaylorr.com (mailing list archive)
State New, archived
Headers show
Series ls-refs: traverse prefixes of disjoint "ref-prefix" sets | expand

Commit Message

Taylor Blau Jan. 20, 2021, 4:04 p.m. UTC
This function was used in the ref-filter.c code to find the longest
common prefix of among a set of refspecs, and then to iterate all of the
references that descend from that prefix.

A future patch will want to use that same code from ls-refs.c, so
prepare by exposing and moving it to refs.c. Since there is nothing
specific to the ref-filter code here (other than that it was previously
the only caller of this function), this really belongs in the more
generic refs.h header.

The code moved in this patch is identical before and after, with the one
exception of renaming some arguments to be consistent with other
functions exposed in refs.h.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 ref-filter.c | 74 ++------------------------------------------
 refs.c       | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 refs.h       |  9 ++++++
 3 files changed, 98 insertions(+), 72 deletions(-)

Comments

Jeff King Jan. 20, 2021, 7:56 p.m. UTC | #1
On Wed, Jan 20, 2021 at 11:04:21AM -0500, Taylor Blau wrote:

> This function was used in the ref-filter.c code to find the longest
> common prefix of among a set of refspecs, and then to iterate all of the
> references that descend from that prefix.
> 
> A future patch will want to use that same code from ls-refs.c, so
> prepare by exposing and moving it to refs.c. Since there is nothing
> specific to the ref-filter code here (other than that it was previously
> the only caller of this function), this really belongs in the more
> generic refs.h header.
> 
> The code moved in this patch is identical before and after, with the one
> exception of renaming some arguments to be consistent with other
> functions exposed in refs.h.

The other non-identical thing is that it handles a namespace parameter
(which is good, but an obvious non-movement).

-Peff
Taylor Blau Jan. 20, 2021, 8:12 p.m. UTC | #2
On Wed, Jan 20, 2021 at 02:56:17PM -0500, Jeff King wrote:
> On Wed, Jan 20, 2021 at 11:04:21AM -0500, Taylor Blau wrote:
> > The code moved in this patch is identical before and after, with the one
> > exception of renaming some arguments to be consistent with other
> > functions exposed in refs.h.
>
> The other non-identical thing is that it handles a namespace parameter
> (which is good, but an obvious non-movement).

Ack; I knew that I forgot to mention something. Thanks for pointing it
out here. (Since you seem to be OK with this patch even without this
mention, I'll avoid sending another revision).

> -Peff

Thanks,
Taylor
Junio C Hamano Jan. 23, 2021, 2:59 a.m. UTC | #3
Taylor Blau <me@ttaylorr.com> writes:

> This function was used in the ref-filter.c code to find the longest
> common prefix of among a set of refspecs, and then to iterate all of the
> references that descend from that prefix.
>
> A future patch will want to use that same code from ls-refs.c, so
> prepare by exposing and moving it to refs.c. Since there is nothing
> specific to the ref-filter code here (other than that it was previously
> the only caller of this function), this really belongs in the more
> generic refs.h header.
>
> The code moved in this patch is identical before and after, with the one
> exception of renaming some arguments to be consistent with other
> functions exposed in refs.h.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  ref-filter.c | 74 ++------------------------------------------
>  refs.c       | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  refs.h       |  9 ++++++
>  3 files changed, 98 insertions(+), 72 deletions(-)

It is amusing that even to a change that is supposedly "expose
existing functionality by moving code around" and nothing else,
we can introduce new glitches.

> diff --git a/refs.c b/refs.c
> index 13dc2c3291..0b5a68588f 100644
> --- a/refs.c
> +++ b/refs.c
> ...
> +	for_each_string_list_item(prefix, &prefixes) {
> +		strbuf_addf(&buf, "%s", prefix->string);

		strbuf_addstr(&buf, prefix->string);

Caught by

https://github.com/git/git/runs/1752536671?check_suite_focus=true#step:4:63

I'll apply the fix suggested by Coccinelle on my end, so there is no
need to send an updated version just for this one.

Thanks.
Taylor Blau Jan. 25, 2021, 1:35 a.m. UTC | #4
On Fri, Jan 22, 2021 at 06:59:30PM -0800, Junio C Hamano wrote:
> Caught by
>
> https://github.com/git/git/runs/1752536671?check_suite_focus=true#step:4:63
>
> I'll apply the fix suggested by Coccinelle on my end, so there is no
> need to send an updated version just for this one.

Oof. How embarrassing. I'm well aware of the existence of
strbuf_addstr() -- there's even a caller just below the line I changed!
-- but clearly wasn't thinking when I wrote this patch.

Thanks for cleaning it up.

Thanks,
Taylor
diff mbox series

Patch

diff --git a/ref-filter.c b/ref-filter.c
index aa260bfd09..f918f00151 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1929,64 +1929,6 @@  static int filter_pattern_match(struct ref_filter *filter, const char *refname)
 	return match_pattern(filter, refname);
 }
 
-static int qsort_strcmp(const void *va, const void *vb)
-{
-	const char *a = *(const char **)va;
-	const char *b = *(const char **)vb;
-
-	return strcmp(a, b);
-}
-
-static void find_longest_prefixes_1(struct string_list *out,
-				  struct strbuf *prefix,
-				  const char **patterns, size_t nr)
-{
-	size_t i;
-
-	for (i = 0; i < nr; i++) {
-		char c = patterns[i][prefix->len];
-		if (!c || is_glob_special(c)) {
-			string_list_append(out, prefix->buf);
-			return;
-		}
-	}
-
-	i = 0;
-	while (i < nr) {
-		size_t end;
-
-		/*
-		* Set "end" to the index of the element _after_ the last one
-		* in our group.
-		*/
-		for (end = i + 1; end < nr; end++) {
-			if (patterns[i][prefix->len] != patterns[end][prefix->len])
-				break;
-		}
-
-		strbuf_addch(prefix, patterns[i][prefix->len]);
-		find_longest_prefixes_1(out, prefix, patterns + i, end - i);
-		strbuf_setlen(prefix, prefix->len - 1);
-
-		i = end;
-	}
-}
-
-static void find_longest_prefixes(struct string_list *out,
-				  const char **patterns)
-{
-	struct strvec sorted = STRVEC_INIT;
-	struct strbuf prefix = STRBUF_INIT;
-
-	strvec_pushv(&sorted, patterns);
-	QSORT(sorted.v, sorted.nr, qsort_strcmp);
-
-	find_longest_prefixes_1(out, &prefix, sorted.v, sorted.nr);
-
-	strvec_clear(&sorted);
-	strbuf_release(&prefix);
-}
-
 /*
  * This is the same as for_each_fullref_in(), but it tries to iterate
  * only over the patterns we'll care about. Note that it _doesn't_ do a full
@@ -1997,10 +1939,6 @@  static int for_each_fullref_in_pattern(struct ref_filter *filter,
 				       void *cb_data,
 				       int broken)
 {
-	struct string_list prefixes = STRING_LIST_INIT_DUP;
-	struct string_list_item *prefix;
-	int ret;
-
 	if (!filter->match_as_path) {
 		/*
 		 * in this case, the patterns are applied after
@@ -2024,16 +1962,8 @@  static int for_each_fullref_in_pattern(struct ref_filter *filter,
 		return for_each_fullref_in("", cb, cb_data, broken);
 	}
 
-	find_longest_prefixes(&prefixes, filter->name_patterns);
-
-	for_each_string_list_item(prefix, &prefixes) {
-		ret = for_each_fullref_in(prefix->string, cb, cb_data, broken);
-		if (ret)
-			break;
-	}
-
-	string_list_clear(&prefixes, 0);
-	return ret;
+	return for_each_fullref_in_prefixes(NULL, filter->name_patterns,
+					    cb, cb_data, broken);
 }
 
 /*
diff --git a/refs.c b/refs.c
index 13dc2c3291..0b5a68588f 100644
--- a/refs.c
+++ b/refs.c
@@ -1546,6 +1546,93 @@  int for_each_rawref(each_ref_fn fn, void *cb_data)
 	return refs_for_each_rawref(get_main_ref_store(the_repository), fn, cb_data);
 }
 
+static int qsort_strcmp(const void *va, const void *vb)
+{
+	const char *a = *(const char **)va;
+	const char *b = *(const char **)vb;
+
+	return strcmp(a, b);
+}
+
+static void find_longest_prefixes_1(struct string_list *out,
+				  struct strbuf *prefix,
+				  const char **patterns, size_t nr)
+{
+	size_t i;
+
+	for (i = 0; i < nr; i++) {
+		char c = patterns[i][prefix->len];
+		if (!c || is_glob_special(c)) {
+			string_list_append(out, prefix->buf);
+			return;
+		}
+	}
+
+	i = 0;
+	while (i < nr) {
+		size_t end;
+
+		/*
+		* Set "end" to the index of the element _after_ the last one
+		* in our group.
+		*/
+		for (end = i + 1; end < nr; end++) {
+			if (patterns[i][prefix->len] != patterns[end][prefix->len])
+				break;
+		}
+
+		strbuf_addch(prefix, patterns[i][prefix->len]);
+		find_longest_prefixes_1(out, prefix, patterns + i, end - i);
+		strbuf_setlen(prefix, prefix->len - 1);
+
+		i = end;
+	}
+}
+
+static void find_longest_prefixes(struct string_list *out,
+				  const char **patterns)
+{
+	struct strvec sorted = STRVEC_INIT;
+	struct strbuf prefix = STRBUF_INIT;
+
+	strvec_pushv(&sorted, patterns);
+	QSORT(sorted.v, sorted.nr, qsort_strcmp);
+
+	find_longest_prefixes_1(out, &prefix, sorted.v, sorted.nr);
+
+	strvec_clear(&sorted);
+	strbuf_release(&prefix);
+}
+
+int for_each_fullref_in_prefixes(const char *namespace,
+				 const char **patterns,
+				 each_ref_fn fn, void *cb_data,
+				 unsigned int broken)
+{
+	struct string_list prefixes = STRING_LIST_INIT_DUP;
+	struct string_list_item *prefix;
+	struct strbuf buf = STRBUF_INIT;
+	int ret = 0, namespace_len;
+
+	find_longest_prefixes(&prefixes, patterns);
+
+	if (namespace)
+		strbuf_addstr(&buf, namespace);
+	namespace_len = buf.len;
+
+	for_each_string_list_item(prefix, &prefixes) {
+		strbuf_addf(&buf, "%s", prefix->string);
+		ret = for_each_fullref_in(buf.buf, fn, cb_data, broken);
+		if (ret)
+			break;
+		strbuf_setlen(&buf, namespace_len);
+	}
+
+	string_list_clear(&prefixes, 0);
+	strbuf_release(&buf);
+	return ret;
+}
+
 static int refs_read_special_head(struct ref_store *ref_store,
 				  const char *refname, struct object_id *oid,
 				  struct strbuf *referent, unsigned int *type)
diff --git a/refs.h b/refs.h
index ff05d2e9fe..c5fd35487d 100644
--- a/refs.h
+++ b/refs.h
@@ -347,6 +347,15 @@  int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,
 int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,
 			unsigned int broken);
 
+/**
+ * iterate all refs in "patterns" by partitioning patterns into disjoint sets
+ * and iterating the longest-common prefix of each set.
+ *
+ * callers should be prepared to ignore references that they did not ask for.
+ */
+int for_each_fullref_in_prefixes(const char *namespace, const char **patterns,
+				 each_ref_fn fn, void *cb_data,
+				 unsigned int broken);
 /**
  * iterate refs from the respective area.
  */