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