Re: [PATCH 0/8] ref-in-want

2018-06-19 Thread Jonathan Tan
> On 06/15, Jonathan Tan wrote:
> > 
> > Supporting patterns would mean that we would possibly be able to
> > eliminate the ls-refs step, thus saving at least a RTT. (Originally I
> > thought that supporting patterns would also allow us to tolerate refs
> > being removed during the fetch process, but I see that this is already
> > handled by the server ignoring "want-ref " wherein  doesn't
> > exist on the server.)
> 
> What's your opinion on this?  Should we keep it how it is in v2 of the
> series where the server ignores refs it doesn't know about or revert to
> what v1 of the series did and have it be a hard error?

I think it should be like in v2 - the server should ignore "want-ref
" lines for refs it doesn't know about. And, after more thought, I
think that the client should die if "fetch " was not
fulfilled, and ignore if a ref in "fetch " was not
fulfilled.

The advantage of doing that is that we make the protocol a bit more
tolerant to adverse conditions (e.g. a rapidly changing repository or an
eventually consistent load-balancing setup), while having little-to-no
effect on regular conditions.

The disadvantage is that there is now one additional place where a
failure can silently occur, but I think that this is a minor
disadvantage. A naive script using "git fetch", in my mind, would assume
that refs/heads/exact exists if "fetch
refs/heads/exact:refs/heads/exact" succeeds, but would not assume that
refs/heads/wildcard-something exists if "fetch
refs/heads/wildcard*:refs/heads/wildcard*" succeeds, which fits in
nicely with the die/ignore behavior I outlined above.


Re: [PATCH 0/8] ref-in-want

2018-06-19 Thread Brandon Williams
On 06/15, Jonathan Tan wrote:
> 
> Supporting patterns would mean that we would possibly be able to
> eliminate the ls-refs step, thus saving at least a RTT. (Originally I
> thought that supporting patterns would also allow us to tolerate refs
> being removed during the fetch process, but I see that this is already
> handled by the server ignoring "want-ref " wherein  doesn't
> exist on the server.)

What's your opinion on this?  Should we keep it how it is in v2 of the
series where the server ignores refs it doesn't know about or revert to
what v1 of the series did and have it be a hard error?

I've gone back and forth on what I think we should do so I'd like to
hear at least one more opinion :)

-- 
Brandon Williams


Re: [PATCH 0/8] ref-in-want

2018-06-19 Thread Jonathan Tan
[snip]

