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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 1, 2018, 21:13 UTC
Message-ID: <87in4tu6wb.fsf@evledraar.gmail.com>
In-Reply-To: <20180801202545.204151-1-jonathantanmy@google.com>

On Wed, Aug 01 2018, Jonathan Tan wrote:

Show 12 quoted lines

>> I think 01/02 in this patch series implements something that's better
>> & more future-proof.
>
> Thanks. Both patches are:
> Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
>
> A small note:
>
>> -	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...) :)

Show 6 quoted lines

> 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.

Show 24 quoted lines

>> 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.
>
>> 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.