[PATCH v5 0/5] remote: replace static variables with struct remote_state

* [PATCH v5 0/5] remote: replace static variables with struct remote_state
       [not found] <id:20211028183101.41013-1-chooglen@google.com>
@ 2021-11-18  0:53 ` Glen Choo
  2021-11-18  0:53   ` [PATCH v5 1/5] t5516: add test case for pushing remote refspecs Glen Choo
                     ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Glen Choo @ 2021-11-18  0:53 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, Junio C Hamano, Glen Choo

This series aims to make the remotes subsystem work with non-the_repository,
which will allow submodule remotes to be accessed in-process, rather than
through child processes. This is accomplished by creating a struct remote_state
and adding it to struct repository.

Thanks for the kind review, Jonathan. I've incorporated most of the feedback
from v4, except squashing patches 2-4. My reasoning is that those changes are
mechanical and keeping them separate makes it more obvious whether or not each
step has been done correctly. However, I agree that they are just intermediate
steps that touch the same lines, so I am happy to squash them if other reviewers
prefer that.

Changes since v4:
* Fixed an incorrect description of 'git push' without explicit refspecs
* In patch 5, remove the branches array and keep the hashmap (note that I did
  not make a similar change to remotes)
* Drop patch 6 because it is untested in this series (will incorporate into a
  future series)

Changes since v3:
* Add a test case for pushing to a remote in detached HEAD. This test
  would have caught the segfault that resulted in this reroll.
* Remove the NEEDSWORK saying that init_remotes_hash() should be moved
  into remote_state_new() and just do it.
* Remove the backpointer to remote_state and add a remote_state
  parameter instead.
* In patch 4, add more remotes_* functions. These functions were not
  needed in v3 because of the backpointer.
* In patch 5, add a function that checks if a branch is in a repo. Add a
  branch hashmap that makes this operation fast.
* In patch 6, add more repo_* functions. These functions were not needed
  in v3 because of the backpointer.

Changes since v2:
* Add .remote_state to struct branch and struct remote, changing the
  implementation appropriately.
* In patch 2, properly consider the initialized state of remote_state.
  In v2, I forgot to convert a static inside read_config() into a
  private member of struct remote_state. Fix this.
* In a new patch 3, add helper methods that get a remote via
  remote_state and the remote name.
* Move read_config(repo) calls to the external facing-functions. This keeps
  "struct repository" away from the remote.c internals.

Changes since v1:
* In v1, we moved static variables into the_repository->remote_state in
  two steps: static variables > static remote_state >
  the_repository->remote_state. In v2, make this change in one step:
  static variables > the_repository->remote_state.
* Add more instances of repo_* that were missed.

Glen Choo (5):
  t5516: add test case for pushing remote refspecs
  remote: move static variables into per-repository struct
  remote: use remote_state parameter internally
  remote: remove the_repository->remote_state from static methods
  remote: die if branch is not found in repository

 remote.c              | 368 +++++++++++++++++++++++++++++-------------
 remote.h              |  35 ++++
 repository.c          |   8 +
 repository.h          |   4 +
 t/t5516-fetch-push.sh |   9 ++
 5 files changed, 311 insertions(+), 113 deletions(-)

Range-diff against v4:
1:  9b29ec27c6 ! 1:  7e60457e11 t5516: add test case for pushing remote refspecs
    @@ Metadata
      ## Commit message ##
         t5516: add test case for pushing remote refspecs

    -    In detached HEAD, "git push remote-name" should push the refspecs in
    -    remote.remote-name.push. Since there is no test case that checks this
    -    behavior, add one.
    +    "git push remote-name" (that is, with no refspec given on the command
    +    line) should push the refspecs in remote.remote-name.push. There is no
    +    test case that checks this behavior in detached HEAD, so add one.

         Signed-off-by: Glen Choo <chooglen@google.com>

    @@ t/t5516-fetch-push.sh: do

      done

    -+test_expect_success "push to remote with detached HEAD and config remote.*.push = src:dest" '
    ++test_expect_success "push to remote with no explicit refspec and config remote.*.push = src:dest" '
     +	mk_test testrepo heads/main &&
     +	git checkout $the_first_commit &&
     +	test_config remote.there.url testrepo &&
2:  ca9b5ab66a = 2:  ecc637ee74 remote: move static variables into per-repository struct
3:  5d6a245cae = 3:  a915155979 remote: use remote_state parameter internally
4:  53f2e31f72 = 4:  8ef43570e9 remote: remove the_repository->remote_state from static methods
5:  d3281c14eb ! 5:  8bb7bddda4 remote: die if branch is not found in repository
    @@ Commit message

         To prevent misuse, add a die_on_missing_branch() helper function that
         dies if a given branch is not from a given repository. Speed up the
    -    existence check by using a new branches_hash hashmap to remote_state,
    -    and use the hashmap to remove the branch array iteration in
    -    make_branch().
    +    existence check by replacing the branches list with a branches_hash
    +    hashmap.

         Like read_config(), die_on_missing_branch() is only called from
         non-static functions; static functions are less prone to misuse because
    @@ remote.c: static void add_merge(struct branch *branch, const char *name)
     +	if (ret)
     +		return ret;

    - 	ALLOC_GROW(remote_state->branches, remote_state->branches_nr + 1,
    - 		   remote_state->branches_alloc);
    -@@ remote.c: static struct branch *make_branch(struct remote_state *remote_state,
    +-	ALLOC_GROW(remote_state->branches, remote_state->branches_nr + 1,
    +-		   remote_state->branches_alloc);
    + 	CALLOC_ARRAY(ret, 1);
    +-	remote_state->branches[remote_state->branches_nr++] = ret;
      	ret->name = xstrndup(name, len);
      	ret->refname = xstrfmt("refs/heads/%s", ret->name);

    @@ remote.c: struct remote_state *remote_state_new(void)
      	return r;
      }

    +@@ remote.c: void remote_state_clear(struct remote_state *remote_state)
    + 	remote_state->remotes_nr = 0;
    + 
    + 	hashmap_clear_and_free(&remote_state->remotes_hash, struct remote, ent);
    +-
    +-	for (i = 0; i < remote_state->branches_nr; i++) {
    +-		FREE_AND_NULL(remote_state->branches[i]);
    +-	}
    +-	FREE_AND_NULL(remote_state->branches);
    +-	remote_state->branches_alloc = 0;
    +-	remote_state->branches_nr = 0;
    ++	hashmap_clear_and_free(&remote_state->branches_hash, struct remote, ent);
    + }

      ## remote.h ##
     @@ remote.h: struct remote_state {
    - 	struct branch **branches;
    - 	int branches_alloc;
    - 	int branches_nr;
    + 	int remotes_nr;
    + 	struct hashmap remotes_hash;
    + 
    +-	struct branch **branches;
    +-	int branches_alloc;
    +-	int branches_nr;
     +	struct hashmap branches_hash;

      	struct branch *current_branch;
6:  0974994cc6 < -:  ---------- remote: add struct repository parameter to external functions
-- 
2.33.GIT

^ permalink raw reply	[flat|nested] 7+ messages in thread