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

On Wed, Aug 01 2018, Jonathan Tan wrote:

quoted

I think 01/02 in this patch series implements something that's better
& more future-proof.

Thanks. Both patches are:
Reviewed-by: Jonathan Tan <redacted>

A small note:

quoted

-	packfile; any other value instructs Git to use the default algorithm
+	packfile; The default is "default" which instructs Git to use the default algorithm

I think we generally don't capitalize words after semicolons.

Yeah I think that's right. Will fix (or if there's no other comments
perhaps Junio will munge it...) :)

Thanks for noticing that the check of fetch.negotiationAlgorithm only
happens when a negotiation actually occurs - before your patches, it
didn't really matter because we tolerated anything, but now we do. I
think this is fine - as far as I know, Git commands generally only read
the configs relevant to them, and if fetch.negotiationAlgorithm is not
relevant in a certain situation, we don't need to read it.

Yeah I think that's OK.

quoted

That's awesome. This is exactly what I wanted, this patch series also
fixes another small issue in 02/02; which is that the docs for the two
really should cross-link to make these discoverable from one another.

That's a good idea; thanks for doing it.

quoted

I.e. the way I'm doing this is I add all the remotes first, then I
fetch them all in parallel, but because the first time around I don't
have anything for that remote (and they don't share any commits) I
need to fake it up and pretend to be fetching from a repo that has
just one commit.

It would be better if I could somehow say that I don't mind that the
ref doesn't exist, but currently you either error out with this, or
ignore the glob, depending on the mode.

So I want this, but can't think of a less shitty UI than:

    git fetch --negotiation-tip=$REF --negotiation-tip-error-handling=missing-ref-means-no-want

Or something equally atrocious, do you have any better ideas?

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.

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.