[PATCH v4 0/6] receive-pack: only use visible refs for connectivity check

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 19538 bytes --]

Hi,

this is the fourth version of my patch series that tries to improve
performance of the connectivity check by only considering preexisting
refs as uninteresting that could actually have been advertised to the
client.

This time around there are only incremental changes compared to v3, the
overall implementation stays the same. Changes:

    - Fixed a pre-existing memory leak in how hidden refs are parsed so
      that tests now pass with TEST_PASSES_SANITIZE_LEAK=true.

    - Improved initialization of `struct ref_exclusions` to reuse the
      `REF_EXCLUSIONS_INIT` macro so we don't have to repeat the logic.

    - Dropped the `--exclude-hidden=transfer` option. Only `receive` and
      `uploadpack` are supported now.

Patrick

Patrick Steinhardt (6):
  refs: get rid of global list of hidden refs
  revision: move together exclusion-related functions
  revision: introduce struct to handle exclusions
  revision: add new parameter to exclude hidden refs
  rev-parse: add `--exclude-hidden=` option
  receive-pack: only use visible refs for connectivity check

 Documentation/git-rev-parse.txt    |   7 ++
 Documentation/rev-list-options.txt |   7 ++
 builtin/receive-pack.c             |  10 ++-
 builtin/rev-list.c                 |   1 +
 builtin/rev-parse.c                |  12 ++-
 connected.c                        |   3 +
 connected.h                        |   7 ++
 ls-refs.c                          |  13 ++-
 refs.c                             |  16 ++--
 refs.h                             |   5 +-
 revision.c                         | 117 +++++++++++++++---------
 revision.h                         |  36 ++++++--
 t/t6018-rev-list-glob.sh           |  11 +++
 t/t6021-rev-list-exclude-hidden.sh | 137 +++++++++++++++++++++++++++++
 upload-pack.c                      |  30 ++++---
 15 files changed, 329 insertions(+), 83 deletions(-)
 create mode 100755 t/t6021-rev-list-exclude-hidden.sh

Range-diff against v3:
1:  3741f0a389 ! 1:  34afe30d60 refs: get rid of global list of hidden refs
    @@ refs.c: int parse_hide_refs_config(const char *var, const char *value, const cha
     -			CALLOC_ARRAY(hide_refs, 1);
     -			hide_refs->strdup_strings = 1;
     -		}
    - 		string_list_append(hide_refs, ref);
    +-		string_list_append(hide_refs, ref);
    ++		string_list_append_nodup(hide_refs, ref);
      	}
      	return 0;
      }
