mbox series

[v3,0/3] Cloning with remote unborn HEAD

Message ID cover.1608587839.git.jonathantanmy@google.com (mailing list archive)
Headers show
Series Cloning with remote unborn HEAD | expand

Message

Jonathan Tan Dec. 21, 2020, 10:30 p.m. UTC 
Thanks everyone for your comments. Changes from v2:

Patch 1:

 - I took a look at head_ref_namespaced() was hoping that it could be
   refactored to meet my new needs, but I don't think it's feasible.
   head_ref_namespaced() seems to have a precise use - for when we
   already have a callback that we're using with
   for_each_namespaced_ref(), and we want to use it with the HEAD ref as
   well. Instead what I did is to eliminate the use of
   head_ref_namespaced() here, and used send_possibly_unborn_head() for
   both the regular and the new unborn case.

 - Renamed null_oid to avoid shadowing, as pointed out by Junio.

 - Changed to a boolean lsrefs.allowUnborn, so no more advertise/allow
   distinction. Currently it still defaults to unallowed. Peff, what did
   you mean in [1] by the following:

> So I dunno. My biggest complaint is that the config option defaults to
> _off_.  So it's helping load-balanced rollouts, but creating complexity
> for everyone else who might want to enable the feature.

   So it seems like you're saying that it should default to "on", but at
   the same time you are talking about enabling the feature (which seems
   to imply switching it from "off" to "on"). (Also, note that this is a
   server-side thing; on the client-side, Git will always use what the
   server gives and there is no option to control this.)

 - Added documentation of lsrefs.allowUnborn, which I forgot.

Patch 2:

 - Used Junio's suggested commit message.

Patch 3:

 - No changes except what was necessary due to the config option change.

[1] https://lore.kernel.org/git/X9xJLWdFJfNJTn0p@coredump.intra.peff.net/

Jonathan Tan (3):
  ls-refs: report unborn targets of symrefs
  connect, transport: add no-op arg for future patch
  clone: respect remote unborn HEAD

 Documentation/config.txt                |  2 +
 Documentation/config/init.txt           |  2 +-
 Documentation/config/lsrefs.txt         |  3 ++
 Documentation/technical/protocol-v2.txt | 10 ++++-
 builtin/clone.c                         | 19 +++++++--
 builtin/fetch-pack.c                    |  3 +-
 builtin/fetch.c                         |  2 +-
 builtin/ls-remote.c                     |  2 +-
 builtin/remote.c                        |  2 +-
 connect.c                               | 29 ++++++++++++--
 ls-refs.c                               | 51 +++++++++++++++++++++++--
 ls-refs.h                               |  1 +
 remote.h                                |  3 +-
 serve.c                                 |  2 +-
 t/t5606-clone-options.sh                |  7 ++--
 t/t5702-protocol-v2.sh                  |  9 +++++
 transport-helper.c                      |  7 +++-
 transport-internal.h                    | 13 +++----
 transport.c                             | 29 ++++++++------
 transport.h                             |  7 +++-
 20 files changed, 160 insertions(+), 43 deletions(-)
 create mode 100644 Documentation/config/lsrefs.txt

