[v4,00/10] serve.[ch]: general API cleanup + --advertise-refs cleanup

Message

Ævar Arnfjörð Bjarmason Aug. 5, 2021, 1:25 a.m. UTC

This is a general API cleanup and simplifiction of of serve.[ch] and
its interaction with "upload-pack --advertise-refs".

Previous rounds of this series had a way to define "startup config"
for commands hanging off the v2 server. That's now entirely gone,
along with prep work required for it.

For v3, see :
https://lore.kernel.org/git/[e-mail]/

The fixup for the missing test shebang has also been squashed in,
thanks Carlo!:
https://lore.kernel.org/git/[e-mail]/

Despite the overall diffstat this is a net deletion of code in any
meaningful sense of reducing complexity, i.e. it goes into the plus on
the diffstat due to the addition of previously missing tests, and the
use of designated initializers.

Ævar Arnfjörð Bjarmason (10):
  serve: mark has_capability() as static
  transport: rename "fetch" in transport_vtable to "fetch_refs"
  transport: use designated initializers
  serve: use designated initializers
  serve.[ch]: don't pass "struct strvec *keys" to commands
  serve: move transfer.advertiseSID check into session_id_advertise()
  serve.c: move version line to advertise_capabilities()
  {upload,receive}-pack tests: add --advertise-refs tests
  serve.[ch]: remove "serve_options", split up --advertise-refs code
  upload-pack: document and rename --advertise-refs

 Documentation/git-receive-pack.txt        |   5 +
 Documentation/git-upload-pack.txt         |  12 +-
 Documentation/technical/http-protocol.txt |   3 +
 Documentation/technical/protocol-v2.txt   |   3 +
 builtin/receive-pack.c                    |   3 +-
 builtin/upload-pack.c                     |  28 ++--
 http-backend.c                            |   2 +-
 ls-refs.c                                 |   3 +-
 ls-refs.h                                 |   4 +-
 protocol-caps.c                           |   3 +-
 protocol-caps.h                           |   4 +-
 serve.c                                   |  82 ++++++-----
 serve.h                                   |  12 +-
 t/helper/test-serve-v2.c                  |  14 +-
 t/t5555-http-smart-common.sh              | 161 ++++++++++++++++++++++
 transport-helper.c                        |  18 +--
 transport-internal.h                      |   2 +-
 transport.c                               |  32 ++---
 upload-pack.c                             |  21 +--
 upload-pack.h                             |  14 +-
 20 files changed, 298 insertions(+), 128 deletions(-)
 create mode 100755 t/t5555-http-smart-common.sh

Range-diff against v3:
 1:  192fb64ef82 =  1:  13bb1a06078 serve: mark has_capability() as static
 2:  d716bd3c537 =  2:  162f717436f transport: rename "fetch" in transport_vtable to "fetch_refs"
 3:  d31690614af =  3:  d3d5818adda transport: use designated initializers
 4:  13f1a8d8325 =  4:  8da15c373cc serve: use designated initializers
 5:  99eeff6f890 <  -:  ----------- serve.c: add call_{advertise,command}() indirection
 6:  be719dc3dc1 <  -:  ----------- serve: add support for a "startup" git_config() callback