> > in which we have rarely-updated branches that we still want to fetch
> > (e.g. an annotated tag when we fetch refs/tags/* or a Gerrit
> > refs/changes/* branch), having the ref advertisement first means that we
> > can omit them from our "want" or "want-ref" list. But not having them
> > means that we send "want-ref refs/tags/*" to the server, and during
> > negotiation inform the server of our master branch (A), and since the
> > server knows of a common ancestor of all our wants (A, B, C), it will
> > terminate the negotiation and send the objects specific to branches B
> > and C even though it didn't need to.
> > 
> > So maybe we still need to keep the ls-refs step around, and thus, this
> > design of only accepting exact refs is perhaps good enough for now.
> 
> I think that taking a smaller step first it probably better.  This is
> something that we've done in the past with the shallow features and
> later capabilities were added to add different ways to request shallow
> fetches.

I think we're agreeing that the smaller step first is better.

> That being said, if we find that this feature doesn't work as-is and
> needs the extra complexity of patterns from the start then they should
> be added.

I agree (although I would be OK too if we decide to do the small
exact-name step now and then the pattern step later guarded by a
capability, as long as the project understood that multiple support
levels would then exist in the wild).

> But it doesn't seem like there's a concrete reason at the
> moment.

I agree. I thought I had a reason, but not after thinking through the
ideas I explained in [1].

[1] 
https://public-inbox.org/git/20180615190458.147775-1-jonathanta...@google.com/


Re: [PATCH 0/8] ref-in-want

2018-06-19 Thread Brandon Williams
On 06/15, Jonathan Tan wrote:
> (replying to the original since my e-mail is about design)
> 
> > This version of ref-in-want is a bit more restrictive than what Jonathan
> > originally proposed (only full ref names are allowed instead of globs
> > and OIDs), but it is meant to accomplish the same goal (solve the issues
> > of refs changing during negotiation).
> 
> One question remains: are we planning to expand this feature (e.g. to
> support patterns ending in *, or to support any pattern that can appear
> on the LHS of a refspec), and if yes, are we OK with having 2 or more
> versions of the service in the wild, each having different pattern
> support?
> 
> Supporting patterns would mean that we would possibly be able to
> eliminate the ls-refs step, thus saving at least a RTT. (Originally I
> thought that supporting patterns would also allow us to tolerate refs
> being removed during the fetch process, but I see that this is already
> handled by the server ignoring "want-ref " wherein  doesn't
> exist on the server.)
> 
> However, after some in-office discussion, I see that eliminating the
> ls-refs step means that we lose some optimizations that can only be done
> when we see that we already have a sought remote ref. For example, in a
> repo like this:
> 
>  A
>  |
>  O
>  |
>  O B C
>  |/ /
>  O O
>  |/
>  O
> 
> in which we have rarely-updated branches that we still want to fetch
> (e.g. an annotated tag when we fetch refs/tags/* or a Gerrit
> refs/changes/* branch), having the ref advertisement first means that we
> can omit them from our "want" or "want-ref" list. But not having them
> means that we send "want-ref refs/tags/*" to the server, and during
> negotiation inform the server of our master branch (A), and since the
> server knows of a common ancestor of all our wants (A, B, C), it will
> terminate the negotiation and send the objects specific to branches B
> and C even though it didn't need to.
> 
> So maybe we still need to keep the ls-refs step around, and thus, this
> design of only accepting exact refs is perhaps good enough for now.

I think that taking a smaller step first it probably better.  This is
something that we've done in the past with the shallow features and
later capabilities were added to add different ways to request shallow
fetches.

That being said, if we find that this feature doesn't work as-is and
needs the extra complexity of patterns from the start then they should
be added.  But it doesn't seem like there's a concrete reason at the
moment.

-- 
Brandon Williams


Re: [PATCH 0/8] ref-in-want

2018-06-15 Thread Jonathan Tan
(replying to the original since my e-mail is about design)

> This version of ref-in-want is a bit more restrictive than what Jonathan
> originally proposed (only full ref names are allowed instead of globs
> and OIDs), but it is meant to accomplish the same goal (solve the issues
> of refs changing during negotiation).

One question remains: are we planning to expand this feature (e.g. to
support patterns ending in *, or to support any pattern that can appear
on the LHS of a refspec), and if yes, are we OK with having 2 or more
versions of the service in the wild, each having different pattern
support?

Supporting patterns would mean that we would possibly be able to
eliminate the ls-refs step, thus saving at least a RTT. (Originally I
thought that supporting patterns would also allow us to tolerate refs
being removed during the fetch process, but I see that this is already
handled by the server ignoring "want-ref " wherein  doesn't
exist on the server.)

However, after some in-office discussion, I see that eliminating the
ls-refs step means that we lose some optimizations that can only be done
when we see that we already have a sought remote ref. For example, in a
repo like this:

 A
 |
 O
 |
 O B C
 |/ /
 O O
 |/
 O

in which we have rarely-updated branches that we still want to fetch
(e.g. an annotated tag when we fetch refs/tags/* or a Gerrit
refs/changes/* branch), having the ref advertisement first means that we
can omit them from our "want" or "want-ref" list. But not having them
means that we send "want-ref refs/tags/*" to the server, and during
negotiation inform the server of our master branch (A), and since the
server knows of a common ancestor of all our wants (A, B, C), it will
terminate the negotiation and send the objects specific to branches B
and C even though it didn't need to.

So maybe we still need to keep the ls-refs step around, and thus, this
design of only accepting exact refs is perhaps good enough for now.


[PATCH 0/8] ref-in-want

2018-06-05 Thread Brandon Williams
This series adds the ref-in-want feature which was originally proposed
by Jonathan Tan
(https://public-inbox.org/git/cover.1485381677.git.jonathanta...@google.com/).

Back when ref-in-want was first discussed it was decided that we should
first solve the issue of moving to a new wire format and find a way to
limit the ref-advertisement before moving forward with ref-in-want.  Now
that protocol version 2 is a reality, and that refs can be filtered on
the server side, we can revisit ref-in-want.

This version of ref-in-want is a bit more restrictive than what Jonathan
originally proposed (only full ref names are allowed instead of globs
and OIDs), but it is meant to accomplish the same goal (solve the issues
of refs changing during negotiation).

Brandon Williams (8):
  test-pkt-line: add unpack-sideband subcommand
  upload-pack: implement ref-in-want
  upload-pack: test negotiation with changing repository
  fetch: refactor the population of peer ref OIDs
  fetch: refactor fetch_refs into two functions
  fetch: refactor to make function args narrower
  fetch-pack: put shallow info in output parameter
  fetch-pack: implement ref-in-want

 Documentation/config.txt|   4 +
 Documentation/technical/protocol-v2.txt |  28 ++-
 builtin/clone.c |   4 +-
 builtin/fetch.c | 126 +++-
 fetch-object.c  |   2 +-
 fetch-pack.c|  52 +++--
 remote.c|   1 +
 remote.h|   1 +
 t/helper/test-pkt-line.c|  37 
 t/lib-httpd.sh  |   1 +
 t/lib-httpd/apache.conf |   8 +
 t/lib-httpd/one-time-sed.sh |  16 ++
 t/t5703-upload-pack-ref-in-want.sh  | 245 
 transport-helper.c  |   6 +-
 transport-internal.h|   9 +-
 transport.c |  34 +++-
 transport.h |   3 +-
 upload-pack.c   |  64 +++
 18 files changed, 564 insertions(+), 77 deletions(-)
 create mode 100644 t/lib-httpd/one-time-sed.sh
 create mode 100755 t/t5703-upload-pack-ref-in-want.sh

-- 
2.17.1.1185.g55be947832-goog