{"thread":{"id":"56562","subject":"[RFC PATCH 0/2] branch: implement in-process --recurse-submodules","startedAt":"2021-09-21T23:25:47Z","lastAt":"2021-09-22T17:24:54Z","messageCount":7,"participants":["Glen Choo","Ævar Arnfjörð Bjarmason","Philippe Blain"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"436688","messageId":"20210921232529.81811-1-chooglen@google.com","threadId":"56562","inReplyTo":null,"subject":"[RFC PATCH 0/2] branch: implement in-process --recurse-submodules","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-21T23:25:27Z","receivedAt":"2021-09-21T23:25:47Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"These patches are an attempt to implement git branch\n--recurse-submodules in-process. This is part of the general submodule\nUX improvements discussed in [1]. Specifically, this is the use case\ndiscussed in the section titled \"Create Mode (git switch -c / git\ncheckout -b)\", but without checking out the newly created branch.\n\nDoing this in-process allows \"git switch -c\" and \"git checkout -b\" to\nreuse create_branch(), instead of relying on a child process to create\nthe branch. create_branch() nominally takes a struct repository *\nparameter, but there is some hidden reliance on the_repository and\nglobal state.\n\nBroadly speaking, I'd like to know how we should approach\n\"--recurse-submodules\", particularly when we are implementing [1]. I'm\nsending these patches as an RFC because I think this is somewhat\nindicative of submodule issues.\n\nIn this patchset, branching works slightly differently between the\nsuperproject and submodule (skip ahead for specifics). There are two\nvery obvious alternatives that can address this:\n\n* A: only implement --recurse-submodules behavior after we are able to\n  eliminate any kind of dependence on the_repository/global state that\n  shouldn't be shared.\n* B: implement --recurse-submodules as child processes, which won't be\n  bothered by global state.\n\nMy cursory thoughts on the matter are that A seems like a good direction\nfor code health, but it seems difficult to do in practice. B seems\nhacky, but it might be a good stopgap while we work on A.\n\nObviously these aren't the only options and this isn't the end of the\ndiscussion, so I'd appreciate any thoughts on the matter :)\n\nChanges:\n* refactor the refs.h functions to accept struct repository * where\n  reasonable.\n* add two new functions to refs.h that accept struct\n  repository *, repo_ref_transaction_commit() and\n  repo_ref_transaction_prepare().\n* change ref_transaction_commit() and ref_transaction_prepare() to be\n  thin wrappers of their repo_ counterparts.\n* use repo_ref_transaction_commit() in create_branch()\n* add a create_branches() function to builtin/branch.c that recursively\n  creates branches in submodules\n\nWhat doesn't work (marked with NEEDSWORK):\n* branch tracking is disabled for submodules because remotes.c only\n  holds state for a single repository\n* similarly, bare repository checking does not work as expected because\n  environment.c only holds state for a single repository\n\nWhat is questionable (specific to these patches, not\n--recurse-submodules in general):\n* files-backend.c only uses the_repository to validate the oid being\n  used and it is the only ref backend that does so. Instead of passing\n  struct repository * through more places in refs.h, we might be able to\n  eliminate this check altogether. This is discussed briefly in [2].\n* create_branches() implements its own submodule iterating, which is\n  similar to that of ls-files, but wholly different from\n  submodule--helper.c.\n\n[1] https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com/\n[2] https://lore.kernel.org/git/20210916172432.1073546-1-jonathantanmy@google.com/\n\nGlen Choo (2):\n  refs: pass struct repository *r through to write_ref_to_lockfile()\n  branch: add --recurse-submodules option for branch creation\n\n branch.c                  | 26 +++++++++------\n branch.h                  |  4 +--\n builtin/branch.c          | 69 ++++++++++++++++++++++++++++++++++++---\n builtin/checkout.c        |  4 +--\n refs.c                    | 21 ++++++------\n refs.h                    | 18 +++++++---\n refs/debug.c              | 14 ++++----\n refs/files-backend.c      | 30 ++++++++---------\n refs/packed-backend.c     |  7 ++--\n refs/refs-internal.h      |  7 ++--\n t/helper/test-ref-store.c |  2 +-\n t/t3200-branch.sh         | 58 ++++++++++++++++++++++++++++++++\n 12 files changed, 199 insertions(+), 61 deletions(-)\n\n-- \n2.33.0.464.g1972c5931b-goog\n\n"},{"id":"436689","messageId":"20210921232529.81811-2-chooglen@google.com","threadId":"56562","inReplyTo":"20210921232529.81811-1-chooglen@google.com","subject":"[RFC PATCH 1/2] refs: pass struct repository *r through to write_ref_to_lockfile()","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-21T23:25:28Z","receivedAt":"2021-09-21T23:25:52Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"In refs/files-backend.c, write_ref_to_lockfile() uses the_repository to\nvalidate the oid of the ref being written. Thus, any function that\nincludes write_ref_to_lockfile() in its call chain has an implicit\ndependence on the odb of the_repository. This is undesirable in the case\nwhere we are using oids that are not in the_repository, e.g. when we are\nupdating refs in a submodule.\n\nLet's fix this by passing struct repository * as a parameter through to\nwrite_ref_to_lockfile().\n\nTo avoid breaking existing call sites, the existing function\nsignatures in refs.h are preserved. struct repository * is only passed\nto new functions; these new functions have names prefixed with repo_.\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n refs.c                    | 21 +++++++++++----------\n refs.h                    | 18 ++++++++++++++----\n refs/debug.c              | 14 ++++++++------\n refs/files-backend.c      | 30 +++++++++++++++---------------\n refs/packed-backend.c     |  7 ++++---\n refs/refs-internal.h      |  7 ++++---\n t/helper/test-ref-store.c |  2 +-\n 7 files changed, 57 insertions(+), 42 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8b9f7c3a80..c217512cd3 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2105,7 +2105,7 @@ static int run_transaction_hook(struct ref_transaction *transaction,\n \treturn ret;\n }\n \n-int ref_transaction_prepare(struct ref_transaction *transaction,\n+int repo_ref_transaction_prepare(struct repository *r, struct ref_transaction *transaction,\n \t\t\t    struct strbuf *err)\n {\n \tstruct ref_store *refs = transaction->ref_store;\n@@ -2132,7 +2132,7 @@ int ref_transaction_prepare(struct ref_transaction *transaction,\n \t\treturn -1;\n \t}\n \n-\tret = refs->be->transaction_prepare(refs, transaction, err);\n+\tret = refs->be->transaction_prepare(r, refs, transaction, err);\n \tif (ret)\n \t\treturn ret;\n \n@@ -2172,7 +2172,8 @@ int ref_transaction_abort(struct ref_transaction *transaction,\n \treturn ret;\n }\n \n-int ref_transaction_commit(struct ref_transaction *transaction,\n+int repo_ref_transaction_commit(struct repository *r,\n+\t\t\t   struct ref_transaction *transaction,\n \t\t\t   struct strbuf *err)\n {\n \tstruct ref_store *refs = transaction->ref_store;\n@@ -2181,7 +2182,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tswitch (transaction->state) {\n \tcase REF_TRANSACTION_OPEN:\n \t\t/* Need to prepare first. */\n-\t\tret = ref_transaction_prepare(transaction, err);\n+\t\tret = repo_ref_transaction_prepare(r, transaction, err);\n \t\tif (ret)\n \t\t\treturn ret;\n \t\tbreak;\n@@ -2421,36 +2422,36 @@ int delete_refs(const char *msg, struct string_list *refnames,\n \treturn refs_delete_refs(get_main_ref_store(the_repository), msg, refnames, flags);\n }\n \n-int refs_rename_ref(struct ref_store *refs, const char *oldref,\n+int refs_rename_ref(struct repository *r, struct ref_store *refs, const char *oldref,\n \t\t    const char *newref, const char *logmsg)\n {\n \tchar *msg;\n \tint retval;\n \n \tmsg = normalize_reflog_message(logmsg);\n-\tretval = refs->be->rename_ref(refs, oldref, newref, msg);\n+\tretval = refs->be->rename_ref(r, refs, oldref, newref, msg);\n \tfree(msg);\n \treturn retval;\n }\n \n int rename_ref(const char *oldref, const char *newref, const char *logmsg)\n {\n-\treturn refs_rename_ref(get_main_ref_store(the_repository), oldref, newref, logmsg);\n+\treturn refs_rename_ref(the_repository, get_main_ref_store(the_repository), oldref, newref, logmsg);\n }\n \n-int refs_copy_existing_ref(struct ref_store *refs, const char *oldref,\n+int refs_copy_existing_ref(struct repository *r, struct ref_store *refs, const char *oldref,\n \t\t    const char *newref, const char *logmsg)\n {\n \tchar *msg;\n \tint retval;\n \n \tmsg = normalize_reflog_message(logmsg);\n-\tretval = refs->be->copy_ref(refs, oldref, newref, msg);\n+\tretval = refs->be->copy_ref(r, refs, oldref, newref, msg);\n \tfree(msg);\n \treturn retval;\n }\n \n int copy_existing_ref(const char *oldref, const char *newref, const char *logmsg)\n {\n-\treturn refs_copy_existing_ref(get_main_ref_store(the_repository), oldref, newref, logmsg);\n+\treturn refs_copy_existing_ref(the_repository, get_main_ref_store(the_repository), oldref, newref, logmsg);\n }\ndiff --git a/refs.h b/refs.h\nindex 48970dfc7e..157d14d7a3 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -524,13 +524,13 @@ char *refs_shorten_unambiguous_ref(struct ref_store *refs,\n char *shorten_unambiguous_ref(const char *refname, int strict);\n \n /** rename ref, return 0 on success **/\n-int refs_rename_ref(struct ref_store *refs, const char *oldref,\n+int refs_rename_ref(struct repository *r, struct ref_store *refs, const char *oldref,\n \t\t    const char *newref, const char *logmsg);\n int rename_ref(const char *oldref, const char *newref,\n \t\t\tconst char *logmsg);\n \n /** copy ref, return 0 on success **/\n-int refs_copy_existing_ref(struct ref_store *refs, const char *oldref,\n+int refs_copy_existing_ref(struct repository *r, struct ref_store *refs, const char *oldref,\n \t\t    const char *newref, const char *logmsg);\n int copy_existing_ref(const char *oldref, const char *newref,\n \t\t\tconst char *logmsg);\n@@ -706,8 +706,13 @@ int ref_transaction_verify(struct ref_transaction *transaction,\n  * Callers who don't need such fine-grained control over committing\n  * reference transactions should just call `ref_transaction_commit()`.\n  */\n-int ref_transaction_prepare(struct ref_transaction *transaction,\n+int repo_ref_transaction_prepare(struct repository *r, struct ref_transaction *transaction,\n \t\t\t    struct strbuf *err);\n+static inline int ref_transaction_prepare(struct ref_transaction *transaction,\n+\t\t\t\t\t  struct strbuf *err)\n+{\n+\treturn repo_ref_transaction_prepare(the_repository, transaction, err);\n+}\n \n /*\n  * Commit all of the changes that have been queued in transaction, as\n@@ -716,8 +721,13 @@ int ref_transaction_prepare(struct ref_transaction *transaction,\n  * transaction, write an error message to `err`, and return one of the\n  * `TRANSACTION_*` constants\n  */\n-int ref_transaction_commit(struct ref_transaction *transaction,\n+int repo_ref_transaction_commit(struct repository *r, struct ref_transaction *transaction,\n \t\t\t   struct strbuf *err);\n+static inline int ref_transaction_commit(struct ref_transaction *transaction,\n+\t\t\t\t\t struct strbuf *err)\n+{\n+\treturn repo_ref_transaction_commit(the_repository, transaction, err);\n+}\n \n /*\n  * Abort `transaction`, which has been begun and possibly prepared,\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 1a7a9e11cf..224ee76563 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -38,14 +38,15 @@ static int debug_init_db(struct ref_store *refs, struct strbuf *err)\n \treturn res;\n }\n \n-static int debug_transaction_prepare(struct ref_store *refs,\n+static int debug_transaction_prepare(struct repository *r,\n+\t\t\t\t     struct ref_store *refs,\n \t\t\t\t     struct ref_transaction *transaction,\n \t\t\t\t     struct strbuf *err)\n {\n \tstruct debug_ref_store *drefs = (struct debug_ref_store *)refs;\n \tint res;\n \ttransaction->ref_store = drefs->refs;\n-\tres = drefs->refs->be->transaction_prepare(drefs->refs, transaction,\n+\tres = drefs->refs->be->transaction_prepare(r, drefs->refs, transaction,\n \t\t\t\t\t\t   err);\n \ttrace_printf_key(&trace_refs, \"transaction_prepare: %d\\n\", res);\n \treturn res;\n@@ -153,23 +154,24 @@ static int debug_delete_refs(struct ref_store *ref_store, const char *msg,\n \treturn res;\n }\n \n-static int debug_rename_ref(struct ref_store *ref_store, const char *oldref,\n+static int debug_rename_ref(struct repository *r,\n+\t\t\t    struct ref_store *ref_store, const char *oldref,\n \t\t\t    const char *newref, const char *logmsg)\n {\n \tstruct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;\n-\tint res = drefs->refs->be->rename_ref(drefs->refs, oldref, newref,\n+\tint res = drefs->refs->be->rename_ref(r, drefs->refs, oldref, newref,\n \t\t\t\t\t      logmsg);\n \ttrace_printf_key(&trace_refs, \"rename_ref: %s -> %s \\\"%s\\\": %d\\n\", oldref, newref,\n \t\tlogmsg, res);\n \treturn res;\n }\n \n-static int debug_copy_ref(struct ref_store *ref_store, const char *oldref,\n+static int debug_copy_ref(struct repository *r, struct ref_store *ref_store, const char *oldref,\n \t\t\t  const char *newref, const char *logmsg)\n {\n \tstruct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;\n \tint res =\n-\t\tdrefs->refs->be->copy_ref(drefs->refs, oldref, newref, logmsg);\n+\t\tdrefs->refs->be->copy_ref(r, drefs->refs, oldref, newref, logmsg);\n \ttrace_printf_key(&trace_refs, \"copy_ref: %s -> %s \\\"%s\\\": %d\\n\", oldref, newref,\n \t\tlogmsg, res);\n \treturn res;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 74c0385873..2bd6115484 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1308,14 +1308,14 @@ static int rename_tmp_log(struct files_ref_store *refs, const char *newrefname)\n \treturn ret;\n }\n \n-static int write_ref_to_lockfile(struct ref_lock *lock,\n+static int write_ref_to_lockfile(struct repository *r, struct ref_lock *lock,\n \t\t\t\t const struct object_id *oid, struct strbuf *err);\n static int commit_ref_update(struct files_ref_store *refs,\n \t\t\t     struct ref_lock *lock,\n \t\t\t     const struct object_id *oid, const char *logmsg,\n \t\t\t     struct strbuf *err);\n \n-static int files_copy_or_rename_ref(struct ref_store *ref_store,\n+static int files_copy_or_rename_ref(struct repository *r, struct ref_store *ref_store,\n \t\t\t    const char *oldrefname, const char *newrefname,\n \t\t\t    const char *logmsg, int copy)\n {\n@@ -1427,7 +1427,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \t}\n \toidcpy(&lock->old_oid, &orig_oid);\n \n-\tif (write_ref_to_lockfile(lock, &orig_oid, &err) ||\n+\tif (write_ref_to_lockfile(r, lock, &orig_oid, &err) ||\n \t    commit_ref_update(refs, lock, &orig_oid, logmsg, &err)) {\n \t\terror(\"unable to write current sha1 into %s: %s\", newrefname, err.buf);\n \t\tstrbuf_release(&err);\n@@ -1448,7 +1448,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \n \tflag = log_all_ref_updates;\n \tlog_all_ref_updates = LOG_REFS_NONE;\n-\tif (write_ref_to_lockfile(lock, &orig_oid, &err) ||\n+\tif (write_ref_to_lockfile(r, lock, &orig_oid, &err) ||\n \t    commit_ref_update(refs, lock, &orig_oid, NULL, &err)) {\n \t\terror(\"unable to write current sha1 into %s: %s\", oldrefname, err.buf);\n \t\tstrbuf_release(&err);\n@@ -1472,19 +1472,19 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \treturn ret;\n }\n \n-static int files_rename_ref(struct ref_store *ref_store,\n+static int files_rename_ref(struct repository *r, struct ref_store *ref_store,\n \t\t\t    const char *oldrefname, const char *newrefname,\n \t\t\t    const char *logmsg)\n {\n-\treturn files_copy_or_rename_ref(ref_store, oldrefname,\n+\treturn files_copy_or_rename_ref(r, ref_store, oldrefname,\n \t\t\t\t newrefname, logmsg, 0);\n }\n \n-static int files_copy_ref(struct ref_store *ref_store,\n+static int files_copy_ref(struct repository *r, struct ref_store *ref_store,\n \t\t\t    const char *oldrefname, const char *newrefname,\n \t\t\t    const char *logmsg)\n {\n-\treturn files_copy_or_rename_ref(ref_store, oldrefname,\n+\treturn files_copy_or_rename_ref(r, ref_store, oldrefname,\n \t\t\t\t newrefname, logmsg, 1);\n }\n \n@@ -1683,14 +1683,14 @@ static int files_log_ref_write(struct files_ref_store *refs,\n  * Write oid into the open lockfile, then close the lockfile. On\n  * errors, rollback the lockfile, fill in *err and return -1.\n  */\n-static int write_ref_to_lockfile(struct ref_lock *lock,\n+static int write_ref_to_lockfile(struct repository *r, struct ref_lock *lock,\n \t\t\t\t const struct object_id *oid, struct strbuf *err)\n {\n \tstatic char term = '\\n';\n \tstruct object *o;\n \tint fd;\n \n-\to = parse_object(the_repository, oid);\n+\to = parse_object(r, oid);\n \tif (!o) {\n \t\tstrbuf_addf(err,\n \t\t\t    \"trying to write ref '%s' with nonexistent object %s\",\n@@ -2390,7 +2390,7 @@ static int check_old_oid(struct ref_update *update, struct object_id *oid,\n  * - If it is an update of head_ref, add a corresponding REF_LOG_ONLY\n  *   update of HEAD.\n  */\n-static int lock_ref_for_update(struct files_ref_store *refs,\n+static int lock_ref_for_update(struct repository *r, struct files_ref_store *refs,\n \t\t\t       struct ref_update *update,\n \t\t\t       struct ref_transaction *transaction,\n \t\t\t       const char *head_ref,\n@@ -2496,7 +2496,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t * The reference already has the desired\n \t\t\t * value, so we don't need to write it.\n \t\t\t */\n-\t\t} else if (write_ref_to_lockfile(lock, &update->new_oid,\n+\t\t} else if (write_ref_to_lockfile(r, lock, &update->new_oid,\n \t\t\t\t\t\t err)) {\n \t\t\tchar *write_err = strbuf_detach(err, NULL);\n \n@@ -2577,7 +2577,7 @@ static void files_transaction_cleanup(struct files_ref_store *refs,\n \ttransaction->state = REF_TRANSACTION_CLOSED;\n }\n \n-static int files_transaction_prepare(struct ref_store *ref_store,\n+static int files_transaction_prepare(struct repository *r, struct ref_store *ref_store,\n \t\t\t\t     struct ref_transaction *transaction,\n \t\t\t\t     struct strbuf *err)\n {\n@@ -2667,7 +2667,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n \n-\t\tret = lock_ref_for_update(refs, update, transaction,\n+\t\tret = lock_ref_for_update(r, refs, update, transaction,\n \t\t\t\t\t  head_ref, &affected_refnames, err);\n \t\tif (ret)\n \t\t\tgoto cleanup;\n@@ -2708,7 +2708,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \n \t\tif (is_packed_transaction_needed(refs->packed_ref_store,\n \t\t\t\t\t\t packed_transaction)) {\n-\t\t\tret = ref_transaction_prepare(packed_transaction, err);\n+\t\t\tret = repo_ref_transaction_prepare(r, packed_transaction, err);\n \t\t\t/*\n \t\t\t * A failure during the prepare step will abort\n \t\t\t * itself, but not free. Do that now, and disconnect\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex f8aa97d799..e6aa1841ba 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1403,7 +1403,8 @@ static void packed_transaction_cleanup(struct packed_ref_store *refs,\n \ttransaction->state = REF_TRANSACTION_CLOSED;\n }\n \n-static int packed_transaction_prepare(struct ref_store *ref_store,\n+static int packed_transaction_prepare(struct repository *r,\n+\t\t\t\t      struct ref_store *ref_store,\n \t\t\t\t      struct ref_transaction *transaction,\n \t\t\t\t      struct strbuf *err)\n {\n@@ -1577,14 +1578,14 @@ static int packed_create_symref(struct ref_store *ref_store,\n \tBUG(\"packed reference store does not support symrefs\");\n }\n \n-static int packed_rename_ref(struct ref_store *ref_store,\n+static int packed_rename_ref(struct repository *r, struct ref_store *ref_store,\n \t\t\t    const char *oldrefname, const char *newrefname,\n \t\t\t    const char *logmsg)\n {\n \tBUG(\"packed reference store does not support renaming references\");\n }\n \n-static int packed_copy_ref(struct ref_store *ref_store,\n+static int packed_copy_ref(struct repository *r, struct ref_store *ref_store,\n \t\t\t   const char *oldrefname, const char *newrefname,\n \t\t\t   const char *logmsg)\n {\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 3155708345..1b7a68db8c 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -530,7 +530,8 @@ typedef struct ref_store *ref_store_init_fn(const char *gitdir,\n \n typedef int ref_init_db_fn(struct ref_store *refs, struct strbuf *err);\n \n-typedef int ref_transaction_prepare_fn(struct ref_store *refs,\n+typedef int ref_transaction_prepare_fn(struct repository *r,\n+\t\t\t\t       struct ref_store *refs,\n \t\t\t\t       struct ref_transaction *transaction,\n \t\t\t\t       struct strbuf *err);\n \n@@ -553,10 +554,10 @@ typedef int create_symref_fn(struct ref_store *ref_store,\n \t\t\t     const char *logmsg);\n typedef int delete_refs_fn(struct ref_store *ref_store, const char *msg,\n \t\t\t   struct string_list *refnames, unsigned int flags);\n-typedef int rename_ref_fn(struct ref_store *ref_store,\n+typedef int rename_ref_fn(struct repository *r, struct ref_store *ref_store,\n \t\t\t  const char *oldref, const char *newref,\n \t\t\t  const char *logmsg);\n-typedef int copy_ref_fn(struct ref_store *ref_store,\n+typedef int copy_ref_fn(struct repository *r, struct ref_store *ref_store,\n \t\t\t  const char *oldref, const char *newref,\n \t\t\t  const char *logmsg);\n \ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex b314b81a45..ad47488e5b 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -99,7 +99,7 @@ static int cmd_rename_ref(struct ref_store *refs, const char **argv)\n \tconst char *newref = notnull(*argv++, \"newref\");\n \tconst char *logmsg = *argv++;\n \n-\treturn refs_rename_ref(refs, oldref, newref, logmsg);\n+\treturn refs_rename_ref(the_repository, refs, oldref, newref, logmsg);\n }\n \n static int each_ref(const char *refname, const struct object_id *oid,\n-- \n2.33.0.464.g1972c5931b-goog\n\n"},{"id":"436690","messageId":"20210921232529.81811-3-chooglen@google.com","threadId":"56562","inReplyTo":"20210921232529.81811-1-chooglen@google.com","subject":"[RFC PATCH 2/2] branch: add --recurse-submodules option for branch creation","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-21T23:25:29Z","receivedAt":"2021-09-21T23:25:59Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"When working with a superproject and submodules, it can be helpful to\ncreate topic branches to coordinate the work between each repository.\n\nTeach cmd_branch to accept the --recurse-submodules option when creating\nbranches. When specified, like\n\n  git branch --recurse-submodules topic\n\ngit-branch will create the \"topic\" branch in the superproject and all\nsubmodules.\n\nrecurse_submodules is only supported for creating a branch. git-branch\nwill fail if the user attempts to use --recurse-submodules with anything\nother than creating a branch. If a user has submodule.recurse in their\nconfig, git-branch will use --recurse-submodules for creating a branch,\nbut will treat it as unset for any other operation.\n\nThere is no easy way to get the remotes of a submodule because remotes.c\nstores state as static variables. As such, branch tracking is disabled\nwhen using --recurse-submodules.\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n branch.c           | 26 ++++++++++-------\n branch.h           |  4 +--\n builtin/branch.c   | 69 ++++++++++++++++++++++++++++++++++++++++++----\n builtin/checkout.c |  4 +--\n t/t3200-branch.sh  | 58 ++++++++++++++++++++++++++++++++++++++\n 5 files changed, 142 insertions(+), 19 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 07a46430b3..1acf4aa407 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -137,6 +137,9 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \tstruct tracking tracking;\n \tint config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n \n+\tif (track == BRANCH_TRACK_NEVER)\n+\t\treturn;\n+\n \tmemset(&tracking, 0, sizeof(tracking));\n \ttracking.spec.dst = (char *)orig_ref;\n \tif (for_each_remote(find_tracked_branch, &tracking))\n@@ -183,12 +186,12 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)\n  * Return 1 if the named branch already exists; return 0 otherwise.\n  * Fill ref with the full refname for the branch.\n  */\n-int validate_branchname(const char *name, struct strbuf *ref)\n+int validate_branchname(struct repository *r, const char *name, struct strbuf *ref)\n {\n \tif (strbuf_check_branch_ref(ref, name))\n \t\tdie(_(\"'%s' is not a valid branch name.\"), name);\n \n-\treturn ref_exists(ref->buf);\n+\treturn refs_ref_exists(get_main_ref_store(r), ref->buf);\n }\n \n /*\n@@ -197,11 +200,11 @@ int validate_branchname(const char *name, struct strbuf *ref)\n  * Return 1 if the named branch already exists; return 0 otherwise.\n  * Fill ref with the full refname for the branch.\n  */\n-int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n+int validate_new_branchname(struct repository *r, const char *name, struct strbuf *ref, int force)\n {\n \tconst char *head;\n \n-\tif (!validate_branchname(name, ref))\n+\tif (!validate_branchname(r, name, ref))\n \t\treturn 0;\n \n \tif (!force)\n@@ -209,6 +212,9 @@ int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n \t\t    ref->buf + strlen(\"refs/heads/\"));\n \n \thead = resolve_ref_unsafe(\"HEAD\", 0, NULL, NULL);\n+\t/*\n+\t * NEEDSWORK is_bare_repository() references the_repository.\n+\t */\n \tif (!is_bare_repository() && head && !strcmp(head, ref->buf))\n \t\tdie(_(\"Cannot force update the current branch.\"));\n \n@@ -260,8 +266,8 @@ void create_branch(struct repository *r,\n \t\texplicit_tracking = 1;\n \n \tif ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok)\n-\t    ? validate_branchname(name, &ref)\n-\t    : validate_new_branchname(name, &ref, force)) {\n+\t    ? validate_branchname(r, name, &ref)\n+\t    : validate_new_branchname(r, name, &ref, force)) {\n \t\tif (!force)\n \t\t\tdont_change_ref = 1;\n \t\telse\n@@ -269,7 +275,7 @@ void create_branch(struct repository *r,\n \t}\n \n \treal_ref = NULL;\n-\tif (get_oid_mb(start_name, &oid)) {\n+\tif (repo_get_oid_mb(r, start_name, &oid)) {\n \t\tif (explicit_tracking) {\n \t\t\tif (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {\n \t\t\t\terror(_(upstream_missing), start_name);\n@@ -281,7 +287,7 @@ void create_branch(struct repository *r,\n \t\tdie(_(\"Not a valid object name: '%s'.\"), start_name);\n \t}\n \n-\tswitch (dwim_ref(start_name, strlen(start_name), &oid, &real_ref, 0)) {\n+\tswitch (repo_dwim_ref(r, start_name, strlen(start_name), &oid, &real_ref, 0)) {\n \tcase 0:\n \t\t/* Not branching from any existing branch */\n \t\tif (explicit_tracking)\n@@ -319,12 +325,12 @@ void create_branch(struct repository *r,\n \t\telse\n \t\t\tmsg = xstrfmt(\"branch: Created from %s\", start_name);\n \n-\t\ttransaction = ref_transaction_begin(&err);\n+\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(r), &err);\n \t\tif (!transaction ||\n \t\t    ref_transaction_update(transaction, ref.buf,\n \t\t\t\t\t   &oid, forcing ? NULL : null_oid(),\n \t\t\t\t\t   0, msg, &err) ||\n-\t\t    ref_transaction_commit(transaction, &err))\n+\t\t    repo_ref_transaction_commit(r, transaction, &err))\n \t\t\tdie(\"%s\", err.buf);\n \t\tref_transaction_free(transaction);\n \t\tstrbuf_release(&err);\ndiff --git a/branch.h b/branch.h\nindex df0be61506..5fb9ed7e03 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -50,7 +50,7 @@ void create_branch(struct repository *r,\n  * Return 1 if the named branch already exists; return 0 otherwise.\n  * Fill ref with the full refname for the branch.\n  */\n-int validate_branchname(const char *name, struct strbuf *ref);\n+int validate_branchname(struct repository *r, const char *name, struct strbuf *ref);\n \n /*\n  * Check if a branch 'name' can be created as a new branch; die otherwise.\n@@ -58,7 +58,7 @@ int validate_branchname(const char *name, struct strbuf *ref);\n  * Return 1 if the named branch already exists; return 0 otherwise.\n  * Fill ref with the full refname for the branch.\n  */\n-int validate_new_branchname(const char *name, struct strbuf *ref, int force);\n+int validate_new_branchname(struct repository *r, const char *name, struct strbuf *ref, int force);\n \n /*\n  * Remove information about the merge state on the current\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 03c7b7253a..58a730b275 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -24,6 +24,7 @@\n #include \"worktree.h\"\n #include \"help.h\"\n #include \"commit-reach.h\"\n+#include \"submodule-config.h\"\n \n static const char * const builtin_branch_usage[] = {\n \tN_(\"git branch [<options>] [-r | -a] [--merged] [--no-merged]\"),\n@@ -38,6 +39,7 @@ static const char * const builtin_branch_usage[] = {\n \n static const char *head;\n static struct object_id head_oid;\n+static int recurse_submodules = 0;\n \n static int branch_use_color = -1;\n static char branch_colors[][COLOR_MAXLEN] = {\n@@ -100,6 +102,11 @@ static int git_branch_config(const char *var, const char *value, void *cb)\n \t\t\treturn config_error_nonbool(var);\n \t\treturn color_parse(value, branch_colors[slot]);\n \t}\n+\tif (!strcmp(var, \"submodule.recurse\")) {\n+\t\trecurse_submodules = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \treturn git_color_default_config(var, value, cb);\n }\n \n@@ -190,6 +197,50 @@ static void delete_branch_config(const char *branchname)\n \tstrbuf_release(&buf);\n }\n \n+static void create_branches(struct repository *r, const char *name,\n+\t\t\t   const char *start_name, int force,\n+\t\t\t   int clobber_head_ok, int reflog, int quiet,\n+\t\t\t   enum branch_track track)\n+{\n+\tint i = 0;\n+\tstruct repository subrepo;\n+\tconst char *r_start_name;\n+\tconst struct submodule *sub;\n+\n+\tr_start_name = strcmp(start_name, \"HEAD\") ? start_name :\n+\t\trefs_resolve_refdup(get_main_ref_store(r), start_name, 0, NULL, NULL);\n+\tif (strcmp(r_start_name, \"HEAD\"))\n+\t\tskip_prefix(r_start_name, \"refs/heads/\", &r_start_name);\n+\tcreate_branch(r, name, r_start_name, force, clobber_head_ok, reflog,\n+\t\t      quiet, track);\n+\n+\tif (!recurse_submodules) {\n+\t\treturn;\n+\t}\n+\n+\tif (repo_read_index(r) < 0)\n+\t\tdie(_(\"index file corrupt\"));\n+\n+\tfor (i = 0; i < r->index->cache_nr; i++) {\n+\t\tconst struct cache_entry *ce = r->index->cache[i];\n+\t\tif (!S_ISGITLINK(ce->ce_mode))\n+\t\t\tcontinue;\n+\t\tsub = submodule_from_path(r, null_oid(), ce->name);\n+\t\tif (repo_submodule_init(&subrepo, r, sub) < 0) {\n+\t\t\twarning(_(\"Unable to initialize subrepo %s, skipping.\"), ce->name);\n+\t\t\tcontinue;\n+\t\t}\n+\t\t/*\n+\t\t * NEEDSWORK: branch tracking is not supported for\n+\t\t * submodules because it is not possible to get the\n+\t\t * remotes of a submodule.\n+\t\t */\n+\t\tcreate_branches(&subrepo, name, start_name, force,\n+\t\t\t\tclobber_head_ok, reflog, quiet, BRANCH_TRACK_NEVER);\n+\t\trepo_clear(&subrepo);\n+\t}\n+}\n+\n static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t   int quiet)\n {\n@@ -531,9 +582,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t * cause the worktree to become inconsistent with HEAD, so allow it.\n \t */\n \tif (!strcmp(oldname, newname))\n-\t\tvalidate_branchname(newname, &newref);\n+\t\tvalidate_branchname(the_repository, newname, &newref);\n \telse\n-\t\tvalidate_new_branchname(newname, &newref, force);\n+\t\tvalidate_new_branchname(the_repository, newname, &newref, force);\n \n \treject_rebase_or_bisect_branch(oldref.buf);\n \n@@ -626,6 +677,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tint icase = 0;\n \tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n \tstruct ref_format format = REF_FORMAT_INIT;\n+\tint recurse_submodules_explicit = 0;\n \n \tstruct option options[] = {\n \t\tOPT_GROUP(N_(\"Generic options\")),\n@@ -669,6 +721,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tOPT_CALLBACK(0, \"points-at\", &filter.points_at, N_(\"object\"),\n \t\t\tN_(\"print only branches of the object\"), parse_opt_object_name),\n \t\tOPT_BOOL('i', \"ignore-case\", &icase, N_(\"sorting and filtering are case insensitive\")),\n+\t\tOPT_BOOL(0, \"recurse-submodules\", &recurse_submodules_explicit, N_(\"recurse through submodules\")),\n \t\tOPT_STRING(  0 , \"format\", &format.format, N_(\"format\"), N_(\"format to use for the output\")),\n \t\tOPT_END(),\n \t};\n@@ -708,6 +761,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tif (!!delete + !!rename + !!copy + !!new_upstream + !!show_current +\n \t    list + edit_description + unset_upstream > 1)\n \t\tusage_with_options(builtin_branch_usage, options);\n+\telse if (recurse_submodules_explicit &&\n+\t\t (delete || rename || copy || new_upstream || show_current || list\n+\t\t  || edit_description || unset_upstream))\n+\t\tdie(_(\"--recurse-submodules can only be used to create branches\"));\n+\n+\trecurse_submodules |= !!recurse_submodules_explicit;\n \n \tif (filter.abbrev == -1)\n \t\tfilter.abbrev = DEFAULT_ABBREV;\n@@ -857,9 +916,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (track == BRANCH_TRACK_OVERRIDE)\n \t\t\tdie(_(\"the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead.\"));\n \n-\t\tcreate_branch(the_repository,\n-\t\t\t      argv[0], (argc == 2) ? argv[1] : head,\n-\t\t\t      force, 0, reflog, quiet, track);\n+\t\tcreate_branches(the_repository,\n+\t\t\t      argv[0], (argc == 2) ? argv[1] : \"HEAD\",\n+\t\t\t\tforce, 0, reflog, quiet, track);\n \n \t} else\n \t\tusage_with_options(builtin_branch_usage, options);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 8c69dcdf72..8859076a14 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1761,10 +1761,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n \t\tif (opts->new_branch_force)\n-\t\t\topts->branch_exists = validate_branchname(opts->new_branch, &buf);\n+\t\t\topts->branch_exists = validate_branchname(the_repository, opts->new_branch, &buf);\n \t\telse\n \t\t\topts->branch_exists =\n-\t\t\t\tvalidate_new_branchname(opts->new_branch, &buf, 0);\n+\t\t\t\tvalidate_new_branchname(the_repository, opts->new_branch, &buf, 0);\n \t\tstrbuf_release(&buf);\n \t}\n \ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex e575ffb4ff..858831d4cf 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1421,5 +1421,63 @@ test_expect_success 'invalid sort parameter in configuration' '\n \t\ttest_must_fail git branch\n \t)\n '\n+# Submodule tests\n+\n+test_expect_success 'setup superproject and submodule' '\n+\tgit init parent &&\n+\ttest_commit -C parent bar &&\n+\tgit init child &&\n+\ttest_commit -C child bar &&\n+\tgit -C parent submodule add ../child sub &&\n+\tgit -C parent commit -m \"add submodule\"\n+'\n+\n+test_expect_success '--recurse-submodules should create branches' '\n+\t(\n+\t\tcd parent &&\n+\t\tgit branch --recurse-submodules abc &&\n+\t\ttest_path_is_file .git/refs/heads/abc &&\n+\t\ttest_path_is_file .git/modules/sub/refs/heads/abc\n+\t)\n+'\n+\n+test_expect_success '--recurse-submodules should fail when not creating branches' '\n+\t(\n+\t\tcd parent &&\n+\t\ttest_must_fail git branch --recurse-submodules -D abc &&\n+\t\ttest_path_is_file .git/refs/heads/abc &&\n+\t\ttest_path_is_file .git/modules/sub/refs/heads/abc\n+\t)\n+'\n+\n+test_expect_success 'should respect submodule.recurse when creating branches' '\n+\t(\n+\t\tcd parent &&\n+\t\tgit -c submodule.recurse=true branch def &&\n+\t\ttest_path_is_file .git/refs/heads/def &&\n+\t\ttest_path_is_file .git/modules/sub/refs/heads/def\n+\t)\n+'\n+\n+test_expect_success 'should ignore submodule.recurse when not creating branches' '\n+\t(\n+\t\tcd parent &&\n+\t\tgit -c submodule.recurse=true branch -D def &&\n+\t\ttest_path_is_missing .git/refs/heads/def &&\n+\t\ttest_path_is_file .git/modules/sub/refs/heads/def\n+\t)\n+'\n+\n+test_expect_success 'should not set up tracking for new submodule branches' '\n+\t(\n+\t\tcd parent &&\n+\t\tgit -c branch.autoSetupMerge=always branch --recurse-submodules ghi main &&\n+\t\ttest_path_is_file .git/modules/sub/refs/heads/ghi &&\n+\t\ttest \"$(git config branch.ghi.remote)\" = . &&\n+\t\ttest \"$(git config branch.ghi.merge)\" = refs/heads/main &&\n+\t\ttest \"z$(git -C ../child config branch.ghi.remote)\" = z &&\n+\t\ttest \"z$(git -C ../child config branch.ghi.merge)\" = z\n+\t)\n+'\n \n test_done\n-- \n2.33.0.464.g1972c5931b-goog\n\n"},{"id":"436713","messageId":"87k0j87tdw.fsf@evledraar.gmail.com","threadId":"56562","inReplyTo":"20210921232529.81811-3-chooglen@google.com","subject":"Re: [RFC PATCH 2/2] branch: add --recurse-submodules option for branch creation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-22T11:10:11Z","receivedAt":"2021-09-22T11:17:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 21 2021, Glen Choo wrote:\n\n>  static const char *head;\n>  static struct object_id head_oid;\n> +static int recurse_submodules = 0;\n\nNit: just s/ = 0// will do here, and is the convention typically...\n\n> +\tr_start_name = strcmp(start_name, \"HEAD\") ? start_name :\n> +\t\trefs_resolve_refdup(get_main_ref_store(r), start_name, 0, NULL, NULL);\n\nIMO clearer just as an if/else.\n\n> +\tif (strcmp(r_start_name, \"HEAD\"))\n> +\t\tskip_prefix(r_start_name, \"refs/heads/\", &r_start_name);\n\n\n> +\tcreate_branch(r, name, r_start_name, force, clobber_head_ok, reflog,\n> +\t\t      quiet, track);\n> +\n> +\tif (!recurse_submodules) {\n> +\t\treturn;\n> +\t}\n\nCan lose the braces here...\n\n> +\n> +\tif (repo_read_index(r) < 0)\n> +\t\tdie(_(\"index file corrupt\"));\n\n...Just as you do here..\n\n> +\n> +\tfor (i = 0; i < r->index->cache_nr; i++) {\n> +\t\tconst struct cache_entry *ce = r->index->cache[i];\n> +\t\tif (!S_ISGITLINK(ce->ce_mode))\n> +\t\t\tcontinue;\n> +\t\tsub = submodule_from_path(r, null_oid(), ce->name);\n> +\t\tif (repo_submodule_init(&subrepo, r, sub) < 0) {\n> +\t\t\twarning(_(\"Unable to initialize subrepo %s, skipping.\"), ce->name);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\t/*\n> +\t\t * NEEDSWORK: branch tracking is not supported for\n> +\t\t * submodules because it is not possible to get the\n> +\t\t * remotes of a submodule.\n> +\t\t */\n\nIt isn't?\n\n    $ git -C sha1collisiondetection/ remote -v\n    origin  https://github.com/cr-marcstevens/sha1collisiondetection.git\n> [...]\n\n> +test_expect_success 'setup superproject and submodule' '\n> +\tgit init parent &&\n> +\ttest_commit -C parent bar &&\n> +\tgit init child &&\n> +\ttest_commit -C child bar &&\n> +\tgit -C parent submodule add ../child sub &&\n> +\tgit -C parent commit -m \"add submodule\"\n> +'\n> +\n> +test_expect_success '--recurse-submodules should create branches' '\n> +\t(\n> +\t\tcd parent &&\n> +\t\tgit branch --recurse-submodules abc &&\n> +\t\ttest_path_is_file .git/refs/heads/abc &&\n> +\t\ttest_path_is_file .git/modules/sub/refs/heads/abc\n> +\t)\n> +'\n\nAll this manual file checking should depend on REFFILES, but better yet\nis there a reason this can't use rev-parse? I.e. why can't we inpect\nthis state with 'for-each-ref', 'rev-parse' and the like? Does this test\nneed to assert that these files end up in these specific locations, or\njust the ref store? Ditto for the later ones.\n\n> [...]\n> +\t\tcd parent &&\n> +\t\tgit -c branch.autoSetupMerge=always branch --recurse-submodules ghi main &&\n> +\t\ttest_path_is_file .git/modules/sub/refs/heads/ghi &&\n> +\t\ttest \"$(git config branch.ghi.remote)\" = . &&\n> +\t\ttest \"$(git config branch.ghi.merge)\" = refs/heads/main &&\n> +\t\ttest \"z$(git -C ../child config branch.ghi.remote)\" = z &&\n> +\t\ttest \"z$(git -C ../child config branch.ghi.merge)\" = z\n\nUse test_cmp, also for this sort of thing the test \"x$y\" = \"x\" idiom\nisn't needed unless you've got a computer from the mid-90s or something\n:)\n"},{"id":"436725","messageId":"084caed9-5854-9fe2-8476-b5325ddac52c@gmail.com","threadId":"56562","inReplyTo":"20210921232529.81811-3-chooglen@google.com","subject":"Re: [RFC PATCH 2/2] branch: add --recurse-submodules option for branch creation","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-09-22T12:28:57Z","receivedAt":"2021-09-22T12:29:02Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Glen,\n\nLe 2021-09-21 à 19:25, Glen Choo a écrit :\n> When working with a superproject and submodules, it can be helpful to\n> create topic branches to coordinate the work between each repository.\n> \n> Teach cmd_branch to accept the --recurse-submodules option when creating\n> branches. When specified, like\n> \n>    git branch --recurse-submodules topic\n> \n> git-branch will create the \"topic\" branch in the superproject and all\n> submodules.\n> \n> recurse_submodules is only supported for creating a branch. git-branch\n\nsmall nit: we usually don't refer to the dashed form of commands anymore: 'git branch'\nwould be preferable.\n\n> will fail if the user attempts to use --recurse-submodules with anything\n> other than creating a branch. If a user has submodule.recurse in their\n> config, git-branch will use --recurse-submodules for creating a branch,\n> but will treat it as unset for any other operation.\n\nThat seems like a good starting point.\n\nHowever, I think that for this new feature, I would prefer *also* having\na new config 'branch.recurseSubmodules' (or similar) that would allow\nmore granular control than 'submodule.recurse', which influences several\ncommands. Personnally I have 'submodule.recurse' set to true in my '~/.gitconfig',\nbecause I want the submodules working trees to be updated when I use 'git checkout'\nto change branches, 'git grep' to search the working tree, etc., but I usually\ndo not *work* on the submodules in my project and I would not like new branches\nbeing created in them every time I create a new branch in the superproject.\nIn other words, 'branch.recurseSubmodules=false' would have higher priority\nthan 'submodule.recurse=true'.\n\n> \n> There is no easy way to get the remotes of a submodule because remotes.c\n> stores state as static variables. As such, branch tracking is disabled\n> when using --recurse-submodules.\n> \n> Signed-off-by: Glen Choo <chooglen@google.com>\n> ---\n\n\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index e575ffb4ff..858831d4cf 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -1421,5 +1421,63 @@ test_expect_success 'invalid sort parameter in configuration' '\n>   \t\ttest_must_fail git branch\n>   \t)\n>   '\n> +# Submodule tests\n> +\n> \n\nMost tests for submodules are usually in separate test files. I don't think\nthis is a set-in-stone rule, but if more tests are coming in the future, maybe\na new test file t????-branch-submodule.sh would be appropriate ? Just a small suggestion.\n\n\nThanks for working on this! It's exciting to see new developments\nin the submodule area :)\n\nPhilippe.\n"},{"id":"436733","messageId":"kl6lv92s35z3.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"56562","inReplyTo":"87k0j87tdw.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH 2/2] branch: add --recurse-submodules option for branch creation","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-22T16:55:28Z","receivedAt":"2021-09-22T16:55:34Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>>  static const char *head;\n>>  static struct object_id head_oid;\n>> +static int recurse_submodules = 0;\n>\n> Nit: just s/ = 0// will do here, and is the convention typically...\n\nThanks for the catch!\n\n>> +\tr_start_name = strcmp(start_name, \"HEAD\") ? start_name :\n>> +\t\trefs_resolve_refdup(get_main_ref_store(r), start_name, 0, NULL, NULL);\n>\n> IMO clearer just as an if/else.\n\nSounds good, I'll use an if/else instead\n\n>> +\tif (!recurse_submodules) {\n>> +\t\treturn;\n>> +\t}\n>\n> Can lose the braces here...\n\nAh, yes. Old habits die hard.. Thanks!\n\n>> +\n>> +\tfor (i = 0; i < r->index->cache_nr; i++) {\n>> +\t\tconst struct cache_entry *ce = r->index->cache[i];\n>> +\t\tif (!S_ISGITLINK(ce->ce_mode))\n>> +\t\t\tcontinue;\n>> +\t\tsub = submodule_from_path(r, null_oid(), ce->name);\n>> +\t\tif (repo_submodule_init(&subrepo, r, sub) < 0) {\n>> +\t\t\twarning(_(\"Unable to initialize subrepo %s, skipping.\"), ce->name);\n>> +\t\t\tcontinue;\n>> +\t\t}\n>> +\t\t/*\n>> +\t\t * NEEDSWORK: branch tracking is not supported for\n>> +\t\t * submodules because it is not possible to get the\n>> +\t\t * remotes of a submodule.\n>> +\t\t */\n>\n> It isn't?\n>\n>     $ git -C sha1collisiondetection/ remote -v\n>     origin  https://github.com/cr-marcstevens/sha1collisiondetection.git\n\nAh, my comment is ambiguous. I meant that we cannot get\nsubmodule remotes in-process because remotes.c stores its state in\nstatic variables I believe it implicitly refers to the remotes of\nthe_repository and can't be reused for submodules.\n\nOf course I hope I am wrong, because that would make this task a lot\neasier :)\n\n> All this manual file checking should depend on REFFILES, but better yet\n> is there a reason this can't use rev-parse? I.e. why can't we inpect\n> this state with 'for-each-ref', 'rev-parse' and the like? Does this test\n> need to assert that these files end up in these specific locations, or\n> just the ref store? Ditto for the later ones.\n\n> Use test_cmp, also for this sort of thing the test \"x$y\" = \"x\" idiom\n> isn't needed unless you've got a computer from the mid-90s or something\n> :)\n\nThe only reason the tests look this way is that I have copied the\nsurrounding tests. From your comments, it seems clear that these tests\nare fairly out-of-date, so I should probably model them after something\nelse.\n\nI will incorporate Philippe's suggestion\n\n  Most tests for submodules are usually in separate test files. I don't think\n  this is a set-in-stone rule, but if more tests are coming in the future, maybe\n  a new test file t????-branch-submodule.sh would be appropriate ? Just a small suggestion.\n\nThen at least my new tests won't look so out of place with the other\nbranch tests.\n\n>> In this patchset, branching works slightly differently between the\n>> superproject and submodule (skip ahead for specifics). There are two\n>> very obvious alternatives that can address this:\n \n>> * A: only implement --recurse-submodules behavior after we are able to\n>>   eliminate any kind of dependence on the_repository/global state that\n>>   shouldn't be shared.\n>> * B: implement --recurse-submodules as child processes, which won't be\n>>   bothered by global state.\n\nI was wondering if you had thoughts on this bit in particular. It seems\nunpleasant for branching to behave differently between superproject and\nsubmodule, so I'd like to discard this RFC (or at least the 'disable\nremotes' behavior) ASAP and start work on a version that serves us\nbetter.\n"},{"id":"436738","messageId":"kl6lsfxw34m5.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"56562","inReplyTo":"084caed9-5854-9fe2-8476-b5325ddac52c@gmail.com","subject":"Re: [RFC PATCH 2/2] branch: add --recurse-submodules option for branch creation","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-22T17:24:50Z","receivedAt":"2021-09-22T17:24:54Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Philippe Blain <levraiphilippeblain@gmail.com> writes:\n\n>> git-branch will create the \"topic\" branch in the superproject and all\n>> submodules.\n>> \n>> recurse_submodules is only supported for creating a branch. git-branch\n>\n> small nit: we usually don't refer to the dashed form of commands anymore: 'git branch'\n> would be preferable.\n\nAh, thanks! I really appreciate the style pointers.\n\n> However, I think that for this new feature, I would prefer *also* having\n> a new config 'branch.recurseSubmodules' (or similar) that would allow\n> more granular control than 'submodule.recurse', which influences several\n> commands. Personnally I have 'submodule.recurse' set to true in my '~/.gitconfig',\n> because I want the submodules working trees to be updated when I use 'git checkout'\n> to change branches, 'git grep' to search the working tree, etc., but I usually\n> do not *work* on the submodules in my project and I would not like new branches\n> being created in them every time I create a new branch in the superproject.\n> In other words, 'branch.recurseSubmodules=false' would have higher priority\n> than 'submodule.recurse=true'.\n\nThat's a good point. This behavior of git branch --recurse-submodules\nhas the potential to directly conflict with established workflows.\nOpting out via 'branch.recurseSubmodules seems reasonable and I think we\ndefinitely want to give users that fine-grained control. I wonder\nif we should take this even further - perhaps --recurse-submodules is\ntoo disruptive to be controlled by the config value as checkout, fetch,\npull, etc.\n\nPerhaps submodule.recurse should be more fine-grained, so we can ship\ngood defaults for different workflows e.g. options that map to the\ncurrent true/false behavior and another option that includes branching\n(I wish I could give these names, but I have no clue what to name them).\n\n> Most tests for submodules are usually in separate test files. I don't think\n> this is a set-in-stone rule, but if more tests are coming in the future, maybe\n> a new test file t????-branch-submodule.sh would be appropriate ? Just a small suggestion.\n\nI think that's a good call. It also looks like the tests in this file\nare somewhat old, so it would be nice to start on a clean slate :)\n\n>> In this patchset, branching works slightly differently between the\n>> superproject and submodule (skip ahead for specifics). There are two\n>> very obvious alternatives that can address this:\n \n>> * A: only implement --recurse-submodules behavior after we are able to\n>>   eliminate any kind of dependence on the_repository/global state that\n>>   shouldn't be shared.\n>> * B: implement --recurse-submodules as child processes, which won't be\n>>   bothered by global state.\n\nYour comments on the UX make a lot of sense, and I'll think about that\nmore carefully. Let me know if you also have thoughts on the\nimplementation itself, particular on in-process vs child process :)\n"}]}