12:  e4eb31b5b8e !  5:  7714f81d62c serve.[ch]: don't pass "struct strvec *keys" to commands
    @@ Commit message
         2018-03-15) was passing in the raw capabilities "keys", but nothing
         downstream of it ever used them.
     
    -    Let's remove that code because it's not needed, and because if and
    -    when we need to pass data about the advertisement (I have some WIP
    -    patches for that), it makes much more sense to have the serve.c parse
    -    the capabilities, and then pass specific information we need down than
    -    expecting its API users to re-do their own parsing of the raw data.
    +    Let's remove that code because it's not needed. If we do end up
    +    needing to pass information about the advertisement in the future
    +    it'll make more sense to have serve.c parse the capabilities keys and
    +    pass the result of its parsing, rather than expecting expecting its
    +    API users to parse the same keys again.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
    @@ ls-refs.h
     -int ls_refs(struct repository *r, struct strvec *keys,
     -	    struct packet_reader *request);
     +int ls_refs(struct repository *r, struct packet_reader *request);
    - int ls_refs_startup_config(const char *var, const char *value, void *data);
      int ls_refs_advertise(struct repository *r, struct strbuf *value);
      
    + #endif /* LS_REFS_H */
     
      ## protocol-caps.c ##
     @@ protocol-caps.c: static void send_info(struct repository *r, struct packet_writer *writer,
    - 	strbuf_release(&send_buffer);
    + 	}
      }
      
     -int cap_object_info(struct repository *r, struct strvec *keys,
    @@ protocol-caps.h
      #endif /* PROTOCOL_CAPS_H */
     
      ## serve.c ##
    -@@ serve.c: static int session_id_advertise(struct repository *r, struct strbuf *value)
    - }
    - 
    - typedef int (*advertise_fn_t)(struct repository *r, struct strbuf *value);
    --typedef int (*command_fn_t)(struct repository *r, struct strvec *keys,
    -+typedef int (*command_fn_t)(struct repository *r,
    - 			    struct packet_reader *request);
    - 
    - struct protocol_capability {
     @@ serve.c: struct protocol_capability {
      
      	/*
    @@ serve.c: struct protocol_capability {
      	 * use to read the command specific part of the request.  Every command
      	 * MUST read until a flush packet is seen before sending a response.
      	 *
    -@@ serve.c: static int call_command(struct protocol_capability *command,
    + 	 * This field should be NULL for capabilities which are not commands.
    + 	 */
    +-	int (*command)(struct repository *r,
    +-		       struct strvec *keys,
    +-		       struct packet_reader *request);
    ++	int (*command)(struct repository *r, struct packet_reader *request);
    + };
      
    - 	read_startup_config(command);
    + static struct protocol_capability capabilities[] = {
    +@@ serve.c: static int process_request(void)
    + 	if (has_capability(&keys, "session-id", &client_sid))
    + 		trace2_data_string("transfer", NULL, "client-sid", client_sid);
      
    --	return command->command(r, keys, request);
    -+	return command->command(r, request);
    - }
    +-	command->command(the_repository, &keys, &reader);
    ++	command->command(the_repository, &reader);
      
    - void protocol_v2_advertise_capabilities(void)
    + 	strvec_clear(&keys);
    + 	return 0;
     
      ## upload-pack.c ##
     @@ upload-pack.c: enum fetch_state {
    @@ upload-pack.c: enum fetch_state {
      	struct upload_pack_data data;
     
      ## upload-pack.h ##
    -@@ upload-pack.h: void upload_pack(const int advertise_refs, const int stateless_rpc,
    - 		 const int timeout);
    +@@ upload-pack.h: struct upload_pack_options {
    + void upload_pack(struct upload_pack_options *options);
      
      struct repository;
     -struct strvec;
 -:  ----------- >  6:  33db6b7cf53 serve: move transfer.advertiseSID check into session_id_advertise()
 7:  b7928ddbe9b !  7:  9828e08cbad serve.c: move version line to advertise_capabilities()
    @@ Commit message
         serve.c: move version line to advertise_capabilities()
     
         The advertise_capabilities() is only called from serve() and we always
    -    emit this version line before it, it makes more sense to consider the
    -    capabilities part of a "header" that has the version, so let's move
    -    the writing of the version there.
    +    emit this version line before it. In a subsequent commit I'll make
    +    builtin/upload-pack.c sometimes call advertise_capabilities()
    +    directly, so it'll make sense to have this line emitted by
    +    advertise_capabilities(), not serve() itself.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
 9:  beafe9811c1 !  8:  fd58abb7420 {upload,receive}-pack tests: add --advertise-refs tests
    @@ Commit message
     
      ## t/t5555-http-smart-common.sh (new) ##
     @@
    ++#!/bin/sh
    ++
     +test_description='test functionality common to smart fetch & push'
     +
     +. ./test-lib.sh
 8:  fb80f152713 !  9:  c9a35868933 serve.[ch]: remove "serve_options", split up --advertise-refs code
    @@ http-backend.c: static void get_info_refs(struct strbuf *hdr, char *arg)
      
     
      ## serve.c ##
    -@@ serve.c: static int call_command(struct protocol_capability *command,
    - 	return command->command(r, keys, request);
    - }
    +@@ serve.c: static struct protocol_capability capabilities[] = {
    + 	},
    + };
      
     -static void advertise_capabilities(void)
     +void protocol_v2_advertise_capabilities(void)
    @@ upload-pack.h
     +		 const int timeout);
      
      struct repository;
    - struct strvec;
    + struct packet_reader;
10:  c6870b5f18a = 10:  2006cb234b6 upload-pack: document and rename --advertise-refs
11:  2d4c3d0d463 <  -:  ----------- upload-pack.c: convert to new serve.c "startup" config cb

Comments

Derrick Stolee Aug. 24, 2021, 4:55 p.m. UTC | #1

On 8/4/2021 9:25 PM, Ævar Arnfjörð Bjarmason wrote:
> This is a general API cleanup and simplifiction of of serve.[ch] and
> its interaction with "upload-pack --advertise-refs".

s/simplifiction/simplification/ (not that it matters much)

The changes included in this series seem innocent enough. The added
tests are nice. Organizing struct initializers is good to do before
adding options, which I see are coming later.

This version LGTM.

Thanks,
-Stolee