Re: [PATCH v4 2/3] connect, transport: add no-op arg for future patch

On Tue, Dec 22, 2020 at 01:54:19PM -0800, Jonathan Tan wrote:

quoted

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.

OK. Since the call stack is so deep, it's nice to get all of this diff
noise out of the way of the third patch.

It does make me wonder if we should be passing a struct like:

  struct transport_fetch_options {
	struct strvec ref_prefixes;
	char **unborn_head;
  }
  #define TRANSPORT_FETCH_OPTIONS_INIT = { STRVEC_INIT }

which would solve this problem once for any future options.

That's a good idea, and I've switched patch 2 to doing this. It also
makes it easier to explain (no "unborn_head" dummy variable that does
nothing, since I can just introduce "unborn_head" in patch 3).

quoted

@@ -455,7 +455,8 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
 			     struct ref **list, int for_push,
 			     const struct strvec *ref_prefixes,
 			     const struct string_list *server_options,
-			     int stateless_rpc)
+			     int stateless_rpc,
+			     char **unborn_head_target)

Is a single string enough? The way the protocol is defined, I think the
server is free to tell us about other unborn symrefs, too (but of course
our implementation does not). And I'm not sure what we'd do with such
values (in a "--mirror" clone, I guess we could make local copies of
them).

Should we be prepared for that at the transport layer, or is it
over-engineering?

With the struct, I think we're prepared for it.