mbox series

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

Message ID cover.1668149149.git.ps@pks.im (mailing list archive)
Headers show
Series receive-pack: only use visible refs for connectivity check | expand

Message

Patrick Steinhardt Nov. 11, 2022, 6:49 a.m. UTC
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

Comments

Taylor Blau Nov. 11, 2022, 10:18 p.m. UTC | #1
On Fri, Nov 11, 2022 at 07:49:49AM +0100, Patrick Steinhardt wrote:
> 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

The new version is looking pretty good to my eyes, though I would like
to hear from Peff, too, since he caught a few things that I missed in
the previous rounds.

I have to say, the final patch is *really* nice ;-).

Thanks,
Taylor
Jeff King Nov. 15, 2022, 5:26 p.m. UTC | #2
On Fri, Nov 11, 2022 at 05:18:23PM -0500, Taylor Blau wrote:

> On Fri, Nov 11, 2022 at 07:49:49AM +0100, Patrick Steinhardt wrote:
> > 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
> 
> The new version is looking pretty good to my eyes, though I would like
> to hear from Peff, too, since he caught a few things that I missed in
> the previous rounds.

This looks good to me, too. There's a typo (s/seciton/section/) in the
commit message of patch 6, but definitely not worth a re-roll. :)

I admit I didn't think _too_ deeply about the interaction with
namespaces, and whether there are any corner cases. I was happy to see
the tests there, and I assume in writing them that Patrick matched how
receive-pack, etc, behave.

-Peff
Taylor Blau Nov. 16, 2022, 9:22 p.m. UTC | #3
On Tue, Nov 15, 2022 at 12:26:25PM -0500, Jeff King wrote:
> On Fri, Nov 11, 2022 at 05:18:23PM -0500, Taylor Blau wrote:
>
> > On Fri, Nov 11, 2022 at 07:49:49AM +0100, Patrick Steinhardt wrote:
> > > 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
> >
> > The new version is looking pretty good to my eyes, though I would like
> > to hear from Peff, too, since he caught a few things that I missed in
> > the previous rounds.
>
> This looks good to me, too. There's a typo (s/seciton/section/) in the
> commit message of patch 6, but definitely not worth a re-roll. :)

Hmm. It looks like this is broken in CI when the default initial branch
name is something other than "master":

    $ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main ./t6021-rev-list-exclude-hidden.sh -i --verbose-only=12 -x
    [...]
    expecting success of 6021.12 'receive: 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

    + git -c transfer.hideRefs=refs/ rev-list --exclude-hidden=receive --all --exclude-hidden=receive --all
    + test_must_be_empty out
    + test 1 -ne 1
    + test_path_is_file out
    + test 1 -ne 1
    + test -f out
    + test -s out
    + echo 'out' is not empty, it contains:
    'out' is not empty, it contains:
    + cat out
    d2e88f5a45c63e4ec7e1fd303542944487abe89a
    + return 1
    error: last command exited with $?=1
    not ok 12 - receive: 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
    #
    1..12

I haven't looked too deeply at what is going on here, but let's make
sure to resolve this before graduating the topic down (which I would
otherwise like to do in the next push-out, probably tomorrow or the next
day).

Thanks,
Taylor
Jeff King Nov. 16, 2022, 10:04 p.m. UTC | #4
On Wed, Nov 16, 2022 at 04:22:24PM -0500, Taylor Blau wrote:

> > This looks good to me, too. There's a typo (s/seciton/section/) in the
> > commit message of patch 6, but definitely not worth a re-roll. :)
> 
> Hmm. It looks like this is broken in CI when the default initial branch
> name is something other than "master":
> 
>     $ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main ./t6021-rev-list-exclude-hidden.sh -i --verbose-only=12 -x
>     [...]
>     expecting success of 6021.12 'receive: 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
> 
>     + git -c transfer.hideRefs=refs/ rev-list --exclude-hidden=receive --all --exclude-hidden=receive --all
>     + test_must_be_empty out
>     + test 1 -ne 1
>     + test_path_is_file out
>     + test 1 -ne 1
>     + test -f out
>     + test -s out
>     + echo 'out' is not empty, it contains:
>     'out' is not empty, it contains:
>     + cat out
>     d2e88f5a45c63e4ec7e1fd303542944487abe89a
>     + return 1
>     error: last command exited with $?=1
>     not ok 12 - receive: 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
>     #
>     1..12
> 
> I haven't looked too deeply at what is going on here, but let's make
> sure to resolve this before graduating the topic down (which I would
> otherwise like to do in the next push-out, probably tomorrow or the next
> day).

