[RFC PATCH 0/2] branch: implement in-process --recurse-submodules

* [RFC PATCH 0/2] branch: implement in-process --recurse-submodules
@ 2021-09-21 23:25 Glen Choo
  2021-09-21 23:25 ` [RFC PATCH 1/2] refs: pass struct repository *r through to write_ref_to_lockfile() Glen Choo
  2021-09-21 23:25 ` [RFC PATCH 2/2] branch: add --recurse-submodules option for branch creation Glen Choo
  0 siblings, 2 replies; 7+ messages in thread
From: Glen Choo @ 2021-09-21 23:25 UTC (permalink / raw)
  To: git; +Cc: Glen Choo

These patches are an attempt to implement git branch
--recurse-submodules in-process. This is part of the general submodule
UX improvements discussed in [1]. Specifically, this is the use case
discussed in the section titled "Create Mode (git switch -c / git
checkout -b)", but without checking out the newly created branch.

Doing this in-process allows "git switch -c" and "git checkout -b" to
reuse create_branch(), instead of relying on a child process to create
the branch. create_branch() nominally takes a struct repository *
parameter, but there is some hidden reliance on the_repository and
global state.

Broadly speaking, I'd like to know how we should approach
"--recurse-submodules", particularly when we are implementing [1]. I'm
sending these patches as an RFC because I think this is somewhat
indicative of submodule issues.

In this patchset, branching works slightly differently between the
superproject and submodule (skip ahead for specifics). There are two
very obvious alternatives that can address this:

* A: only implement --recurse-submodules behavior after we are able to
  eliminate any kind of dependence on the_repository/global state that
  shouldn't be shared.
* B: implement --recurse-submodules as child processes, which won't be
  bothered by global state.

My cursory thoughts on the matter are that A seems like a good direction
for code health, but it seems difficult to do in practice. B seems
hacky, but it might be a good stopgap while we work on A.

Obviously these aren't the only options and this isn't the end of the
discussion, so I'd appreciate any thoughts on the matter :)

Changes:
* refactor the refs.h functions to accept struct repository * where
  reasonable.
* add two new functions to refs.h that accept struct
  repository *, repo_ref_transaction_commit() and
  repo_ref_transaction_prepare().
* change ref_transaction_commit() and ref_transaction_prepare() to be
  thin wrappers of their repo_ counterparts.
* use repo_ref_transaction_commit() in create_branch()
* add a create_branches() function to builtin/branch.c that recursively
  creates branches in submodules

What doesn't work (marked with NEEDSWORK):
* branch tracking is disabled for submodules because remotes.c only
  holds state for a single repository
* similarly, bare repository checking does not work as expected because
  environment.c only holds state for a single repository

What is questionable (specific to these patches, not
--recurse-submodules in general):
* files-backend.c only uses the_repository to validate the oid being
  used and it is the only ref backend that does so. Instead of passing
  struct repository * through more places in refs.h, we might be able to
  eliminate this check altogether. This is discussed briefly in [2].
* create_branches() implements its own submodule iterating, which is
  similar to that of ls-files, but wholly different from
  submodule--helper.c.

[1] https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com/
[2] https://lore.kernel.org/git/20210916172432.1073546-1-jonathantanmy@google.com/

Glen Choo (2):
  refs: pass struct repository *r through to write_ref_to_lockfile()
  branch: add --recurse-submodules option for branch creation

 branch.c                  | 26 +++++++++------
 branch.h                  |  4 +--
 builtin/branch.c          | 69 ++++++++++++++++++++++++++++++++++++---
 builtin/checkout.c        |  4 +--
 refs.c                    | 21 ++++++------
 refs.h                    | 18 +++++++---
 refs/debug.c              | 14 ++++----
 refs/files-backend.c      | 30 ++++++++---------
 refs/packed-backend.c     |  7 ++--
 refs/refs-internal.h      |  7 ++--
 t/helper/test-ref-store.c |  2 +-
 t/t3200-branch.sh         | 58 ++++++++++++++++++++++++++++++++
 12 files changed, 199 insertions(+), 61 deletions(-)

-- 
2.33.0.464.g1972c5931b-goog

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