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:
> > 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.
> >> -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.
> >> +/*
> >> + 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