Range-diff against v2:
1:  7f5b50c7b2 ! 1:  7d20ec323a 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" (guarded by 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.
    +    "unborn" feature in "ls-refs" (guarded by the lsrefs.allowunborn
    +    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.
     
         This change is only for protocol v2. A similar change for protocol v0
         would require independent protocol design (there being no analogous
    @@ Commit message
     
         Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
     
    + ## Documentation/config.txt ##
    +@@ Documentation/config.txt: include::config/interactive.txt[]
    + 
    + include::config/log.txt[]
    + 
    ++include::config/lsrefs.txt[]
    ++
    + include::config/mailinfo.txt[]
    + 
    + include::config/mailmap.txt[]
    +
    + ## Documentation/config/lsrefs.txt (new) ##
    +@@
    ++lsrefs.allowUnborn::
    ++	Allow the server to send information about unborn symrefs during the
    ++	protocol v2 ref advertisement.
    +
      ## Documentation/technical/protocol-v2.txt ##
     @@ Documentation/technical/protocol-v2.txt: ls-refs takes in the following arguments:
      	When specified, only references having a prefix matching one of
    @@ ls-refs.c: static int send_ref(const char *refname, const struct object_id *oid,
     +	struct strbuf namespaced = STRBUF_INIT;
     +	struct object_id oid;
     +	int flag;
    -+	int null_oid;
    ++	int oid_is_null;
     +
     +	strbuf_addf(&namespaced, "%sHEAD", get_git_namespace());
     +	resolve_ref_unsafe(namespaced.buf, 0, &oid, &flag);
    -+	null_oid = is_null_oid(&oid);
    -+	if (!null_oid || (data->symrefs && (flag & REF_ISSYMREF)))
    -+		send_ref(namespaced.buf, null_oid ? NULL : &oid, flag, data);
    ++	oid_is_null = is_null_oid(&oid);
    ++	if (!oid_is_null ||
    ++	    (data->unborn && data->symrefs && (flag & REF_ISSYMREF)))
    ++		send_ref(namespaced.buf, oid_is_null ? NULL : &oid, flag, data);
     +	strbuf_release(&namespaced);
     +}
     +
    @@ ls-refs.c: static int send_ref(const char *refname, const struct object_id *oid,
     +{
     +	struct ls_refs_data *data = cb_data;
     +
    -+	if (!strcmp("lsrefs.unborn", var))
    -+		data->allow_unborn = !strcmp(value, "allow") ||
    -+			!strcmp(value, "advertise");
    ++	if (!strcmp("lsrefs.allowunborn", var))
    ++		data->allow_unborn = git_config_bool(var, value);
    ++
      	/*
      	 * We only serve fetches over v2 for now, so respect only "uploadpack"
      	 * config. This may need to eventually be expanded to "receive", but we
    @@ ls-refs.c: int ls_refs(struct repository *r, struct strvec *keys,
      		die(_("expected flush after ls-refs arguments"));
      
     -	head_ref_namespaced(send_ref, &data);
    -+	if (data.unborn)
    -+		send_possibly_unborn_head(&data);
    -+	else
    -+		head_ref_namespaced(send_ref, &data);
    ++	send_possibly_unborn_head(&data);
      	for_each_namespaced_ref(send_ref, &data);
      	packet_flush(1);
      	strvec_clear(&data.prefixes);
    @@ ls-refs.c: int ls_refs(struct repository *r, struct strvec *keys,
     +int ls_refs_advertise(struct repository *r, struct strbuf *value)
     +{
     +	if (value) {
    -+		char *str = NULL;
    ++		int allow_unborn_value;
     +
    -+		if (!repo_config_get_string(the_repository, "lsrefs.unborn",
    -+					    &str) &&
    -+		    !strcmp("advertise", str)) {
    ++		if (!repo_config_get_bool(the_repository,
    ++					 "lsrefs.allowunborn",
    ++					 &allow_unborn_value) &&
    ++		    allow_unborn_value)
     +			strbuf_addstr(value, "unborn");
    -+			free(str);
    -+		}
     +	}
     +
     +	return 1;
2:  e24fb6d746 ! 2:  b5a78857eb connect, transport: add no-op arg for future patch
    @@ Metadata
      ## Commit message ##
         connect, transport: add no-op arg for future patch
     
    -    A future patch will require transport_get_remote_refs() and
    -    get_remote_refs() to gain a new argument. Add the argument in this
    -    patch, with no effect on execution, so that the future patch only needs
    -    to concern itself with new logic.
    +    In a future patch we plan to return the name of an unborn current branch
    +    from deep in the callchain to a caller via a new pointer parameter that
    +    points at a variable in the caller when the caller calls
    +    get_remote_refs() and transport_get_remote_refs(). Add the parameter to
    +    functions involved in the callchain, but no caller passes an actual
    +    argument yet in this step. Thus, the future patch only needs to concern
    +    itself with new logic.
     
         Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
     
3:  6fcb3b16ce ! 3:  c2303dc976 clone: respect remote unborn HEAD
    @@ t/t5606-clone-options.sh: test_expect_success 'redirected clone -v does show pro
      test_expect_success 'chooses correct default initial branch name' '
     -	git init --bare empty &&
     +	git -c init.defaultBranch=foo init --bare empty &&
    -+	test_config -C empty lsrefs.unborn advertise &&
    ++	test_config -C empty lsrefs.allowUnborn true &&
      	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
      
     +test_expect_success 'clone of empty repo propagates name of default branch' '
     +	git -c init.defaultBranch=mydefaultbranch init file_empty_parent &&
    -+	test_config -C file_empty_parent lsrefs.unborn advertise &&
    ++	test_config -C file_empty_parent lsrefs.allowUnborn true &&
     +
     +	git -c init.defaultBranch=main -c protocol.version=2 \
     +		clone "file://$(pwd)/file_empty_parent" file_empty_child &&
     +	grep "refs/heads/mydefaultbranch" file_empty_child/.git/HEAD
     +'
    -+
    -+test_expect_success '...but not if it is not advertised' '
    -+	test_config -C file_empty_parent lsrefs.unborn none &&
    -+
    -+	git -c init.defaultBranch=main -c protocol.version=2 \
    -+		clone "file://$(pwd)/file_empty_parent" file_empty_child_2 &&
    -+	grep "refs/heads/main" file_empty_child_2/.git/HEAD
    -+'
     +
      test_expect_success 'fetch with file:// using protocol v2' '
      	test_when_finished "rm -f log" &&

Comments

Junio C Hamano Dec. 21, 2020, 11:48 p.m. UTC | #1 
Jonathan Tan <jonathantanmy@google.com> writes:

> Jonathan Tan (3):
>   ls-refs: report unborn targets of symrefs
>   connect, transport: add no-op arg for future patch
>   clone: respect remote unborn HEAD

This passes standalone all its tests, but in 'seen', seems to break
some tests.

https://travis-ci.org/github/git/git/builds/750936755 has just
started, but my local tests before publishing the day's integration
result failed 5702, 0031, and 5606 with this topic, and all passed
without the topic.

Thanks.
Jeff King Jan. 21, 2021, 8:14 p.m. UTC | #2 
On Mon, Dec 21, 2020 at 02:30:58PM -0800, Jonathan Tan wrote:

> > So I dunno. My biggest complaint is that the config option defaults to
> > _off_.  So it's helping load-balanced rollouts, but creating complexity
> > for everyone else who might want to enable the feature.
> 
>    So it seems like you're saying that it should default to "on", but at
>    the same time you are talking about enabling the feature (which seems
>    to imply switching it from "off" to "on"). (Also, note that this is a
>    server-side thing; on the client-side, Git will always use what the
>    server gives and there is no option to control this.)

Sorry, I missed this question over the holidays. Yes, what I meant is
that everyone should really want this feature on, because it gives
strictly more information and lets the client be smarter.

But if it defaults to "off", server operators may well not bother to
turn it on (or even know it exists). And the clients who would benefit
may have trouble convincing server operators to do so.

So I would strongly prefer it default to "on", and the onus be on server
operators with non-atomic clusters, etc, to turn it off when deploying
in their environments.

-Peff