| Message ID | cover.1612493309.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Cloning with remote unborn HEAD | expand |
Jonathan Tan <jonathantanmy@google.com> writes: > For what it's worth, here's v7 with advertise/allow/ignore and by > default, advertise. I think that some server operators will have use for > this feature, and people who want to disable it for whatever reason can > still do so. The main disadvantage is complexity - the server knob that > server administrators will need to control (but between a simpler > allow/ignore knob and a more complicated advertise/allow/ignore knob, I > think we might as well go for the slightly more complex one) and > complexity in the code (but now that is constrained to one function and > a few global variables). > > As you can see from the range-diff, not much has changed from v6. > > I've also included Junio's suggestion of tightening the promise made by > the server (when the client says "unborn"). This looks reasonable overall, especially with the feature turned on by default, we'd hopefully get reasonable exposure from the get-go. Let's mark the topic to be merged to 'next' soonish, unless people object. Thanks.
On Thu, Feb 04, 2021 at 09:25:57PM -0800, Junio C Hamano wrote: > Jonathan Tan <jonathantanmy@google.com> writes: > > > For what it's worth, here's v7 with advertise/allow/ignore and by > > default, advertise. I think that some server operators will have use for > > this feature, and people who want to disable it for whatever reason can > > still do so. The main disadvantage is complexity - the server knob that > > server administrators will need to control (but between a simpler > > allow/ignore knob and a more complicated advertise/allow/ignore knob, I > > think we might as well go for the slightly more complex one) and > > complexity in the code (but now that is constrained to one function and > > a few global variables). > > > > As you can see from the range-diff, not much has changed from v6. > > > > I've also included Junio's suggestion of tightening the promise made by > > the server (when the client says "unborn"). > > This looks reasonable overall, especially with the feature turned on > by default, we'd hopefully get reasonable exposure from the get-go. > > Let's mark the topic to be merged to 'next' soonish, unless people > object. No objection here. I sent a few comments in response to patch 1; the doc fix and the leak are probably worth addressing before it hits next. I couldn't help express my thoughts on the protocol wording, but it may be best to ignore me. ;) Thanks for working on this, Jonathan. I think it's a very useful feature. -Peff
On Fri, Feb 05 2021, Junio C Hamano wrote: > Jonathan Tan <jonathantanmy@google.com> writes: > >> For what it's worth, here's v7 with advertise/allow/ignore and by >> default, advertise. I think that some server operators will have use for >> this feature, and people who want to disable it for whatever reason can >> still do so. The main disadvantage is complexity - the server knob that >> server administrators will need to control (but between a simpler >> allow/ignore knob and a more complicated advertise/allow/ignore knob, I >> think we might as well go for the slightly more complex one) and >> complexity in the code (but now that is constrained to one function and >> a few global variables). >> >> As you can see from the range-diff, not much has changed from v6. >> >> I've also included Junio's suggestion of tightening the promise made by >> the server (when the client says "unborn"). > > This looks reasonable overall, especially with the feature turned on > by default, we'd hopefully get reasonable exposure from the get-go. > > Let's mark the topic to be merged to 'next' soonish, unless people > object. FWIW I'm still unclear on re [1] whether Jonathan thinks the semantics of this shouldn't be documented for <reasons>, or whether he just doesn't want to submit the patch and I should, or something else. I still think this "remote as config source" without actually explaining that it is is a very glaring hole in the docs[2]. And in [3] I noted that we introduced the word "branches" into protocol-v2.txt for the first time without defining what it means (presumably just refs/heads/*, but then let's say so...). There was a reply promising clarification, but I note that v7 still just says "branches". To be clear I'm perfectly fine with a note in a CL to the effect of "had X feedback on last version, Ævar said Y and Z both of which I think are stupid ideas, so I'm not doing them" :) The only thing I mind is being left hanging to the effect of not knowing if a diff you proposed is considered bad by the primary author, or if I should just submit it myself as a patch on top or whatever. It also saves other people following along with reviews time, since they can read later cover letters and get a brief summary of some side-discussion in an earlier round without diving into it themselves. Me too honestly, sometimes I come back to these threads and completely forgot what I had to say in earlier rounds, and when I try to find out it's hit-and-miss whether I agree with much/any of it :) 1. https://lore.kernel.org/git/87h7n3p363.fsf@evledraar.gmail.com/ 2. https://lore.kernel.org/git/878s8apthr.fsf@evledraar.gmail.com/ 3. https://lore.kernel.org/git/87k0rzp3qx.fsf@evledraar.gmail.com/
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > And in [3] I noted that we introduced the word "branches" into > protocol-v2.txt for the first time without defining what it means > (presumably just refs/heads/*, but then let's say so...). There was a > reply promising clarification, but I note that v7 still just says > "branches". I do not think it so bad to mention "branch" in the part that explains things to humans in the terminology they are used to. It is a different matter to introduce EBNF terminal <branch> without a proper definition of the word, but I od not think we are doing so here. I however have to agree with the need to tighten what gets sent; that is why a suggested replacement in my earlier review phrased it this way: unborn If HEAD is a symref pointing to an unborn branch <b>, the server reports it as "unborn HEAD symref-target:refs/heads/<b>" in its response. to make it clear that a full refname is sent for the pointee by HEAD. Thanks.
For what it's worth, here's v7 with advertise/allow/ignore and by default, advertise. I think that some server operators will have use for this feature, and people who want to disable it for whatever reason can still do so. The main disadvantage is complexity - the server knob that server administrators will need to control (but between a simpler allow/ignore knob and a more complicated advertise/allow/ignore knob, I think we might as well go for the slightly more complex one) and complexity in the code (but now that is constrained to one function and a few global variables). As you can see from the range-diff, not much has changed from v6. I've also included Junio's suggestion of tightening the promise made by the server (when the client says "unborn"). Jonathan Tan (3): ls-refs: report unborn targets of symrefs connect, transport: encapsulate arg in struct clone: respect remote unborn HEAD Documentation/config.txt | 2 + Documentation/config/init.txt | 2 +- Documentation/config/lsrefs.txt | 9 +++ Documentation/technical/protocol-v2.txt | 11 +++- builtin/clone.c | 34 +++++++++--- builtin/fetch-pack.c | 3 +- builtin/fetch.c | 18 +++--- builtin/ls-remote.c | 9 +-- connect.c | 32 ++++++++++- ls-refs.c | 74 ++++++++++++++++++++++++- ls-refs.h | 1 + remote.h | 4 +- serve.c | 2 +- t/t5606-clone-options.sh | 8 ++- t/t5701-git-serve.sh | 2 +- t/t5702-protocol-v2.sh | 25 +++++++++ transport-helper.c | 5 +- transport-internal.h | 10 +--- transport.c | 23 ++++---- transport.h | 29 +++++++--- 20 files changed, 240 insertions(+), 63 deletions(-) create mode 100644 Documentation/config/lsrefs.txt Range-diff against v6: 1: 411bbafe25 ! 1: 2d35075369 ls-refs: report unborn targets of symrefs @@ Commit message Currently, symrefs that have unborn targets (such as in this case) are not communicated by the protocol. Teach Git to advertise and support the "unborn" feature in "ls-refs" (by default, this is advertised, but - server administrators may turn this off through the lsrefs.allowunborn + server administrators may turn this off through the lsrefs.unborn config). This feature indicates that "ls-refs" supports the "unborn" argument; when it is specified, "ls-refs" will send the HEAD symref with the name of its unborn target. @@ Documentation/config.txt: include::config/interactive.txt[] ## Documentation/config/lsrefs.txt (new) ## @@ -+lsrefs.allowUnborn:: -+ Allow the server to send information about unborn symrefs during the -+ protocol v2 ref advertisement. ++lsrefs.unborn:: ++ May be "advertise" (the default), "allow", or "ignore". If "advertise", ++ the server will respond to the client sending "unborn" (as described in ++ protocol-v2.txt) and will advertise support for this feature during the ++ protocol v2 capability advertisement. "allow" is the same as ++ "advertise" except that the server will not advertise support for this ++ feature; this is useful for load-balanced servers that cannot be ++ updated automatically (for example), since the administrator could ++ configure "allow", then after a delay, configure "advertise". ## Documentation/technical/protocol-v2.txt ## @@ Documentation/technical/protocol-v2.txt: ls-refs takes in the following arguments: @@ Documentation/technical/protocol-v2.txt: ls-refs takes in the following argument +included in the client's request. + + unborn -+ The server may send symrefs pointing to unborn branches in the form -+ "unborn <refname> symref-target:<target>". ++ The server will send information about HEAD even if it is a symref ++ pointing to an unborn branch in the form "unborn HEAD ++ symref-target:<target>". + The output of ls-refs is as follows: @@ ls-refs.c #include "config.h" +static int config_read; ++static int advertise_unborn; +static int allow_unborn; + +static void ensure_config_read(void) +{ ++ char *str = NULL; ++ + if (config_read) + return; + -+ if (repo_config_get_bool(the_repository, "lsrefs.allowunborn", -+ &allow_unborn)) ++ if (repo_config_get_string(the_repository, "lsrefs.unborn", &str)) { + /* -+ * If there is no such config, set it to 1 to allow it by ++ * If there is no such config, advertise and allow it by + * default. + */ ++ advertise_unborn = 1; + allow_unborn = 1; ++ } else { ++ if (!strcmp(str, "advertise")) { ++ advertise_unborn = 1; ++ allow_unborn = 1; ++ } else if (!strcmp(str, "allow")) { ++ allow_unborn = 1; ++ } else if (!strcmp(str, "ignore")) { ++ /* do nothing */ ++ } else { ++ die(_("invalid value '%s' for lsrefs.unborn"), str); ++ } ++ } + config_read = 1; +} + @@ ls-refs.c: int ls_refs(struct repository *r, struct strvec *keys, +{ + if (value) { + ensure_config_read(); -+ if (allow_unborn) ++ if (advertise_unborn) + strbuf_addstr(value, "unborn"); + } + 2: fad1ebe6b6 = 2: d4ed13d02e connect, transport: encapsulate arg in struct 3: 45a48ccc0d ! 3: a3e5a0a7c5 clone: respect remote unborn HEAD @@ t/t5606-clone-options.sh: test_expect_success 'redirected clone -v does show pro - git init --bare empty && + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \ + git -c init.defaultBranch=foo init --bare empty && -+ test_config -C empty lsrefs.allowUnborn true && ++ test_config -C empty lsrefs.unborn advertise && GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \ git -c init.defaultBranch=up clone empty whats-up && - test refs/heads/up = $(git -C whats-up symbolic-ref HEAD) && @@ t/t5702-protocol-v2.sh: test_expect_success 'clone with file:// using protocol v + + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \ + git -c init.defaultBranch=mydefaultbranch init file_empty_parent && -+ test_config -C file_empty_parent lsrefs.allowUnborn false && ++ test_config -C file_empty_parent lsrefs.unborn ignore && + + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \ + git -c init.defaultBranch=main -c protocol.version=2 \