2:  a6dcc99ca9 = 2:  b4f21d0a80 revision: move together exclusion-related functions
3:  2a6a67df1d ! 3:  265b292ed5 revision: introduce struct to handle exclusions
    @@ Commit message
         of exclusion.
     
         Introduce a new `struct ref_exclusions` that encapsulates all the logic
    -    related to excluding refs.
    +    related to excluding refs and move the `struct string_list` that holds
    +    all wildmatch patterns of excluded refs into it. Rename functions that
    +    operate on this struct to match its name.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ revision.c: static void add_rev_cmdline_list(struct rev_info *revs,
     -		free(*ref_excludes_p);
     -	}
     -	*ref_excludes_p = NULL;
    -+	string_list_init_dup(&exclusions->excluded_refs);
    ++	struct ref_exclusions blank = REF_EXCLUSIONS_INIT;
    ++	memcpy(exclusions, &blank, sizeof(*exclusions));
      }
      
     -void add_ref_exclusion(struct string_list **ref_excludes_p, const char *exclude)
    @@ revision.h: struct rev_cmdline_info {
     +	 */
     +	struct string_list excluded_refs;
     +};
    ++
    ++/**
    ++ * Initialize a `struct ref_exclusions` with a macro.
    ++ */
    ++#define REF_EXCLUSIONS_INIT { \
    ++	.excluded_refs = STRING_LIST_INIT_DUP, \
    ++}
     +
      struct oidset;
      struct topo_walk_info;
    @@ revision.h: void mark_trees_uninteresting_sparse(struct repository *r, struct oi
     -int ref_excluded(struct string_list *, const char *path);
     -void clear_ref_exclusion(struct string_list **);
     -void add_ref_exclusion(struct string_list **, const char *exclude);
    -+#define REF_EXCLUSIONS_INIT { .excluded_refs = STRING_LIST_INIT_DUP }
    -+
     +int ref_excluded(const struct ref_exclusions *exclusions, const char *path);
     +void init_ref_exclusions(struct ref_exclusions *);
     +void clear_ref_exclusions(struct ref_exclusions *);
4:  de7c1aa210 ! 4:  c7fa6698db revision: add new parameter to exclude hidden refs
    @@ Documentation/rev-list-options.txt: respectively, and they must begin with `refs
      or `--all`. If a trailing '/{asterisk}' is intended, it must be given
      explicitly.
      
    -+--exclude-hidden=[transfer|receive|uploadpack]::
    ++--exclude-hidden=[receive|uploadpack]::
     +	Do not include refs that have been hidden via either one of
    -+	`transfer.hideRefs`, `receive.hideRefs` or `uploadpack.hideRefs` that
    -+	the next `--all`, `--branches`, `--tags`, `--remotes` or `--glob` would
    -+	otherwise consider.  This option is cleared when seeing one of these
    -+	pseudo-refs.
    ++	`receive.hideRefs` or `uploadpack.hideRefs` (see linkgit:git-config[1])
    ++	that the next `--all`, `--branches`, `--tags`, `--remotes` or `--glob`
    ++	would otherwise consider. This option is cleared when seeing one of
    ++	these pseudo-refs.
     +
      --reflog::
      	Pretend as if all objects mentioned by reflogs are listed on the
    @@ builtin/rev-list.c: static const char rev_list_usage[] =
      "    --tags\n"
      "    --remotes\n"
      "    --stdin\n"
    -+"    --exclude-hidden=[transfer|receive|uploadpack]\n"
    ++"    --exclude-hidden=[receive|uploadpack]\n"
      "    --quiet\n"
      "  ordering output:\n"
      "    --topo-order\n"
    @@ revision.c: static void add_rev_cmdline_list(struct rev_info *revs,
      	return 0;
      }
      
    - void init_ref_exclusions(struct ref_exclusions *exclusions)
    - {
    - 	string_list_init_dup(&exclusions->excluded_refs);
    -+	string_list_init_dup(&exclusions->hidden_refs);
    - }
    - 
    +@@ revision.c: void init_ref_exclusions(struct ref_exclusions *exclusions)
      void clear_ref_exclusions(struct ref_exclusions *exclusions)
      {
      	string_list_clear(&exclusions->excluded_refs, 0);
    -+	string_list_clear(&exclusions->hidden_refs, 1);
    ++	string_list_clear(&exclusions->hidden_refs, 0);
      }
      
      void add_ref_exclusion(struct ref_exclusions *exclusions, const char *exclude)
    @@ revision.c: void add_ref_exclusion(struct ref_exclusions *exclusions, const char
     +{
     +	struct exclude_hidden_refs_cb cb;
     +
    -+	if (strcmp(section, "transfer") && strcmp(section, "receive") &&
    -+	    strcmp(section, "uploadpack"))
    ++	if (strcmp(section, "receive") && strcmp(section, "uploadpack"))
     +		die(_("unsupported section for hidden refs: %s"), section);
     +
     +	if (exclusions->hidden_refs.nr)
    @@ revision.h: struct ref_exclusions {
     +	struct string_list hidden_refs;
      };
      
    + /**
    +@@ revision.h: struct ref_exclusions {
    +  */
    + #define REF_EXCLUSIONS_INIT { \
    + 	.excluded_refs = STRING_LIST_INIT_DUP, \
    ++	.hidden_refs = STRING_LIST_INIT_DUP, \
    + }
    + 
      struct oidset;
     @@ revision.h: void show_object_with_name(FILE *, struct object *, const char *);
      /**
       * Helpers to check if a reference should be excluded.
       */
    --#define REF_EXCLUSIONS_INIT { .excluded_refs = STRING_LIST_INIT_DUP }
    -+#define REF_EXCLUSIONS_INIT { .excluded_refs = STRING_LIST_INIT_DUP, .hidden_refs = STRING_LIST_INIT_DUP }
    - 
    ++
      int ref_excluded(const struct ref_exclusions *exclusions, const char *path);
      void init_ref_exclusions(struct ref_exclusions *);
      void clear_ref_exclusions(struct ref_exclusions *);
    @@ t/t6021-rev-list-exclude-hidden.sh (new)
     +
     +test_description='git rev-list --exclude-hidden test'
     +
    -+TEST_PASSES_SANITIZE_LEAK=true
     +. ./test-lib.sh
     +
     +test_expect_success 'setup' '
    @@ t/t6021-rev-list-exclude-hidden.sh (new)
     +	test_cmp expected err
     +'
     +
    -+test_expect_success 'passed multiple times' '
    -+	echo "fatal: --exclude-hidden= passed more than once" >expected &&
    -+	test_must_fail git -c transfer.hideRefs=refs/hidden/ rev-list --exclude-hidden=transfer --exclude-hidden=transfer 2>err &&
    -+	test_cmp expected err
    -+'
    -+
    -+test_expect_success '--exclude-hidden without hiddenRefs' '
    -+	git rev-list --exclude-hidden=transfer --all >out &&
    -+	cat >expected <<-EOF &&
    -+	$NAMESPACE
    -+	$HIDDEN
    -+	$TAG
    -+	$COMMIT
    -+	EOF
    -+	test_cmp expected out
    -+'
    -+
    -+test_expect_success 'hidden via transfer.hideRefs' '
    -+	git -c transfer.hideRefs=refs/hidden/ rev-list --exclude-hidden=transfer --all >out &&
    -+	cat >expected <<-EOF &&
    -+	$NAMESPACE
    -+	$TAG
    -+	$COMMIT
    -+	EOF
    -+	test_cmp expected out
    -+'
    -+
    -+test_expect_success '--all --exclude-hidden=transfer --not --all without hidden refs' '
    -+	git rev-list --all --exclude-hidden=transfer --not --all >out &&
    -+	test_must_be_empty out
    -+'
    -+
    -+test_expect_success '--all --exclude-hidden=transfer --not --all with hidden ref' '
    -+	git -c transfer.hideRefs=refs/hidden/ rev-list --all --exclude-hidden=transfer --not --all >out &&
    -+	cat >expected <<-EOF &&
    -+	$HIDDEN
    -+	EOF
    -+	test_cmp expected out
    -+'
    -+
    -+test_expect_success '--exclude-hidden with --exclude' '
    -+	git -c transfer.hideRefs=refs/hidden/ rev-list --exclude=refs/tags/* --exclude-hidden=transfer --all >out &&
    -+	cat >expected <<-EOF &&
    -+	$NAMESPACE
    -+	$COMMIT
    -+	EOF
    -+	test_cmp expected out
    -+'
    -+
    -+test_expect_success '--exclude-hidden is reset' '
    -+	git -c transfer.hideRefs=refs/ rev-list --exclude-hidden=transfer --all --all >out &&
    -+	cat >expected <<-EOF &&
    -+	$NAMESPACE
    -+	$HIDDEN
    -+	$TAG
    -+	$COMMIT
    -+	EOF
    -+	test_cmp expected out
    -+'
    -+
    -+test_expect_success '--exclude-hidden operates on stripped refs by default' '
    -+	GIT_NAMESPACE=namespace git -c transfer.hideRefs=refs/namespaced/ rev-list --exclude-hidden=transfer --all >out &&
    -+	cat >expected <<-EOF &&
    -+	$HIDDEN
    -+	$TAG
    -+	$COMMIT
    -+	EOF
    -+	test_cmp expected out
    -+'
    -+
    -+test_expect_success '--exclude-hidden does not hide namespace by default' '
    -+	GIT_NAMESPACE=namespace git -c transfer.hideRefs=refs/namespaces/namespace/ rev-list --exclude-hidden=transfer --all >out &&
    -+	cat >expected <<-EOF &&
    -+	$NAMESPACE
    -+	$HIDDEN
    -+	$TAG
    -+	$COMMIT
    -+	EOF
    -+	test_cmp expected out
    -+'
    -+
    -+test_expect_success '--exclude-hidden= may operate on unstripped refs' '
    -+	GIT_NAMESPACE=namespace git -c transfer.hideRefs=^refs/namespaces/namespace/ rev-list --exclude-hidden=transfer --all >out &&
    -+	cat >expected <<-EOF &&
    -+	$HIDDEN
    -+	$TAG
    -+	$COMMIT
    -+	EOF
    -+	test_cmp expected out
    -+'
    -+
     +for section in receive uploadpack
     +do
    -+	test_expect_success "hidden via $section.hideRefs" '
    -+		git -c $section.hideRefs=refs/hidden/ rev-list --exclude-hidden=$section --all >out &&
    -+		cat >expected <<-EOF &&
    -+		$NAMESPACE
    -+		$TAG
    -+		$COMMIT
    -+		EOF
    -+		test_cmp expected out
    ++	test_expect_success "$section: passed multiple times" '
    ++		echo "fatal: --exclude-hidden= passed more than once" >expected &&
    ++		test_must_fail git -c transfer.hideRefs=refs/hidden/ rev-list --exclude-hidden=$section --exclude-hidden=$section 2>err &&
    ++		test_cmp expected err
     +	'
     +
    -+	test_expect_success "--exclude-hidden=$section respects transfer.hideRefs" '
    -+		git -c transfer.hideRefs=refs/hidden/ rev-list --exclude-hidden=$section --all >out &&
    -+		cat >expected <<-EOF &&
    -+		$NAMESPACE
    -+		$TAG
    -+		$COMMIT
    -+		EOF
    -+		test_cmp expected out
    -+	'
    -+
    -+	test_expect_success "--exclude-hidden=transfer ignores $section.hideRefs" '
    -+		git -c $section.hideRefs=refs/hidden/ rev-list --exclude-hidden=transfer --all >out &&
    ++	test_expect_success "$section: without hiddenRefs" '
    ++		git rev-list --exclude-hidden=$section --all >out &&
     +		cat >expected <<-EOF &&
     +		$NAMESPACE
     +		$HIDDEN
    @@ t/t6021-rev-list-exclude-hidden.sh (new)
     +		test_cmp expected out
     +	'
     +
    -+	test_expect_success "--exclude-hidden=$section respects both transfer.hideRefs and $section.hideRefs" '
    ++	test_expect_success "$section: hidden via transfer.hideRefs" '
    ++		git -c transfer.hideRefs=refs/hidden/ rev-list --exclude-hidden=$section --all >out &&
    ++		cat >expected <<-EOF &&
    ++		$NAMESPACE
    ++		$TAG
    ++		$COMMIT
    ++		EOF
    ++		test_cmp expected out
    ++	'
    ++
    ++	test_expect_success "$section: hidden via $section.hideRefs" '
    ++		git -c $section.hideRefs=refs/hidden/ rev-list --exclude-hidden=$section --all >out &&
    ++		cat >expected <<-EOF &&
    ++		$NAMESPACE
    ++		$TAG
    ++		$COMMIT
    ++		EOF
    ++		test_cmp expected out
    ++	'
    ++
    ++	test_expect_success "$section: respects both transfer.hideRefs and $section.hideRefs" '
     +		git -c transfer.hideRefs=refs/tags/ -c $section.hideRefs=refs/hidden/ rev-list --exclude-hidden=$section --all >out &&
     +		cat >expected <<-EOF &&
     +		$NAMESPACE
    @@ t/t6021-rev-list-exclude-hidden.sh (new)
     +		EOF
     +		test_cmp expected out
     +	'
    ++
    ++	test_expect_success "$section: negation without hidden refs marks everything as uninteresting" '
    ++		git rev-list --all --exclude-hidden=$section --not --all >out &&
    ++		test_must_be_empty out
    ++	'
    ++
    ++	test_expect_success "$section: negation with hidden refs marks them as interesting" '
    ++		git -c transfer.hideRefs=refs/hidden/ rev-list --all --exclude-hidden=$section --not --all >out &&
    ++		cat >expected <<-EOF &&
    ++		$HIDDEN
    ++		EOF
    ++		test_cmp expected out
    ++	'
    ++
    ++	test_expect_success "$section: hidden refs and excludes work together" '
    ++		git -c transfer.hideRefs=refs/hidden/ rev-list --exclude=refs/tags/* --exclude-hidden=$section --all >out &&
    ++		cat >expected <<-EOF &&
    ++		$NAMESPACE
    ++		$COMMIT
    ++		EOF
    ++		test_cmp expected out
    ++	'
    ++
    ++	test_expect_success "$section: excluded hidden refs get reset" '
    ++		git -c transfer.hideRefs=refs/ rev-list --exclude-hidden=$section --all --all >out &&
    ++		cat >expected <<-EOF &&
    ++		$NAMESPACE
    ++		$HIDDEN
    ++		$TAG
    ++		$COMMIT
    ++		EOF
    ++		test_cmp expected out
    ++	'
    ++
    ++	test_expect_success "$section: operates on stripped refs by default" '
    ++		GIT_NAMESPACE=namespace git -c transfer.hideRefs=refs/namespaced/ rev-list --exclude-hidden=$section --all >out &&
    ++		cat >expected <<-EOF &&
    ++		$HIDDEN
    ++		$TAG
    ++		$COMMIT
    ++		EOF
    ++		test_cmp expected out
    ++	'
    ++
    ++	test_expect_success "$section: does not hide namespace by default" '
    ++		GIT_NAMESPACE=namespace git -c transfer.hideRefs=refs/namespaces/namespace/ rev-list --exclude-hidden=$section --all >out &&
    ++		cat >expected <<-EOF &&
    ++		$NAMESPACE
    ++		$HIDDEN
    ++		$TAG
    ++		$COMMIT
    ++		EOF
    ++		test_cmp expected out
    ++	'
    ++
    ++	test_expect_success "$section: can operate on unstripped refs" '
    ++		GIT_NAMESPACE=namespace git -c transfer.hideRefs=^refs/namespaces/namespace/ rev-list --exclude-hidden=$section --all >out &&
    ++		cat >expected <<-EOF &&
    ++		$HIDDEN
    ++		$TAG
    ++		$COMMIT
    ++		EOF
    ++		test_cmp expected out
    ++	'
     +done
     +
     +test_done
5:  68a5e56304 ! 5:  79c5c64a80 revparse: add `--exclude-hidden=` option
    @@ Metadata
     Author: Patrick Steinhardt <ps@pks.im>
     
      ## Commit message ##
    -    revparse: add `--exclude-hidden=` option
    +    rev-parse: add `--exclude-hidden=` option
     
         Add a new `--exclude-hidden=` option that is similar to the one we just
    -    added to git-rev-list(1). Given a seciton name `transfer`, `uploadpack`
    -    or `receive` as argument, it causes us to exclude all references that
    -    would be hidden by the respective `$seciton.hideRefs` configuration.
    +    added to git-rev-list(1). Given a seciton name `uploadpack` or `receive`
    +    as argument, it causes us to exclude all references that would be hidden
    +    by the respective `$section.hideRefs` configuration.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ Documentation/git-rev-parse.txt: respectively, and they must begin with `refs/`
      or `--all`. If a trailing '/{asterisk}' is intended, it must be given
      explicitly.
      
    -+--exclude-hidden=[transfer|receive|uploadpack]::
    ++--exclude-hidden=[receive|uploadpack]::
     +	Do not include refs that have been hidden via either one of
    -+	`transfer.hideRefs`, `receive.hideRefs` or `uploadpack.hideRefs` that
    -+	the next `--all`, `--branches`, `--tags`, `--remotes` or `--glob` would
    -+	otherwise consider.  This option is cleared when seeing one of these
    -+	pseudo-refs.
    ++	`receive.hideRefs` or `uploadpack.hideRefs` (see linkgit:git-config[1])
    ++	that the next `--all`, `--branches`, `--tags`, `--remotes` or `--glob`
    ++	would otherwise consider. This option is cleared when seeing one of
    ++	these pseudo-refs.
     +
      --disambiguate=<prefix>::
      	Show every object whose name begins with the given prefix.
    @@ t/t6018-rev-list-glob.sh: test_expect_success 'rev-parse --exclude=ref with --re
      	compare rev-parse "--exclude=upstream/x --remotes=upstream/*" "upstream/one upstream/two"
      '
      
    -+test_expect_success 'rev-parse --exclude-hidden= with --all' '
    -+	compare "-c transfer.hideRefs=refs/remotes/ rev-parse" "--exclude-hidden=transfer --all" "--branches --tags"
    -+'
    ++for section in receive uploadpack
    ++do
    ++	test_expect_success "rev-parse --exclude-hidden=$section with --all" '
    ++		compare "-c transfer.hideRefs=refs/remotes/ rev-parse" "--exclude-hidden=$section --all" "--branches --tags"
    ++	'
     +
    -+test_expect_success 'rev-parse --exclude-hidden= with --all' '
    -+	compare "-c transfer.hideRefs=refs/heads/subspace/ rev-parse" "--exclude-hidden=transfer --all" "--exclude=refs/heads/subspace/* --all"
    -+'
    ++	test_expect_success "rev-parse --exclude-hidden=$section with --all" '
    ++		compare "-c transfer.hideRefs=refs/heads/subspace/ rev-parse" "--exclude-hidden=$section --all" "--exclude=refs/heads/subspace/* --all"
    ++	'
    ++done
     +
      test_expect_success 'rev-list --exclude=glob with --branches=glob' '
      	compare rev-list "--exclude=subspace-* --branches=sub*" "subspace/one subspace/two"
6:  9d15449559 = 6:  39b4741734 receive-pack: only use visible refs for connectivity check
-- 
2.38.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]