The issue is that some of the tests assume that hiding "refs/" should
produce no output from "--exclude-hidden=receive --all". But it will
also show HEAD, even if it points to a hidden ref (which I think is OK,
and matches what receive-pack would do).

But because the setup uses "main" as one of the sample refs, HEAD may or
may not be valid, depending on what it points to (without setting
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME it points to master, which is
unborn).

So the fix is just:

diff --git a/t/t6021-rev-list-exclude-hidden.sh b/t/t6021-rev-list-exclude-hidden.sh
index 018796d41c..1543a93fe0 100755
--- a/t/t6021-rev-list-exclude-hidden.sh
+++ b/t/t6021-rev-list-exclude-hidden.sh
@@ -5,8 +5,8 @@ test_description='git rev-list --exclude-hidden test'
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-	test_commit_bulk --id=commit --ref=refs/heads/main 1 &&
-	COMMIT=$(git rev-parse refs/heads/main) &&
+	test_commit_bulk --id=commit --ref=refs/heads/foo 1 &&
+	COMMIT=$(git rev-parse refs/heads/foo) &&
 	test_commit_bulk --id=tag --ref=refs/tags/lightweight 1 &&
 	TAG=$(git rev-parse refs/tags/lightweight) &&
 	test_commit_bulk --id=hidden --ref=refs/hidden/commit 1 &&

but Patrick may want to select a more meaningful name. :)

Notably I don't think we want to do the usual

  GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

at the top of the script. We really don't mean to depend on having a
specific branch that HEAD points to here.

-Peff
Taylor Blau Nov. 16, 2022, 10:33 p.m. UTC | #5
On Wed, Nov 16, 2022 at 05:04:10PM -0500, Jeff King wrote:
> > I haven't looked too deeply at what is going on here, but let's make
> > sure to resolve this before graduating the topic down (which I would
> > otherwise like to do in the next push-out, probably tomorrow or the next
> > day).
>
> The issue is that some of the tests assume that hiding "refs/" should
> produce no output from "--exclude-hidden=receive --all". But it will
> also show HEAD, even if it points to a hidden ref (which I think is OK,
> and matches what receive-pack would do).
>
> But because the setup uses "main" as one of the sample refs, HEAD may or
> may not be valid, depending on what it points to (without setting
> GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME it points to master, which is
> unborn).
>
> So the fix is just:
>
> [...]

Makes perfect sense, and thanks for looking into it.

Patrick: it sounds like there was one typo in the earlier round which
you may want to pick up also, assuming you reroll this.

Thanks,
Taylor
Patrick Steinhardt Nov. 17, 2022, 5:45 a.m. UTC | #6
On Wed, Nov 16, 2022 at 05:33:12PM -0500, Taylor Blau wrote:
> On Wed, Nov 16, 2022 at 05:04:10PM -0500, Jeff King wrote:
> > > I haven't looked too deeply at what is going on here, but let's make
> > > sure to resolve this before graduating the topic down (which I would
> > > otherwise like to do in the next push-out, probably tomorrow or the next
> > > day).
> >
> > The issue is that some of the tests assume that hiding "refs/" should
> > produce no output from "--exclude-hidden=receive --all". But it will
> > also show HEAD, even if it points to a hidden ref (which I think is OK,
> > and matches what receive-pack would do).
> >
> > But because the setup uses "main" as one of the sample refs, HEAD may or
> > may not be valid, depending on what it points to (without setting
> > GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME it points to master, which is
> > unborn).
> >
> > So the fix is just:
> >
> > [...]
> 
> Makes perfect sense, and thanks for looking into it.
> 
> Patrick: it sounds like there was one typo in the earlier round which
> you may want to pick up also, assuming you reroll this.
> 
> Thanks,
> Taylor

Thanks to both of you, I'll send out v6 in a bit.

Patrick