Re: [PATCH v2] builtin/fetch: skip unnecessary tasks when using --negotiate-only

From: Glen Choo <chooglen@google.com>
Date: Dec 21, 2021, 23:07 UTC
Message-ID: <kl6lczlpzhdn.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <kl6ltuf3ysnw.fsf@chooglen-macbookpro.roam.corp.google.com>

Glen Choo <chooglen@google.com> writes:

Show 10 quoted lines

>>>  	prepare_repo_settings(the_repository);
>>
>> This is existing code, but I wonder why it can be done _SO_ late in
>> the sequence.  We've already called the transport API for the
>> negotiate-only communication at this point, but a call to this
>> function is the only thing that gives fetch_negotiation_algorithm
>> member in the_repository its default value, isn't it?
>
> That's right, this looks like it could be a bug. Maybe Jonathan knows
> more.

It seems that fetch negotiation always calls prepare_repo_settings(). Fetch negotiation uses negotiate_using_fetch(), which calls fetch_negotiator_init(), which calls prepare_repo_settings():

  void fetch_negotiator_init(struct repository *r,
          struct fetch_negotiator *negotiator)
  {
    prepare_repo_settings(r);
    switch(r->settings.fetch_negotiation_algorithm) {
    case FETCH_NEGOTIATION_SKIPPING:
      skipping_negotiator_init(negotiator);
      return;

    case FETCH_NEGOTIATION_NOOP:
      noop_negotiator_init(negotiator);
      return;

    case FETCH_NEGOTIATION_DEFAULT:
      default_negotiator_init(negotiator);
      return;
    }
  }

If anything, this seems safer than calling prepare_repo_settings() in cmd_fetch() :)