Re: [PATCH] receive-pack: purge temporary data if no command is ready to run

[Lertz Chen <bojun.cbj@gmail.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2022年1月24日周一 23:29写道：
>
>
> On Mon, Jan 24 2022, BoJun via GitGitGadget wrote:
>
> > From: Chen Bojun <bojun.cbj@alibaba-inc.com>
> >
> > When pushing a hidden ref, e.g.:
> >
> >     $ git push origin HEAD:refs/hidden/foo
> >
> > "receive-pack" will reject our request with an error message like this:
> >
> >     ! [remote rejected] HEAD -> refs/hidden/foo (deny updating a hidden ref)
> >
> > The remote side ("git-receive-pack") will not create the hidden ref as
> > expected, but the pack file sent by "git-send-pack" is left inside the
> > remote repository. I.e. the quarantine directory is not purged as it
> > should be.
>
> Hrm, shouldn't the tmp-objdir.[ch]'s atexit() make sure that won't
> happen (but maybe it's buggy/not acting as I thought...)?
>

Although the command is marked with an error, tmp_objdir_migrate() is still
executed In the scenario of pushing a hidden branch, which leads to the
quarantine data to be released to .git/objects/.

> > Add a checkpoint before calling "tmp_objdir_migrate()" and after calling
> > the "pre-receive" hook to purge that temporary data in the quarantine
> > area when there is no command ready to run.
>
> But we're not purging anything, just returning early?
>
> If we'll always refuse this update, why do we need to run the
> pre-receive hook at all, isn't that another bug?....
>

unpack_with_sideband() receive the pack file pushed by the client and save it
in the created temporary quarantine area. Returning before tmp_objdir_migrate()
executes ensures that the quarantine data is cleaned up by programs registered
with atexit().

> > The reason we do not add the checkpoint before the "pre-receive" hook,
> > but after it, is that the "pre-receive" hook is called with a switch-off
> > "skip_broken" flag, and all commands, even broken ones, should be fed
> > by calling "feed_receive_hook()".
>
> ...but I see it's intentional, but does this make sense per the
> rationale of 160b81ed819 (receive-pack: don't pass non-existent refs to
> post-{receive,update} hooks, 2011-09-28)? Maybe, but the reason we have
> these for "non-existent refs" != this categorical denial of a hidden
> ref.
>

Commit 160b81ed819 (receive-pack: don't pass non-existent refs to
post-{receive,update}
hooks, 2011-09-28) executes the pre-receive hook when deleting a
non-existent branch
instead of executing the post-{receive,update} hooks. I think the
purpose of this is to gain
the opportunity to perceive the push content through pre-receive hook.
If we return directly
before pre-receive hook, are we going to lose this possibility?

> > Add a new test case and fix some formatting issues in t5516 as well.
> >
> > Helped-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> > Helped-by: Teng Long <dyroneteng@gmail.com>
> > Signed-off-by: Chen Bojun <bojun.cbj@alibaba-inc.com>
> > ---
> >     receive-pack: purge temporary data if no command is ready to run
>
> [...odd duplication of mostly the same commit message from GGG
> (presumably...]
>
> > -mk_empty () {
> > +mk_empty() {
>
> This patch includes a lot of line-re-wrapping, shell formatting changes
> etc. You should really submit this without any of those & just have the
> meaningful changes here.
>

Sorry, it was indeed a formatting issue, I'll roll back this part.

> > [...]
> > -for head in HEAD @
> > -do
> > +for head in HEAD @; do
>
> e.g. this, indentation changes earlier, and most of the changes here...
>
> >
> >       test_expect_success "push with $head" '
> >               mk_test testrepo heads/main &&
> > @@ -1020,7 +1011,7 @@ test_expect_success 'push into aliased refs (inconsistent)' '
> >       )
> >  '
> >
> > -test_force_push_tag () {
> > +test_force_push_tag() {
> >       tag_type_description=$1
> >       tag_args=$2
> >
> > @@ -1066,7 +1057,7 @@ test_force_push_tag () {
> >  test_force_push_tag "lightweight tag" "-f"
> >  test_force_push_tag "annotated tag" "-f -a -m'tag message'"
> >
> > -test_force_fetch_tag () {
> > +test_force_fetch_tag() {
> >       tag_type_description=$1
> >       tag_args=$2
> >
> > @@ -1158,8 +1149,7 @@ test_expect_success 'push --prune refspec' '
> >       ! check_push_result testrepo $the_first_commit tmp/foo tmp/bar
> >  '
> >
> > -for configsection in transfer receive
> > -do
> > +for configsection in transfer receive; do
> >       test_expect_success "push to update a ref hidden by $configsection.hiderefs" '
> >               mk_test testrepo heads/main hidden/one hidden/two hidden/three &&
> >               (
> > @@ -1250,8 +1240,7 @@ test_expect_success 'fetch exact SHA1 in protocol v2' '
> >       git -C child fetch -v ../testrepo $the_commit:refs/heads/copy
> >  '
> >
> > -for configallowtipsha1inwant in true false
> > -do
> > +for configallowtipsha1inwant in true false; do
> >       test_expect_success "shallow fetch reachable SHA1 (but not a ref), allowtipsha1inwant=$configallowtipsha1inwant" '
> >               mk_empty testrepo &&
> >               (
> > @@ -1809,4 +1798,12 @@ test_expect_success 'refuse fetch to current branch of bare repository worktree'
> >       git -C bare.git fetch -u .. HEAD:wt
> >  '
> >
> > +test_expect_success 'refuse to push a hidden ref, and make sure do not pollute the repository' '
> > +     mk_empty testrepo &&
> > +     git -C testrepo config receive.hiderefs refs/hidden &&
> > +     git -C testrepo config receive.unpackLimit 1 &&
> > +     test_must_fail git push testrepo HEAD:refs/hidden/foo &&
> > +     test_dir_is_empty testrepo/.git/objects/pack
> > +'
> > +
> >  test_done
> >
> > base-commit: 297ca895a27a6bbdb7906371d533f72a12ad25b2
>
>
> ...until we get to this, this mostly OK, but maybe test the case for
> what the hook does here (depending on what we want to do).
>
> If the quarantine directory was not purged as before how does checking
> whether testrepo/.git/objects/pack is empty help? We place those in
> .git/objects/tmp_objdir-* don't we?

If we split the patch into two parts and put the test case before the patch
of receive-pack.c. Then in this test case, we will find that although the
user pushes hidden references will fail, the object files contained in these
references will still exist in the .git/objects/pack directory. A patch of
receive-pack.c fixes this use case. The reason not splitting into two
commits is to protect the changes I made in receive-pack.c.