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

Re: [PATCH v2 6/6] refs/reftable: wire up support for exclude patterns

From
Taylor Blau <me@ttaylorr.com>
Date
Sep 17, 2024, 09:26 UTC
Message-ID
<ZulLWJWLgU7gL1MK@nand.local>
In-Reply-To
<050f4906393d1f9c58a6b6bc695b004695d219be.1726476401.git.ps@pks.im>
On Mon, Sep 16, 2024 at 10:50:16AM +0200, Patrick Steinhardt wrote:
Show 16 quoted lines
> This makes t1419 work with the "reftable" backend with some slight
> modifications. Of course it also speeds up listing of references with
> hidden refs. The following benchmark prints one reference with 1 million
> hidden references:
>
>     Benchmark 1: HEAD~
>       Time (mean ± σ):      93.3 ms ±   2.1 ms    [User: 90.3 ms, System: 2.5 ms]
>       Range (min … max):    89.8 ms …  97.2 ms    33 runs
>
>     Benchmark 2: HEAD
>       Time (mean ± σ):       4.2 ms ±   0.6 ms    [User: 2.2 ms, System: 1.8 ms]
>       Range (min … max):     3.1 ms …   8.1 ms    765 runs
>
>     Summary
>       HEAD ran
>        22.15 ± 3.19 times faster than HEAD~
Very nice.
Show 18 quoted lines
> +		/*
> +		 * When the reference name is lexicographically bigger than the
> +		 * current exclude pattern we know that it won't ever match any
> +		 * of the following references, either. We thus advance to the
> +		 * next pattern and re-check whether it matches.
> +		 *
> +		 * Otherwise, if it's smaller, then we do not have a match and
> +		 * thus want to show the current reference.
> +		 */
> +		cmp = strncmp(iter->ref.refname, pattern,
> +			      iter->exclude_patterns_strlen);
> +		if (cmp > 0) {
> +			iter->exclude_patterns_index++;
> +			iter->exclude_patterns_strlen = 0;
> +			continue;
> +		}
> +		if (cmp < 0)
> +			return 0;

Perhaps I am showing my ignorance for the reftable backend, but is it OK to call strncmp() against all patterns here?

In the packed-refs implementation which I worked on with Peff sometime last year, we rejected exclude patterns that contain special glob characters in them for exactly this reason.

The implementation that I'm referring to has a helpful comment that jogged my memory for what we were thinking at the time (see the function refs/packed-backend.c::populate_excluded_jump_list()).

Ugh, I just read the next hunk below, so ignore me here ;-).
Show 35 quoted lines
> +static char **filter_exclude_patterns(const char **exclude_patterns)
> +{
> +	size_t filtered_size = 0, filtered_alloc = 0;
> +	char **filtered = NULL;
> +
> +	if (!exclude_patterns)
> +		return NULL;
> +
> +	for (size_t i = 0; ; i++) {
> +		const char *exclude_pattern = exclude_patterns[i];
> +		int has_glob = 0;
> +
> +		if (!exclude_pattern)
> +			break;
> +
> +		for (const char *p = exclude_pattern; *p; p++) {
> +			has_glob = is_glob_special(*p);
> +			if (has_glob)
> +				break;
> +		}
> +		if (has_glob)
> +			continue;
> +
> +		ALLOC_GROW(filtered, filtered_size + 1, filtered_alloc);
> +		filtered[filtered_size++] = xstrdup(exclude_pattern);
> +	}
> +
> +	if (filtered_size) {
> +		QSORT(filtered, filtered_size, qsort_strcmp);
> +		ALLOC_GROW(filtered, filtered_size + 1, filtered_alloc);
> +		filtered[filtered_size++] = NULL;
> +	}
> +
> +	return filtered;
> +}
Ohhh, here's where we filter out un-excludeable patterns. Ignore me :-).

One question I had reading this is why we don't filter these out on the fly in the iterator itself instead of allocating a separate array that we have to xstrdup() into and free later on.

We may be at the point of diminishing returns here, but I wonder if allocating this thing is more expensive than a few redundant strcmp()s and calls to is_glob_special(). I dunno.

Thanks, Taylor

Previous: Patrick SteinhardtNext: Patrick Steinhardt
Message 33 of 36 in “refs/reftable: wire up exclude patterns”
  1. 0/6 refs/reftable: wire up exclude patternsPatrick Steinhardt, Sep 9, 2024
  2. 1/6 refs: properly apply exclude patterns to namespaced refsPatrick Steinhardt, Sep 9, 2024
  3. karthik nayakSep 13, 2024
  4. Patrick SteinhardtSep 16, 2024
  5. 2/6 builtin/receive-pack: fix exclude patterns when announcing refsPatrick Steinhardt, Sep 9, 2024
  6. karthik nayakSep 13, 2024
  7. 3/6 Makefile: stop listing test library objects twicePatrick Steinhardt, Sep 9, 2024
  8. 4/6 t/unit-tests: introduce reftable libraryPatrick Steinhardt, Sep 9, 2024
  9. 5/6 reftable/reader: make table iterator reseekablePatrick Steinhardt, Sep 9, 2024
  10. karthik nayakSep 13, 2024
  11. Patrick SteinhardtSep 16, 2024
  12. karthik nayakSep 17, 2024
  13. 6/6 refs/reftable: wire up support for exclude patternsPatrick Steinhardt, Sep 9, 2024
  14. karthik nayakSep 13, 2024
  15. Patrick SteinhardtSep 16, 2024
  16. karthik nayakSep 17, 2024
  17. karthik nayakSep 13, 2024
  18. Patrick SteinhardtSep 16, 2024
  19. 0/6 refs/reftable: wire up exclude patternsPatrick Steinhardt, Sep 16, 2024
  20. 1/6 refs: properly apply exclude patterns to namespaced refsPatrick Steinhardt, Sep 16, 2024
  21. Taylor BlauSep 17, 2024
  22. Patrick SteinhardtSep 17, 2024
  23. Taylor BlauSep 17, 2024
  24. Patrick SteinhardtSep 17, 2024
  25. Taylor BlauSep 17, 2024
  26. Patrick SteinhardtSep 17, 2024
  27. 2/6 builtin/receive-pack: fix exclude patterns when announcing refsPatrick Steinhardt, Sep 16, 2024
  28. Taylor BlauSep 17, 2024
  29. 3/6 Makefile: stop listing test library objects twicePatrick Steinhardt, Sep 16, 2024
  30. 4/6 t/unit-tests: introduce reftable libraryPatrick Steinhardt, Sep 16, 2024
  31. 5/6 reftable/reader: make table iterator reseekablePatrick Steinhardt, Sep 16, 2024
  32. 6/6 refs/reftable: wire up support for exclude patternsPatrick Steinhardt, Sep 16, 2024
  33. Taylor BlauSep 17, 2024
  34. Patrick SteinhardtSep 17, 2024
  35. Taylor BlauSep 17, 2024
  36. karthik nayakSep 17, 2024

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.