[PATCH v2 0/8] Refactor fetch negotiation into its own API

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jun 6, 2018, 20:47 UTC
Message-ID: <cover.1528317619.git.jonathantanmy@google.com>
In-Reply-To: <cover.1527894919.git.jonathantanmy@google.com>

Thanks, Jonathan Nieder, for your comments.

This patch set now includes a patch at the beginning to split up everything_local(), making it clear which functions have side effects and which don't, and a patch at the end to reformat some comments.

While I was implementing the suggestions, there were some that I were unsure about. Here they are:

Show 6 quoted lines

> > nit: this adds the new test as last in the script.  Is there some
> > logical earlier place in the file it can go instead?  That way, the
> > file stays organized and concurrent patches that modify the same test
> > script are less likely to conflict.
> 
> Good point. I'll find a place.

I couldn't find a logical place (I didn't see any existing tests that test negotiation). I did move it up a bit earlier in the file, before filtering by blob size, because that seemed more distinct from the rest of the tests.

Show 11 quoted lines

> >> -static struct prio_queue rev_list = { compare_commits_by_commit_date };
> >> -static int non_common_revs, multi_ack, use_sideband;
> >> +struct data {
> >> +     struct prio_queue rev_list;
> >> +     int non_common_revs;
> >> +};
> >
> > How does this struct get used?  What does it represent?  A comment
> > might help.
> 
> I'll add a comment.

I thought of adding a comment, but felt that I would end up removing it anyway upon its move to negotiator/default.c, so I didn't end up adding it.

Show 10 quoted lines

> >> +/*
> >> +   This function marks a rev and its ancestors as common.
> >> +   In some cases, it is desirable to mark only the ancestors (for example
> >> +   when only the server does not yet know that they are common).
> >> +*/
> >
> > Not about this change: comments should have ' *' at the start of each
> > line (could do in a preparatory patch or a followup).
> 
> I'll add a followup.

I'm now not sure of the value of making a change just to update formatting, but I added the followup commit anyway - it can be easily dropped if we decide to do so.

Jonathan Tan (8):
  fetch-pack: split up everything_local()
  fetch-pack: clear marks before re-marking
  fetch-pack: directly end negotiation if ACK ready
  fetch-pack: use ref adv. to prune "have" sent
  fetch-pack: make negotiation-related vars local
  fetch-pack: move common check and marking together
  fetch-pack: introduce negotiator API
  negotiator/default: use better style in comments

 Makefile              |   2 +
 fetch-negotiator.c    |   8 ++
 fetch-negotiator.h    |  57 ++++++++++
 fetch-pack.c          | 254 ++++++++++++++----------------------------
 negotiator/default.c  | 174 +++++++++++++++++++++++++++++
 negotiator/default.h  |   8 ++
 object.h              |   3 +-
 t/t5500-fetch-pack.sh |  39 +++++++
 8 files changed, 376 insertions(+), 169 deletions(-)
 create mode 100644 fetch-negotiator.c
 create mode 100644 fetch-negotiator.h
 create mode 100644 negotiator/default.c
 create mode 100644 negotiator/default.h

-- 
2.17.0.768.g1526ddbba1.dirty