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

[Patrick Steinhardt <ps@pks.im>]

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

Hi,

this is the fifth 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.

Changes compared to v4:

    - Split out the memory leak fix when parsing hidden refs into a
      separate commit 1/7.

    - Fixed calls to `string_list_clear()` to not free the `util` field
      of `hidden_refs`.

    - Updated the documentation of the new `--exclude-hidden=` option to
      hopefully be easier to understand.

    - We now return an error when `--exclude-hidden=` is used with
      either one of `--branches`, `--tags` or `--remotes`.

    - Fixed a bug where we didn't bail when `--exclude-hidden=` was
      passed multiple times when there are no hidden refs.

    - Extended test coverage.

Patrick

Patrick Steinhardt (7):
  refs: fix memory leak when parsing hideRefs config
  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                |  18 +++-
 connected.c                        |   3 +
 connected.h                        |   7 ++
 ls-refs.c                          |  13 ++-
 refs.c                             |  16 +--
 refs.h                             |   5 +-
 revision.c                         | 131 +++++++++++++++--------
 revision.h                         |  43 ++++++--
 t/t6018-rev-list-glob.sh           |  40 +++++++
 t/t6021-rev-list-exclude-hidden.sh | 163 +++++++++++++++++++++++++++++
 upload-pack.c                      |  30 +++---
 15 files changed, 411 insertions(+), 83 deletions(-)
 create mode 100755 t/t6021-rev-list-exclude-hidden.sh

