> >
> > if (from_stdin) {
> > if (args.stateless_rpc) {
> > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> > index 0916f76302..5ce32e531a 100755
> > --- a/t/t5516-fetch-push.sh
> > +++ b/t/t5516-fetch-push.sh
> > @@ -1768,4 +1768,53 @@ test_expect_success 'denyCurrentBranch and worktrees' '
> > test_must_fail git -C cloned push --delete origin new-wt
> > '
> >
> > +. "$TEST_DIRECTORY"/lib-httpd.sh
> > +start_httpd
>
> Ah, so fetch-push test wasn't doing any HTTP testing whatsoever. Does
> that mean there is a better place for these to go? Or does it mean that
> fetch-push test was under-testing?
These are closely related to the non-HTTP push negotiation tests that I
previously added to this file, so I don't think there is a better place.
As for whether this test is under-testing, I also don't think so - most
fetch/push processes would be the same regardless of protocol (but not
push negotiation, apparently).
>
> > +
> > +test_expect_success 'http push with negotiation' '
> > + SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" &&
> > + URI="$HTTPD_URL/smart/server" &&
> > +
> > + rm -rf client &&
> > + git init client &&
> > + test_commit -C client first_commit &&
> > + test_commit -C client second_commit &&
> > +
> > + # Without negotiation
> > + test_create_repo "$SERVER" &&
> > + test_config -C "$SERVER" http.receivepack true &&
> > + git -C client push "$URI" first_commit:refs/remotes/origin/first_commit &&
> Pushing a branch with just the first commit...
> > + git -C "$SERVER" config receive.hideRefs refs/remotes/origin/first_commit &&
> transfer.hideRefs (referenced by receive.hideRefs) says this ref will be
> omitted from advertisement, so we are forcing either an inefficient push
> or a negotiation to occur, by having the server initially claim not to
> know about it.
Currently, the client attempts the negotiation before the push ref
advertisement, so the ref advertisement does not influence whether the
negotiation happens or not.
(One might ask, should the negotiation happen after the push ref
advertisement then? This is an interesting idea, because the push ref
advertisement could have information that shows us that negotiation is
unnecessary, but I think that eventually it's best if we suppress the
push ref advertisement.)
> But it's only omitted from the *initial* advertisement,
> so it will be advertised in later rounds of negotiation, right?
There is no ref advertisement in any round of negotiation (there is no
ls-refs call). It will be acknowledged if the client queries for it,
though. (In any case, this config is not meant to affect the negotiation
part at all, because the negotiation part is using the "fetch" protocol
and the "receive" in "receive.hideRefs" means that it only affects the
"push" protocol.)
> > + GIT_TRACE2_EVENT="$(pwd)/event" git -C client -c protocol.version=2 \
> > + push "$URI" refs/heads/main:refs/remotes/origin/main &&
> And then from 'main' we push first_commit and second_commit?
Depending on the result of negotiation. Here, we push both commits (as
you can see from the next line you quoted below).
> > + grep_wrote 6 event && # 2 commits, 2 trees, 2 blobs
> Nice, I like the comment - this helps.
Thanks.
> > +
> > + # Same commands, but with negotiation
> > + rm event &&
> > + rm -rf "$SERVER" &&
> Ok, clean up the trace and the server so we can start over, but we don't
> need to recreate the client commits because the server doesn't know
> about them anyway. Fine.
> > + test_create_repo "$SERVER" &&
> > + test_config -C "$SERVER" http.receivepack true &&
> > + git -C client push "$URI" first_commit:refs/remotes/origin/first_commit &&
> > + git -C "$SERVER" config receive.hideRefs refs/remotes/origin/first_commit &&
> > + GIT_TRACE2_EVENT="$(pwd)/event" git -C client -c protocol.version=2 -c push.negotiate=1 \
> > + push "$URI" refs/heads/main:refs/remotes/origin/main &&
> And then here's the same set of commands with push negotiation, ok.
> > + grep_wrote 3 event # 1 commit, 1 tree, 1 blob
>
> Is there any reason the event counts would change or be
> non-deterministic outside of negotiation? Or, in other words, is this
> potentially flaky?
In general, no - the information that the server conveys to the client
in the ref advertisement (which commits it has) is quite well-defined,
so it can be precisely computed which objects the server has and hasn't.
One potential point of flakiness is if we change the client to push
extra objects (say, if the client decided to store one packfile per
commit and all its trees and blobs, and then push the entire packfile
whenever it needs to push that commit), but I don't think that we're
heading in that direction.
>
> > +'
> > +
> > +test_expect_success 'http push with negotiation proceeds anyway even if negotiation fails' '
> > + rm event &&
> > + rm -rf "$SERVER" &&
> > + test_create_repo "$SERVER" &&
> > + test_config -C "$SERVER" http.receivepack true &&
> > + git -C client push "$URI" first_commit:refs/remotes/origin/first_commit &&
> Hmm, this relies on 'client' being in the same state the above test left
> it. Probably better to recreate it or at least leave a loud warning
> about it in a comment above this test definition...
I ended up recreating it.
>
> > + git -C "$SERVER" config receive.hideRefs refs/remotes/origin/first_commit &&
> > + GIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT="$(pwd)/event" git -C client -c push.negotiate=1 \
> > + push "$URI" refs/heads/main:refs/remotes/origin/main 2>err &&
>
> And we're pushing with protocol v0 so no negotiation can occur here,
> right?
Yes, but this is worth adding a comment for, so I added it.
>
> > + grep_wrote 6 event && # 2 commits, 2 trees, 2 blobs
> > + test_i18ngrep "push negotiation failed" err
> > +'
> > +
> > +# DO NOT add non-httpd-specific tests here, because the last part of this
> > +# test script is only executed when httpd is available and enabled.
> > +
> > test_done
> > --
> > 2.32.0.288.g62a8d224e6-goog
> >
>
> Thanks for answering novice questions :)
Thanks for taking a look.
