Re: [PATCH 0/2] negotiator: improve recent behavior + docs

On Thu, Sep 27 2018, Jonathan Tan wrote:

quoted

quoted

If you wanted to do this, it seems better to me to just declare a "null"
negotiation algorithm that does not perform any negotiation at all.

I think such an algorithm is a good idea in general, especially for
testing, and yeah, maybe that's the best way out of this, i.e. to do:

    if git rev-parse {}/HEAD 2>/dev/null
    then
        git fetch --negotiation-tip={}/HEAD {}
    else
        git -c fetch.negotiationAlgorithm=null fetch {}
    fi

Would such an algorithm be added by overriding default.c's add_tip
function to never add anything by calling default_negotiator_init()
followed by null_negotiator_init(), which would only override add_tip?
(yay C OO)

If so from fetch-pack.c it looks like there may be the limitation on the
interface that the negotiator can't exit early (in
fetch-pack.c:mark_tips). But I've just skimmed this, so maybe I've
missed something.

(I was reminded to reply to this offlist - sorry for the late reply.)

I think too many things need to be replaced (known_common, add_tip, and
ack all need to do nothing), so it's best to start from scratch. That
way, we also don't need to deal with the subtleties of C OO :-)

quoted

Also, looks like because of the current interface =null and
--negotiation-tip=* would (somewhat confusingly) do a "real" negotiation
if done that way, since it'll bypass the API and insert tips for it to
negotiate, but it looks like overriding next() will get around that.

If you do it as I suggest (in particular, add_tip doing nothing) then
there is the opposite problem that it won't be easy to inform the user
that --negotiation-tip does nothing in this case. Maybe there needs to
be an "accepts_tips" field in struct fetch_negotiator that, if false,
means that custom tips (or any tips) are not accepted, allowing the
caller of the negotiator to print a warning message in this case.

Thanks, yeah it seems the interface would need to be tweaked for such a
"null" negotiator.

Some more general questions (which I can turn into docs once I
understand this). If I run this, as a testcase for two random repos
where I "fetch" an unrelated one and use the first ever commit to
git.git as an alias for this "null" negotiatior, i.e. "just present this
one commit":

    (
        rm -rf /tmp/git &&
        git clone https://github.com/git/git.git /tmp/git &&
        cd /tmp/git &&
        git remote add gitlab-shell https://github.com/cr-marcstevens/sha1collisiondetection &&
        GIT_TRACE_PACKET=/tmp/git/packet.trace git fetch --negotiation-tip=$(git log --reverse|head -n 1|cut -d ' ' -f2) gitlab-shell &&
        grep -c "fetch-pack> have" /tmp/git/packet.trace
    )

I get:

    warning: Ignoring --negotiation-tip because the protocol does not support it.

And the grep -c shows we tried to present 55170 commits in "have" lines
to the server. Now, change that to SSH and all is well:

    (
        rm -rf /tmp/git &&
        git clone git@github.com:git/git.git /tmp/git &&
        cd /tmp/git &&
        git remote add gitlab-shell git@github.com:cr-marcstevens/sha1collisiondetection &&
        GIT_TRACE_PACKET=/tmp/git/packet.trace git fetch --negotiation-tip=$(git log --reverse|head -n 1|cut -d ' ' -f2) gitlab-shell &&
        grep -c "fetch-pack> have" /tmp/git/packet.trace
    )

I don't understand this limitation. With the SSH version we skip
straight to saying we "want" with just the 1 "have" line of
"e83c5163316f89bfbde7d9ab23ca2e25604af290".

Why aren't we doing the same over http? I don't get how protocol support
is needed, it's us who decide to send over the "have" lines. Some
variant of this does work over "skipping":

    (
        rm -rf /tmp/git &&
        git clone https://github.com/git/git.git /tmp/git &&
        cd /tmp/git &&
        git remote add gitlab-shell https://github.com/cr-marcstevens/sha1collisiondetection &&
        GIT_TRACE_PACKET=/tmp/git/packet.trace git -c fetch.negotiationAlgorithm=skipping fetch gitlab-shell &&
        grep -c "fetch-pack> have" /tmp/git/packet.trace
    )

There we send 14002 "have" lines, which seems expected, but then with
the same thing over SSH we don't send any:

    (
        rm -rf /tmp/git &&
        git clone git@github.com:git/git.git /tmp/git &&
        cd /tmp/git &&
        git remote add gitlab-shell git@github.com:cr-marcstevens/sha1collisiondetection &&
        GIT_TRACE_PACKET=/tmp/git/packet.trace git -c fetch.negotiationAlgorithm=skipping fetch gitlab-shell &&
        grep -c "fetch-pack> have" /tmp/git/packet.trace
    )

So that seems like another bug, and as an aside, a "skipping"
implementation that sends ~1/4 of the commits in the repo seems way less
aggressive than it should be. I was expecting something that would
gradually "ramp up" from the tips. Where say starting at master/next/pu
we present every 100th commit as a "have" until the 1000th commit, then
every 1000 commits until 10k and quickly after that step up the size
rapidly.