[v2,0/2] Last big GIT_TEST_PROTOCOL_VERSION=2 fix, hopefully

Message

Jonathan Tan March 26, 2019, 5:53 p.m. UTC

Updated to remove the unnecessary NULL check and memory leak that Peff
noticed [1].

(Only commit 2/2 is changed, so the range-diff only shows 1 commit.)

[1] https://public-inbox.org/git/[e-mail]/

Jonathan Tan (2):
  fetch-pack: call prepare_shallow_info only if v0
  fetch-pack: respect --no-update-shallow in v2

 commit.h     |  4 ++++
 fetch-pack.c | 51 +++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 45 insertions(+), 10 deletions(-)

Range-diff against v1:
1:  c4d2f409e2 ! 1:  943b1cbc61 fetch-pack: respect --no-update-shallow in v2
    @@ -33,13 +33,11 @@
      static void receive_shallow_info(struct fetch_pack_args *args,
     -				 struct packet_reader *reader)
     +				 struct packet_reader *reader,
    ++				 struct oid_array *shallows,
     +				 struct shallow_info *si)
      {
     -	int line_received = 0;
    -+	struct oid_array *shallows;
     +	int unshallow_received = 0;
    -+
    -+	shallows = xcalloc(1, sizeof(*shallows));
      
      	process_section_header(reader, "shallow-info", 0);
      	while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
    @@ -77,8 +75,6 @@
     +
     +		for (i = 0; i < shallows->nr; i++)
     +			register_shallow(the_repository, &shallows->oid[i]);
    -+		oid_array_clear(shallows);
    -+		free(shallows);
      		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
      					NULL);
      		args->deepen = 1;
    @@ -95,14 +91,13 @@
     +		else
     +			alternate_shallow_file = NULL;
      	} else {
    -+		free(shallows);
      		alternate_shallow_file = NULL;
      	}
    - }
     @@
      				    int fd[2],
      				    const struct ref *orig_ref,
      				    struct ref **sought, int nr_sought,
    ++				    struct oid_array *shallows,
     +				    struct shallow_info *si,
      				    char **pack_lockfile)
      {
    @@ -112,16 +107,36 @@
      			/* Check for shallow-info section */
      			if (process_section_header(&reader, "shallow-info", 1))
     -				receive_shallow_info(args, &reader);
    -+				receive_shallow_info(args, &reader, si);
    ++				receive_shallow_info(args, &reader, shallows, si);
      
      			if (process_section_header(&reader, "wanted-refs", 1))
      				receive_wanted_refs(&reader, sought, nr_sought);
     @@
    + {
    + 	struct ref *ref_cpy;
    + 	struct shallow_info si;
    ++	struct oid_array shallows_scratch = OID_ARRAY_INIT;
    + 
    + 	fetch_pack_setup();
    + 	if (nr_sought)
    +@@
    + 		die(_("no matching remote head"));
    + 	}
    + 	if (version == protocol_v2) {
    +-		if (shallow && shallow->nr)
    ++		if (shallow->nr)
      			BUG("Protocol V2 does not provide shallows at this point in the fetch");
      		memset(&si, 0, sizeof(si));
      		ref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,
    --					   pack_lockfile);
    -+					   &si, pack_lockfile);
    ++					   &shallows_scratch, &si,
    + 					   pack_lockfile);
      	} else {
      		prepare_shallow_info(&si, shallow);
    - 		ref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,
    +@@
    + 	update_shallow(args, sought, nr_sought, &si);
    + cleanup:
    + 	clear_shallow_info(&si);
    ++	oid_array_clear(&shallows_scratch);
    + 	return ref_cpy;
    + }
    +

Comments

Jeff King March 26, 2019, 6:20 p.m. UTC | #1

On Tue, Mar 26, 2019 at 10:53:24AM -0700, Jonathan Tan wrote:

> Updated to remove the unnecessary NULL check and memory leak that Peff
> noticed [1].
> 
> (Only commit 2/2 is changed, so the range-diff only shows 1 commit.)

But isn't this line:

>     + 	if (version == protocol_v2) {
>     +-		if (shallow && shallow->nr)
>     ++		if (shallow->nr)
>       			BUG("Protocol V2 does not provide shallows at this point in the fetch");

added by patch 1? It's added with "shallow &&" in patch 1, and then
modified in patch 2.

I think the "it's never NULL" property is true even before this series,
right?

-Peff