Range-diff against v4:
-:  ---------- > 1:  cfab8ba1a2 refs: fix memory leak when parsing hideRefs config
1:  34afe30d60 ! 2:  d8118c6dd8 refs: get rid of global list of hidden refs
    @@ builtin/receive-pack.c: int cmd_receive_pack(int argc, const char **argv, const
      		packet_flush(1);
      	oid_array_clear(&shallow);
      	oid_array_clear(&ref);
    -+	string_list_clear(&hidden_refs, 1);
    ++	string_list_clear(&hidden_refs, 0);
      	free((void *)push_cert_nonce);
      	return 0;
      }
    @@ ls-refs.c: int ls_refs(struct repository *r, struct packet_reader *request)
      	packet_fflush(stdout);
      	strvec_clear(&data.prefixes);
      	strbuf_release(&data.buf);
    -+	string_list_clear(&data.hidden_refs, 1);
    ++	string_list_clear(&data.hidden_refs, 0);
      	return 0;
      }
      
    @@ 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_nodup(hide_refs, ref);
    + 		string_list_append_nodup(hide_refs, ref);
      	}
      	return 0;
      }
    @@ upload-pack.c: static void upload_pack_data_clear(struct upload_pack_data *data)
      {
      	string_list_clear(&data->symref, 1);
      	string_list_clear(&data->wanted_refs, 1);
    -+	string_list_clear(&data->hidden_refs, 1);
    ++	string_list_clear(&data->hidden_refs, 0);
      	object_array_clear(&data->want_obj);
      	object_array_clear(&data->have_obj);
      	oid_array_clear(&data->haves);
2:  b4f21d0a80 = 3:  93a627fb7f revision: move together exclusion-related functions
3:  265b292ed5 = 4:  ad41ade332 revision: introduce struct to handle exclusions
4:  c7fa6698db ! 5:  b5a4ce432a revision: add new parameter to exclude hidden refs
    @@ Documentation/rev-list-options.txt: respectively, and they must begin with `refs
      explicitly.
      
     +--exclude-hidden=[receive|uploadpack]::
    -+	Do not include refs that have been hidden via either one of
    -+	`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.
    ++	Do not include refs that would be hidden by `git-receive-pack` or
    ++	`git-upload-pack` by consulting the appropriate `receive.hideRefs` or
    ++	`uploadpack.hideRefs` configuration along with `transfer.hideRefs` (see
    ++	linkgit:git-config[1]). This option affects the next pseudo-ref option
    ++	`--all` or `--glob` and is cleared after processing them.
     +
      --reflog::
      	Pretend as if all objects mentioned by reflogs are listed on the
    @@ revision.c: void init_ref_exclusions(struct ref_exclusions *exclusions)
      {
      	string_list_clear(&exclusions->excluded_refs, 0);
     +	string_list_clear(&exclusions->hidden_refs, 0);
    ++	exclusions->hidden_refs_configured = 0;
      }
      
      void add_ref_exclusion(struct ref_exclusions *exclusions, const char *exclude)
    @@ revision.c: void add_ref_exclusion(struct ref_exclusions *exclusions, const char
     +static int hide_refs_config(const char *var, const char *value, void *cb_data)
     +{
     +	struct exclude_hidden_refs_cb *cb = cb_data;
    ++	cb->exclusions->hidden_refs_configured = 1;
     +	return parse_hide_refs_config(var, value, cb->section,
     +				      &cb->exclusions->hidden_refs);
     +}
    @@ revision.c: void add_ref_exclusion(struct ref_exclusions *exclusions, const char
     +	if (strcmp(section, "receive") && strcmp(section, "uploadpack"))
     +		die(_("unsupported section for hidden refs: %s"), section);
     +
    -+	if (exclusions->hidden_refs.nr)
    ++	if (exclusions->hidden_refs_configured)
     +		die(_("--exclude-hidden= passed more than once"));
     +
     +	cb.exclusions = exclusions;
    @@ revision.c: static int handle_revision_opt(struct rev_info *revs, int argc, cons
      	    starts_with(arg, "--branches=") || starts_with(arg, "--tags=") ||
      	    starts_with(arg, "--remotes=") || starts_with(arg, "--no-walk="))
      	{
    +@@ revision.c: static int handle_revision_pseudo_opt(struct rev_info *revs,
    + 		}
    + 		clear_ref_exclusions(&revs->ref_excludes);
    + 	} else if (!strcmp(arg, "--branches")) {
    ++		if (revs->ref_excludes.hidden_refs_configured)
    ++			return error(_("--exclude-hidden cannot be used together with --branches"));
    + 		handle_refs(refs, revs, *flags, refs_for_each_branch_ref);
    + 		clear_ref_exclusions(&revs->ref_excludes);
    + 	} else if (!strcmp(arg, "--bisect")) {
    +@@ revision.c: static int handle_revision_pseudo_opt(struct rev_info *revs,
    + 			    for_each_good_bisect_ref);
    + 		revs->bisect = 1;
    + 	} else if (!strcmp(arg, "--tags")) {
    ++		if (revs->ref_excludes.hidden_refs_configured)
    ++			return error(_("--exclude-hidden cannot be used together with --tags"));
    + 		handle_refs(refs, revs, *flags, refs_for_each_tag_ref);
    + 		clear_ref_exclusions(&revs->ref_excludes);
    + 	} else if (!strcmp(arg, "--remotes")) {
    ++		if (revs->ref_excludes.hidden_refs_configured)
    ++			return error(_("--exclude-hidden cannot be used together with --remotes"));
    + 		handle_refs(refs, revs, *flags, refs_for_each_remote_ref);
    + 		clear_ref_exclusions(&revs->ref_excludes);
    + 	} else if ((argcount = parse_long_opt("glob", argv, &optarg))) {
     @@ revision.c: static int handle_revision_pseudo_opt(struct rev_info *revs,
      	} else if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
      		add_ref_exclusion(&revs->ref_excludes, optarg);
    @@ revision.c: static int handle_revision_pseudo_opt(struct rev_info *revs,
     +		return argcount;
      	} else if (skip_prefix(arg, "--branches=", &optarg)) {
      		struct all_refs_cb cb;
    ++		if (revs->ref_excludes.hidden_refs_configured)
    ++			return error(_("--exclude-hidden cannot be used together with --branches"));
      		init_all_refs_cb(&cb, revs, *flags);
    + 		for_each_glob_ref_in(handle_one_ref, optarg, "refs/heads/", &cb);
    + 		clear_ref_exclusions(&revs->ref_excludes);
    + 	} else if (skip_prefix(arg, "--tags=", &optarg)) {
    + 		struct all_refs_cb cb;
    ++		if (revs->ref_excludes.hidden_refs_configured)
    ++			return error(_("--exclude-hidden cannot be used together with --tags"));
    + 		init_all_refs_cb(&cb, revs, *flags);
    + 		for_each_glob_ref_in(handle_one_ref, optarg, "refs/tags/", &cb);
    + 		clear_ref_exclusions(&revs->ref_excludes);
    + 	} else if (skip_prefix(arg, "--remotes=", &optarg)) {
    + 		struct all_refs_cb cb;
    ++		if (revs->ref_excludes.hidden_refs_configured)
    ++			return error(_("--exclude-hidden cannot be used together with --remotes"));
    + 		init_all_refs_cb(&cb, revs, *flags);
    + 		for_each_glob_ref_in(handle_one_ref, optarg, "refs/remotes/", &cb);
    + 		clear_ref_exclusions(&revs->ref_excludes);
     
      ## revision.h ##
     @@ revision.h: struct ref_exclusions {
    @@ revision.h: struct ref_exclusions {
     +	 * `ref_is_hidden()`.
     +	 */
     +	struct string_list hidden_refs;
    ++
    ++	/*
    ++	 * Indicates whether hidden refs have been configured. This is to
    ++	 * distinguish between no hidden refs existing and hidden refs not
    ++	 * being parsed.
    ++	 */
    ++	char hidden_refs_configured;
      };
      
      /**
    @@ t/t6021-rev-list-exclude-hidden.sh (new)
     +do
     +	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_must_fail git rev-list --exclude-hidden=$section --exclude-hidden=$section 2>err &&
     +		test_cmp expected err
     +	'
     +
    @@ t/t6021-rev-list-exclude-hidden.sh (new)
     +		test_cmp expected out
     +	'
     +
    ++	test_expect_success "$section: excluded hidden refs can be used with multiple pseudo-refs" '
    ++		git -c transfer.hideRefs=refs/ rev-list --exclude-hidden=$section --all --exclude-hidden=$section --all >out &&
    ++		test_must_be_empty out
    ++	'
    ++
    ++	test_expect_success "$section: works with --glob" '
    ++		git -c transfer.hideRefs=refs/hidden/ rev-list --exclude-hidden=$section --glob=refs/h* >out &&
    ++		cat >expected <<-EOF &&
    ++		$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 &&
    @@ t/t6021-rev-list-exclude-hidden.sh (new)
     +		EOF
     +		test_cmp expected out
     +	'
    ++
    ++	for pseudoopt in remotes branches tags
    ++	do
    ++		test_expect_success "$section: fails with --$pseudoopt" '
    ++			test_must_fail git rev-list --exclude-hidden=$section --$pseudoopt 2>err &&
    ++			test_i18ngrep "error: --exclude-hidden cannot be used together with --$pseudoopt" err
    ++		'
    ++
    ++		test_expect_success "$section: fails with --$pseudoopt=pattern" '
    ++			test_must_fail git rev-list --exclude-hidden=$section --$pseudoopt=pattern 2>err &&
    ++			test_i18ngrep "error: --exclude-hidden cannot be used together with --$pseudoopt" err
    ++		'
    ++	done
     +done
     +
     +test_done
5:  79c5c64a80 ! 6:  2eeb25eef0 rev-parse: add `--exclude-hidden=` option
    @@ Documentation/git-rev-parse.txt: respectively, and they must begin with `refs/`
      explicitly.
      
     +--exclude-hidden=[receive|uploadpack]::
    -+	Do not include refs that have been hidden via either one of
    -+	`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.
    ++	Do not include refs that would be hidden by `git-receive-pack` or
    ++	`git-upload-pack` by consulting the appropriate `receive.hideRefs` or
    ++	`uploadpack.hideRefs` configuration along with `transfer.hideRefs` (see
    ++	linkgit:git-config[1]). This option affects the next pseudo-ref option
    ++	`--all` or `--glob` and is cleared after processing them.
     +
      --disambiguate=<prefix>::
      	Show every object whose name begins with the given prefix.
      	The <prefix> must be at least 4 hexadecimal digits long to
     
      ## builtin/rev-parse.c ##
    +@@ builtin/rev-parse.c: int cmd_rev_parse(int argc, const char **argv, const char *prefix)
    + 				continue;
    + 			}
    + 			if (opt_with_value(arg, "--branches", &arg)) {
    ++				if (ref_excludes.hidden_refs_configured)
    ++					return error(_("--exclude-hidden cannot be used together with --branches"));
    + 				handle_ref_opt(arg, "refs/heads/");
    + 				continue;
    + 			}
    + 			if (opt_with_value(arg, "--tags", &arg)) {
    ++				if (ref_excludes.hidden_refs_configured)
    ++					return error(_("--exclude-hidden cannot be used together with --tags"));
    + 				handle_ref_opt(arg, "refs/tags/");
    + 				continue;
    + 			}
    +@@ builtin/rev-parse.c: int cmd_rev_parse(int argc, const char **argv, const char *prefix)
    + 				continue;
    + 			}
    + 			if (opt_with_value(arg, "--remotes", &arg)) {
    ++				if (ref_excludes.hidden_refs_configured)
    ++					return error(_("--exclude-hidden cannot be used together with --remotes"));
    + 				handle_ref_opt(arg, "refs/remotes/");
    + 				continue;
    + 			}
     @@ builtin/rev-parse.c: int cmd_rev_parse(int argc, const char **argv, const char *prefix)
      				add_ref_exclusion(&ref_excludes, arg);
      				continue;
    @@ t/t6018-rev-list-glob.sh: test_expect_success 'rev-parse --exclude=ref with --re
     +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"
    ++		compare "-c transfer.hideRefs=refs/remotes/ rev-parse" "--branches --tags" "--exclude-hidden=$section --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"
    ++		compare "-c transfer.hideRefs=refs/heads/subspace/ rev-parse" "--exclude=refs/heads/subspace/* --all" "--exclude-hidden=$section --all"
     +	'
    ++
    ++	test_expect_success "rev-parse --exclude-hidden=$section with --glob" '
    ++		compare "-c transfer.hideRefs=refs/heads/subspace/ rev-parse" "--exclude=refs/heads/subspace/* --glob=refs/heads/*" "--exclude-hidden=$section --glob=refs/heads/*"
    ++	'
    ++
    ++	test_expect_success "rev-parse --exclude-hidden=$section can be passed once per pseudo-ref" '
    ++		compare "-c transfer.hideRefs=refs/remotes/ rev-parse" "--branches --tags --branches --tags" "--exclude-hidden=$section --all --exclude-hidden=$section --all"
    ++	'
    ++
    ++	test_expect_success "rev-parse --exclude-hidden=$section can only be passed once per pseudo-ref" '
    ++		echo "fatal: --exclude-hidden= passed more than once" >expected &&
    ++		test_must_fail git rev-parse --exclude-hidden=$section --exclude-hidden=$section 2>err &&
    ++		test_cmp expected err
    ++	'
    ++
    ++	for pseudoopt in branches tags remotes
    ++	do
    ++		test_expect_success "rev-parse --exclude-hidden=$section fails with --$pseudoopt" '
    ++			echo "error: --exclude-hidden cannot be used together with --$pseudoopt" >expected &&
    ++			test_must_fail git rev-parse --exclude-hidden=$section --$pseudoopt 2>err &&
    ++			test_cmp expected err
    ++		'
    ++
    ++		test_expect_success "rev-parse --exclude-hidden=$section fails with --$pseudoopt=pattern" '
    ++			echo "error: --exclude-hidden cannot be used together with --$pseudoopt" >expected &&
    ++			test_must_fail git rev-parse --exclude-hidden=$section --$pseudoopt=pattern 2>err &&
    ++			test_cmp expected err
    ++		'
    ++	done
     +done
     +
      test_expect_success 'rev-list --exclude=glob with --branches=glob' '
6:  39b4741734 = 7:  f5f18f3939 receive-pack: only use visible refs for connectivity check
-- 
2.38.1


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