git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Brandon Williams <bmwill@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 2/8] upload-pack: implement ref-in-want
Date: Thu, 07 Jun 2018 00:42:36 +0200	[thread overview]
Message-ID: <87d0x3zgdf.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20180606213213.GA154134@google.com>


On Wed, Jun 06 2018, Brandon Williams wrote:

> On 06/05, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Tue, Jun 05 2018, Brandon Williams wrote:
>>
>> > +uploadpack.allowRefInWant::
>> > +	If this option is set, `upload-pack` will support the `ref-in-want`
>> > +	feature of the protocol version 2 `fetch` command.
>> > +
>>
>> I think it makes sense to elaborate a bit on what this is for. Having
>> read this series through, and to make sure I understood this, maybe
>> something like this:
>>
>>    This feature is intended for the benefit of load-balanced servers
>>    which may not have the same view of what SHA-1s their refs point to,
>>    but are guaranteed to never advertise a reference that another server
>>    serving the request doesn't know about.
>>
>> I.e. from what I can tell this gives no benefits for someone using a
>> monolithic git server, except insofar as there would be a slight
>> decrease in network traffic if the average length of refs is less than
>> the length of a SHA-1.
>
> Yeah I agree that the motivation should probably be spelled out more,
> thanks for the suggestion.
>
>>
>> That's fair enough, just something we should prominently say.
>>
>> It does have the "disadvantage", if you can call it that, that it's
>> introducing a race condition between when we read the ref advertisement
>> and are promised XYZ refs, but may actually get ABC, but I can't think
>> of a reason anyone would care about this in practice.
>>
>> The reason I'm saying "another server [...] doesn't know about" above is
>> that 2/8 has this:
>>
>> 	if (read_ref(arg, &oid))
>> 		die("unknown ref %s", arg);
>>
>> Doesn't that mean that if server A in your pool advertises master, next
>> & pu, and you then go and fetch from server B advertising master & next,
>> but not "pu" that the clone will die?
>>
>> Presumably at Google you either have something to ensure a consistent
>> view, e.g. only advertise refs by name older than N seconds, or globally
>> update ref name but not their contents, and don't allow deleting refs
>> (or give them the same treatment).
>>
>> But that, and again, I may have misunderstood this whole thing,
>> significantly reduces the utility of this feature for anyone "in the
>> wild" since nothing shipped with "git" gives you that feature.
>>
>> The naïve way to do slave mirroring with stock git is to have a
>> post-receive hook that pushes to your mirrors in a for-loop, or has them
>> fetch from the master in a loop, and then round-robin LB those
>> servers. Due to the "die on nonexisting" semantics in this extension
>> that'll result in failed clones.
>>
>> So I think we should either be really vocal about that caveat, or
>> perhaps think of how we could make that configurable, e.g. what happens
>> if the server says "sorry, don't know about that one", and carries on
>> with the rest it does know about?
>
> Jonathan actually pointed this out to me earlier and I think the best
> way to deal with this is to just ignore the refs that the server doesn't
> know about instead of dying here. I mean its no worse than what we
> already have and we shouldn't hit this case too often.  And that way the
> fetch can still proceed.
>
>>
>> Is there a way for client & server to gracefully recover from that?
>> E.g. send "master" & "next" now, and when I pull again in a few seconds
>> I get the new "pu"?
>
> I think in this case the client would just need to wait for some amount
> of replication delay and attempt fetching at a later point.
>
>>
>> Also, as a digression isn't that a problem shared with protocol v2 in
>> general? I.e. without this extension isn't it going to make another
>> connection to the naïve LB'd mirroring setup described above and find
>> that SHA-1s as well as refs don't match?
>
> This is actually an issue with fetch using either v2 or v0.  Unless I'm
> misunderstanding what you're asking here.

Isn't the whole dialog in v1 guaranteed to be with one server from
intial ref advertisement to the client saying have/want, or is that just
with ssh?

In any case the reason the above is an issue here is because you're
getting the advertisement from a different server than you're
negotiating the pack with, right?

>>
>> BREAK.
>>
>> Also is if this E-Mail wasn't long enough, on a completely different
>> topic, in an earlier discussion in
>> https://public-inbox.org/git/87inaje1uv.fsf@evledraar.gmail.com/ I noted
>> that it would be neat-o to have optional wildmatch/pcre etc. matching
>> for the use case you're not caring about here (and I don't expect you
>> to, you're solving a different problem).
>>
>> But let's say I want to add that after this, and being unfamiliar with
>> the protocol v2 conventions. Would that be a whole new
>> ref-in-want-wildmatch-prefix capability with a new
>> want-ref-wildmatch-prefix verb, or is there some less verbose way we can
>> anticipate that use-case and internally version / advertise
>> sub-capabilities?
>>
>> I don't know if that makes any sense, and would be fine with just a
>> ref-in-want-wildmatch-prefix if that's the way to do it. I just think
>> it's inevitable that we'll have such a thing eventually, so it's worth
>> thinking about how such a future extension fits in.
>
> Yes back when introducing the server-side ref filtering in ls-refs we
> originally talked about included wildmatch or other forms of pattern
> matching.  We opted to not over complicate things and favored prefix
> matching because it didn't bake in some subset of globbing or regex and
> it was easier to compute on the server side.
>
> Anyway back to your question.  Yes if at some point in the future we
> wanted to add in wildmatch/pcre to the protocol for ls-refs or for
> ref-in-want then it could be added as a feature or capability.  I don't
> think it would require adding a whole new verb (it probably would for
> the ls-refs case since the verb used there is "ref-prefix") but the
> capability could mean that the "want-ref" verb now understands wildmatch
> patterns in addition to fully qualified refs.

Probably still makes sense to have it be a different verb since some
things in wildmatch / regex are metachars but may be valid in ref names.

Thanks!

  reply	other threads:[~2018-06-06 22:42 UTC|newest]

Thread overview: 122+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-05 17:51 [PATCH 0/8] ref-in-want Brandon Williams
2018-06-05 17:51 ` [PATCH 1/8] test-pkt-line: add unpack-sideband subcommand Brandon Williams
2018-06-05 17:51 ` [PATCH 2/8] upload-pack: implement ref-in-want Brandon Williams
2018-06-05 19:11   ` Ramsay Jones
2018-06-05 20:32   ` Ævar Arnfjörð Bjarmason
2018-06-06 21:32     ` Brandon Williams
2018-06-06 22:42       ` Ævar Arnfjörð Bjarmason [this message]
2018-06-06 22:45         ` Brandon Williams
2018-06-05 17:51 ` [PATCH 3/8] upload-pack: test negotiation with changing repository Brandon Williams
2018-06-05 17:51 ` [PATCH 4/8] fetch: refactor the population of peer ref OIDs Brandon Williams
2018-06-05 17:51 ` [PATCH 5/8] fetch: refactor fetch_refs into two functions Brandon Williams
2018-06-05 17:51 ` [PATCH 6/8] fetch: refactor to make function args narrower Brandon Williams
2018-06-05 17:51 ` [PATCH 7/8] fetch-pack: put shallow info in output parameter Brandon Williams
2018-06-05 17:51 ` [PATCH 8/8] fetch-pack: implement ref-in-want Brandon Williams
2018-06-13 21:39 ` [PATCH v2 0/8] ref-in-want Brandon Williams
2018-06-13 21:39   ` [PATCH v2 1/8] test-pkt-line: add unpack-sideband subcommand Brandon Williams
2018-06-14 18:09     ` Stefan Beller
2018-06-14 19:21       ` Brandon Williams
2018-06-13 21:39   ` [PATCH v2 2/8] upload-pack: implement ref-in-want Brandon Williams
2018-06-14 18:40     ` Stefan Beller
2018-06-14 18:52       ` Brandon Williams
2018-06-15 21:08     ` Junio C Hamano
2018-06-15 21:14       ` Junio C Hamano
2018-06-19 18:50       ` Brandon Williams
2018-06-19 20:37         ` Junio C Hamano
2018-06-19 23:14           ` Brandon Williams
2018-06-21 16:38             ` Junio C Hamano
2018-06-13 21:39   ` [PATCH v2 3/8] upload-pack: test negotiation with changing repository Brandon Williams
2018-06-14 19:23     ` Stefan Beller
2018-06-13 21:39   ` [PATCH v2 4/8] fetch: refactor the population of peer ref OIDs Brandon Williams
2018-06-13 21:39   ` [PATCH v2 5/8] fetch: refactor fetch_refs into two functions Brandon Williams
2018-06-13 21:39   ` [PATCH v2 6/8] fetch: refactor to make function args narrower Brandon Williams
2018-06-14 19:32     ` Stefan Beller
2018-06-13 21:39   ` [PATCH v2 7/8] fetch-pack: put shallow info in output parameter Brandon Williams
2018-06-14 19:42     ` Stefan Beller
2018-06-14 23:59     ` Jonathan Tan
2018-06-19 17:41       ` Brandon Williams
2018-06-13 21:39   ` [PATCH v2 8/8] fetch-pack: implement ref-in-want Brandon Williams
2018-06-14 19:56     ` Stefan Beller
2018-06-14 21:18       ` Brandon Williams
2018-06-22 22:29         ` Jonathan Nieder
2018-06-15 21:20   ` [PATCH v2 0/8] ref-in-want Junio C Hamano
2018-06-18 18:05     ` Brandon Williams
2018-06-20 21:32   ` [PATCH v3 " Brandon Williams
2018-06-20 21:32     ` [PATCH v3 1/8] test-pkt-line: add unpack-sideband subcommand Brandon Williams
2018-06-22 21:12       ` Jonathan Nieder
2018-06-20 21:32     ` [PATCH v3 2/8] upload-pack: implement ref-in-want Brandon Williams
2018-06-25 17:40       ` Jonathan Tan
2018-06-25 18:09       ` Jonathan Tan
2018-06-25 18:20         ` Brandon Williams
2018-06-20 21:32     ` [PATCH v3 3/8] upload-pack: test negotiation with changing repository Brandon Williams
2018-06-20 21:32     ` [PATCH v3 4/8] fetch: refactor the population of peer ref OIDs Brandon Williams
2018-06-25 17:45       ` Jonathan Tan
2018-06-20 21:32     ` [PATCH v3 5/8] fetch: refactor fetch_refs into two functions Brandon Williams
2018-06-22 21:26       ` Jonathan Nieder
2018-06-22 21:42       ` Jonathan Nieder
2018-06-20 21:32     ` [PATCH v3 6/8] fetch: refactor to make function args narrower Brandon Williams
2018-06-20 21:32     ` [PATCH v3 7/8] fetch-pack: put shallow info in output parameter Brandon Williams
2018-06-25 18:03       ` Jonathan Tan
2018-06-25 18:18         ` Brandon Williams
2018-06-20 21:32     ` [PATCH v3 8/8] fetch-pack: implement ref-in-want Brandon Williams
2018-06-22 23:01       ` Jonathan Nieder
2018-06-25 18:08         ` Brandon Williams
2018-06-25 18:53     ` [PATCH v4 0/8] ref-in-want Brandon Williams
2018-06-25 18:53       ` [PATCH v4 1/8] test-pkt-line: add unpack-sideband subcommand Brandon Williams
2018-06-25 18:53       ` [PATCH v4 2/8] upload-pack: implement ref-in-want Brandon Williams
2018-06-25 18:53       ` [PATCH v4 3/8] upload-pack: test negotiation with changing repository Brandon Williams
2018-06-25 22:27         ` Jonathan Tan
2018-06-25 18:53       ` [PATCH v4 4/8] fetch: refactor the population of peer ref OIDs Brandon Williams
2018-06-25 18:53       ` [PATCH v4 5/8] fetch: refactor fetch_refs into two functions Brandon Williams
2018-06-25 18:53       ` [PATCH v4 6/8] fetch: refactor to make function args narrower Brandon Williams
2018-06-25 22:36         ` Jonathan Tan
2018-06-25 18:53       ` [PATCH v4 7/8] fetch-pack: put shallow info in output parameter Brandon Williams
2018-06-25 18:53       ` [PATCH v4 8/8] fetch-pack: implement ref-in-want Brandon Williams
2018-06-25 23:03       ` [PATCH v4 0/8] ref-in-want Jonathan Tan
2018-06-26 20:54       ` [PATCH v5 " Brandon Williams
2018-06-26 20:54         ` [PATCH v5 1/8] test-pkt-line: add unpack-sideband subcommand Brandon Williams
2018-06-26 20:54         ` [PATCH v5 2/8] upload-pack: implement ref-in-want Brandon Williams
2018-06-26 21:25           ` Junio C Hamano
2018-06-27 18:05             ` Brandon Williams
2018-06-27 18:53               ` Junio C Hamano
2018-06-27 20:46                 ` Brandon Williams
2018-06-27 20:59                   ` Stefan Beller
2018-06-27 18:06             ` Jonathan Tan
2018-06-26 20:54         ` [PATCH v5 3/8] upload-pack: test negotiation with changing repository Brandon Williams
2018-06-26 21:34           ` Junio C Hamano
2018-06-27 18:09             ` Brandon Williams
2018-06-27 17:58           ` Jonathan Tan
2018-06-26 20:54         ` [PATCH v5 4/8] fetch: refactor the population of peer ref OIDs Brandon Williams
2018-06-26 20:54         ` [PATCH v5 5/8] fetch: refactor fetch_refs into two functions Brandon Williams
2018-06-26 20:54         ` [PATCH v5 6/8] fetch: refactor to make function args narrower Brandon Williams
2018-06-26 21:40           ` Junio C Hamano
2018-06-26 20:54         ` [PATCH v5 7/8] fetch-pack: put shallow info in output parameter Brandon Williams
2018-06-26 21:42           ` Junio C Hamano
2018-06-27 18:15             ` Brandon Williams
2018-06-26 20:54         ` [PATCH v5 8/8] fetch-pack: implement ref-in-want Brandon Williams
2018-06-27 18:09           ` Jonathan Tan
2018-06-27 18:18             ` Brandon Williams
2018-06-27 22:30         ` [PATCH v6 0/8] ref-in-want Brandon Williams
2018-06-27 22:30           ` [PATCH v6 1/8] test-pkt-line: add unpack-sideband subcommand Brandon Williams
2018-06-27 22:30           ` [PATCH v6 2/8] upload-pack: implement ref-in-want Brandon Williams
2018-06-27 22:30           ` [PATCH v6 3/8] upload-pack: test negotiation with changing repository Brandon Williams
2018-06-27 22:30           ` [PATCH v6 4/8] fetch: refactor the population of peer ref OIDs Brandon Williams
2018-06-27 22:30           ` [PATCH v6 5/8] fetch: refactor fetch_refs into two functions Brandon Williams
2018-06-27 22:30           ` [PATCH v6 6/8] fetch: refactor to make function args narrower Brandon Williams
2018-06-27 22:30           ` [PATCH v6 7/8] fetch-pack: put shallow info in output parameter Brandon Williams
2018-06-27 22:30           ` [PATCH v6 8/8] fetch-pack: implement ref-in-want Brandon Williams
2018-07-22  9:20             ` Duy Nguyen
2018-07-23 17:53               ` Brandon Williams
2018-07-23 18:13                 ` Duy Nguyen
2018-07-23 21:28                   ` Jonathan Nieder
2018-07-23 17:56               ` [PATCH] fetch-pack: mark die strings for translation Brandon Williams
2018-07-23 18:14                 ` Stefan Beller
2018-07-23 21:29                 ` Jonathan Nieder
2018-07-23 22:57                 ` Junio C Hamano
2018-07-23 22:59                   ` Junio C Hamano
2018-07-23 23:00                     ` Brandon Williams
2018-06-15 19:04 ` [PATCH 0/8] ref-in-want Jonathan Tan
2018-06-19 17:32   ` Brandon Williams
2018-06-19 19:23     ` Jonathan Tan
2018-06-19 23:16   ` Brandon Williams
2018-06-19 23:38     ` Jonathan Tan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87d0x3zgdf.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).