{"thread":{"id":"56874","subject":"[PATCH v1 0/3] make create_branch() accept any repository","startedAt":"2021-11-11T17:16:57Z","lastAt":"2021-11-15T23:16:11Z","messageCount":7,"participants":["Glen Choo","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"440915","messageId":"20211111171643.13805-1-chooglen@google.com","threadId":"56874","inReplyTo":null,"subject":"[PATCH v1 0/3] make create_branch() accept any repository","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-11-11T17:16:40Z","receivedAt":"2021-11-11T17:16:57Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"create_branch() accepts a struct repository parameter, which seems to\nsuggest that it works with any repository, but it actually only works\nwith the_repository. This series aims to fix this discrepancy.\n\nThis series depends on gc/remote-with-fewer-static-global-variables [1]\nbecause setup_tracking() needs tracking information from\nrepositories other than the_repository.\n\nNote that this fix is not as clean as \"just replace the problematic\nfunctions with an equivalent that doesn't use the_repository\".\n\n* git_config_set() uses the_repository deep in its call chain. Patch 2\n  adds an alternative implementation instead of trying to fix the entire\n  call chain.\n* We cannot check if a non-the_repository is bare (yet). In patch 3,\n  just die instead of trying to perform this check.\n\nWhile this series isn't perfect, I think it is close enough to an\n\"ideal\" removal of the_repository that it is ready for review. My hope\nis to use this series to implement \"git branch --recurse-submodules\"\nin-process. If this series doesn't pass review, I'll implement the\nfeature with child processes instead.\n\n[1] https://lore.kernel.org/git/20211028183101.41013-1-chooglen@google.com/\n\nGlen Choo (3):\n  refs/files-backend: remove the_repository\n  config: introduce repo_config_set* functions\n  branch: remove implicit the_repository from create_branch()\n\n branch.c             | 69 ++++++++++++++++++++++++++++++--------------\n branch.h             |  9 ++++--\n builtin/branch.c     |  5 ++--\n builtin/checkout.c   |  7 +++--\n config.c             | 24 +++++++++++++++\n config.h             | 11 +++++++\n refs/files-backend.c | 20 +++++++------\n 7 files changed, 107 insertions(+), 38 deletions(-)\n\n-- \n2.33.GIT\n\n"},{"id":"440916","messageId":"20211111171643.13805-2-chooglen@google.com","threadId":"56874","inReplyTo":"20211111171643.13805-1-chooglen@google.com","subject":"[PATCH v1 1/3] refs/files-backend: remove the_repository","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-11-11T17:16:41Z","receivedAt":"2021-11-11T17:16:57Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"refs/files-backend.c references the_repository in order to validate\nobject ids. However, 34224e14d6 (refs: plumb repo into ref stores,\n2021-10-08), added repository pointers to ref stores, so we no longer\nneed to hardcode the_repository.\n\nReplace the reference to the_repository with files_ref_store.base.repo.\nThis allows the files backend to work with in-core repositories other\nthan the_repository.\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n refs/files-backend.c | 20 +++++++++++---------\n 1 file changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 4b14f30d48..10eac93cd4 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1353,8 +1353,9 @@ 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-\t\t\t\t const struct object_id *oid, struct strbuf *err);\n+static int write_ref_to_lockfile(struct repository *repo, struct ref_lock *lock,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t 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@@ -1501,7 +1502,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(refs->base.repo, 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@@ -1521,7 +1522,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(refs->base.repo, 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@@ -1756,14 +1757,15 @@ 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-\t\t\t\t const struct object_id *oid, struct strbuf *err)\n+static int write_ref_to_lockfile(struct repository *repo, struct ref_lock *lock,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t 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(repo, oid);\n \tif (!o) {\n \t\tstrbuf_addf(err,\n \t\t\t    \"trying to write ref '%s' with nonexistent object %s\",\n@@ -2576,8 +2578,8 @@ 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\t\t\t\t err)) {\n+\t\t} else if (write_ref_to_lockfile(refs->base.repo, lock,\n+\t\t\t\t\t\t &update->new_oid, err)) {\n \t\t\tchar *write_err = strbuf_detach(err, NULL);\n \n \t\t\t/*\n-- \n2.33.GIT\n\n"},{"id":"440917","messageId":"20211111171643.13805-3-chooglen@google.com","threadId":"56874","inReplyTo":"20211111171643.13805-1-chooglen@google.com","subject":"[PATCH v1 2/3] config: introduce repo_config_set* functions","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-11-11T17:16:42Z","receivedAt":"2021-11-11T17:17:07Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"We have git_config_set() that sets a config value for the_repository's\nconfig file, and repo_config_get* that reads config values from a struct\nrepository. Thus, it seems reasonable to have to have\nrepo_git_config_set() that can set values for a config file of a struct\nrepository.\n\nImplement repo_config_set() and repo_config_set_gently(), which\ntake struct repository. However, unlike other instances where struct\nrepository is added to a repo_* function, this patch does not change the\nimplementations of git_config_set() or git_config_set_gently(); those\nfunctions use the_repository much deeper in their call chain through\ngit_pathdup(). Mark this inconsistency as NEEDSWORK.\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n config.c | 24 ++++++++++++++++++++++++\n config.h | 11 +++++++++++\n 2 files changed, 35 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex cd51efe99a..8dd00b8a13 100644\n--- a/config.c\n+++ b/config.c\n@@ -2869,6 +2869,30 @@ void git_config_set_in_file(const char *config_filename,\n \tgit_config_set_multivar_in_file(config_filename, key, value, NULL, 0);\n }\n \n+/*\n+ * Sets a config value in a repository.\n+ */\n+int repo_config_set_gently(struct repository *r, const char *key,\n+\t\t\t   const char *value)\n+{\n+\tint ret;\n+\tchar *path;\n+\n+\tpath = repo_git_path(r, \"config\");\n+\tret = git_config_set_in_file_gently(path, key, value);\n+\tfree(path);\n+\treturn ret;\n+}\n+\n+void repo_config_set(struct repository *r, const char *key, const char *value)\n+{\n+\tchar *path;\n+\n+\tpath = repo_git_path(r, \"config\");\n+\tgit_config_set_in_file(path, key, value);\n+\tfree(path);\n+}\n+\n int git_config_set_gently(const char *key, const char *value)\n {\n \treturn git_config_set_multivar_gently(key, value, NULL, 0);\ndiff --git a/config.h b/config.h\nindex 69d733824a..4a6919b984 100644\n--- a/config.h\n+++ b/config.h\n@@ -253,6 +253,17 @@ void git_config_set_in_file(const char *, const char *, const char *);\n \n int git_config_set_gently(const char *, const char *);\n \n+/*\n+ * Write config values to a repo's config file.\n+ *\n+ * NEEDSWORK: These are non-the_repository equivalents of\n+ * git_config_set*, but have a completely separate implementation. In\n+ * the ideal case, we git_config_set* should just use the repo_*\n+ * equivalents, just like most other repo_* functions.\n+ */\n+int repo_config_set_gently(struct repository *, const char *, const char *);\n+void repo_config_set(struct repository *, const char *, const char *);\n+\n /**\n  * write config values to `.git/config`, takes a key/value pair as parameter.\n  */\n-- \n2.33.GIT\n\n"},{"id":"440918","messageId":"20211111171643.13805-4-chooglen@google.com","threadId":"56874","inReplyTo":"20211111171643.13805-1-chooglen@google.com","subject":"[PATCH v1 3/3] branch: remove implicit the_repository from create_branch()","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-11-11T17:16:43Z","receivedAt":"2021-11-11T17:17:07Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"create_branch() takes a struct repository parameter, which implies that\nit behaves as expected when called on any repository. However, this is\nnot true because it depends on functions that ultimately use\nthe_repository.\n\nRemove all implicit references to the_repository in create_branch().\nThis is achieved by replacing a function with a variant that accepts\nstruct repository or struct ref_store (typically named repo_* or\nrefs_*). For functions defined in branch.c, add a struct repository\nparameter where necessary.\n\nvalidate_new_branchname() still has an implicit reference to\nthe_repository via is_bare_repository(), which uses environment.c's\nglobal state. Instead of pretending that we can tell if an arbitrary\nrepository is bare, validate_new_branchname() will simply die if r !=\nthe_repository. This shortcoming will only show itself if the user works\nacross multiple repositories in-process and some of them are bare -\nwhich is probably rare in practice.\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n branch.c           | 69 +++++++++++++++++++++++++++++++---------------\n branch.h           |  9 ++++--\n builtin/branch.c   |  5 ++--\n builtin/checkout.c |  7 +++--\n 4 files changed, 61 insertions(+), 29 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 5cc105ff8d..688da607cd 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -5,6 +5,7 @@\n #include \"refs.h\"\n #include \"refspec.h\"\n #include \"remote.h\"\n+#include \"repository.h\"\n #include \"sequencer.h\"\n #include \"commit.h\"\n #include \"worktree.h\"\n@@ -55,7 +56,9 @@ N_(\"\\n\"\n \"the remote tracking information by invoking\\n\"\n \"\\\"git branch --set-upstream-to=%s%s%s\\\".\");\n \n-int install_branch_config(int flag, const char *local, const char *origin, const char *remote)\n+int repo_install_branch_config(struct repository *r, int flag,\n+\t\t\t       const char *local, const char *origin,\n+\t\t\t       const char *remote)\n {\n \tconst char *shortname = NULL;\n \tstruct strbuf key = STRBUF_INIT;\n@@ -70,18 +73,18 @@ int install_branch_config(int flag, const char *local, const char *origin, const\n \t}\n \n \tstrbuf_addf(&key, \"branch.%s.remote\", local);\n-\tif (git_config_set_gently(key.buf, origin ? origin : \".\") < 0)\n+\tif (repo_config_set_gently(r, key.buf, origin ? origin : \".\") < 0)\n \t\tgoto out_err;\n \n \tstrbuf_reset(&key);\n \tstrbuf_addf(&key, \"branch.%s.merge\", local);\n-\tif (git_config_set_gently(key.buf, remote) < 0)\n+\tif (repo_config_set_gently(r, key.buf, remote) < 0)\n \t\tgoto out_err;\n \n \tif (rebasing) {\n \t\tstrbuf_reset(&key);\n \t\tstrbuf_addf(&key, \"branch.%s.rebase\", local);\n-\t\tif (git_config_set_gently(key.buf, \"true\") < 0)\n+\t\tif (repo_config_set_gently(r, key.buf, \"true\") < 0)\n \t\t\tgoto out_err;\n \t}\n \tstrbuf_release(&key);\n@@ -126,6 +129,13 @@ int install_branch_config(int flag, const char *local, const char *origin, const\n \treturn -1;\n }\n \n+int install_branch_config(int flag, const char *local, const char *origin,\n+\t\t\t  const char *remote)\n+{\n+\treturn repo_install_branch_config(the_repository, flag, local, origin,\n+\t\t\t\t\t  remote);\n+}\n+\n static int inherit_tracking(struct tracking *tracking, const char *orig_ref)\n {\n \tstruct strbuf key = STRBUF_INIT;\n@@ -164,8 +174,9 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)\n  * to infer the settings for branch.<new_ref>.{remote,merge} from the\n  * config.\n  */\n-static void setup_tracking(const char *new_ref, const char *orig_ref,\n-\t\t\t   enum branch_track track, int quiet)\n+static void setup_tracking(struct repository *r, const char *new_ref,\n+\t\t\t   const char *orig_ref, enum branch_track track,\n+\t\t\t   int quiet)\n {\n \tstruct tracking tracking;\n \tint config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n@@ -173,7 +184,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \tmemset(&tracking, 0, sizeof(tracking));\n \ttracking.spec.dst = (char *)orig_ref;\n \tif (track != BRANCH_TRACK_INHERIT) {\n-\t\tfor_each_remote(find_tracked_branch, &tracking);\n+\t\trepo_for_each_remote(r, find_tracked_branch, &tracking);\n \t} else if (inherit_tracking(&tracking, orig_ref))\n \t\treturn;\n \n@@ -191,8 +202,9 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \t\tdie(_(\"Not tracking: ambiguous information for ref %s\"),\n \t\t    orig_ref);\n \n-\tif (install_branch_config(config_flags, new_ref, tracking.remote,\n-\t\t\t      tracking.src ? tracking.src : orig_ref) < 0)\n+\tif (repo_install_branch_config(\n+\t\t    r, config_flags, new_ref, tracking.remote,\n+\t\t    tracking.src ? tracking.src : orig_ref) < 0)\n \t\texit(-1);\n \n \tfree(tracking.src);\n@@ -218,12 +230,13 @@ 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,\n+\t\t\tstruct 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@@ -232,19 +245,31 @@ 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,\n+\t\t\t    struct strbuf *ref, int force)\n {\n \tconst char *head;\n+\tint ignore_errno;\n \n-\tif (!validate_branchname(name, ref))\n+\tif (!validate_branchname(r, name, ref))\n \t\treturn 0;\n \n \tif (!force)\n \t\tdie(_(\"A branch named '%s' already exists.\"),\n \t\t    ref->buf + strlen(\"refs/heads/\"));\n \n-\thead = resolve_ref_unsafe(\"HEAD\", 0, NULL, NULL);\n-\tif (!is_bare_repository() && head && !strcmp(head, ref->buf))\n+\thead = refs_resolve_ref_unsafe(get_main_ref_store(r), \"HEAD\", 0, NULL,\n+\t\t\t\t       NULL, &ignore_errno);\n+\n+\t/*\n+\t * If we would move the repository head, allow the move only if\n+\t * the repository is bare.\n+\t *\n+\t * NEEDSWORK because we don't have an easy way to check if a\n+\t * non-the_repository is bare, fail if r is not the_repository.\n+\t */\n+\tif (head && !strcmp(head, ref->buf) &&\n+\t    (r != the_repository || !is_bare_repository()))\n \t\tdie(_(\"Cannot force update the current branch.\"));\n \n \treturn 1;\n@@ -294,9 +319,9 @@ void create_branch(struct repository *r,\n \tif (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)\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+\tif ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok) ?\n+\t\t\t  validate_branchname(r, name, &ref) :\n+\t\t\t  validate_new_branchname(r, name, &ref, force)) {\n \t\tif (!force)\n \t\t\tdont_change_ref = 1;\n \t\telse\n@@ -304,7 +329,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@@ -316,7 +341,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@@ -354,7 +379,7 @@ 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@@ -367,7 +392,7 @@ void create_branch(struct repository *r,\n \t}\n \n \tif (real_ref && track)\n-\t\tsetup_tracking(ref.buf + 11, real_ref, track, quiet);\n+\t\tsetup_tracking(r, ref.buf + 11, real_ref, track, quiet);\n \n \tstrbuf_release(&ref);\n \tfree(real_ref);\ndiff --git a/branch.h b/branch.h\nindex 6484bda8a2..0177528304 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -51,7 +51,8 @@ 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,\n+\t\t\tstruct strbuf *ref);\n \n /*\n  * Check if a branch 'name' can be created as a new branch; die otherwise.\n@@ -59,7 +60,8 @@ 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,\n+\t\t\t    struct strbuf *ref, int force);\n \n /*\n  * Remove information about the merge state on the current\n@@ -79,6 +81,9 @@ void remove_branch_state(struct repository *r, int verbose);\n  * Returns 0 on success.\n  */\n #define BRANCH_CONFIG_VERBOSE 01\n+int repo_install_branch_config(struct repository *r, int flag,\n+\t\t\t       const char *local, const char *origin,\n+\t\t\t       const char *remote);\n int install_branch_config(int flag, const char *local, const char *origin, const char *remote);\n \n /*\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 1fb4b57ca9..46fe7cb634 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -531,9 +531,10 @@ 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,\n+\t\t\t\t\tforce);\n \n \treject_rebase_or_bisect_branch(oldref.buf);\n \ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2b9501520c..c98ba384dd 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1762,10 +1762,11 @@ 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(\n+\t\t\t\tthe_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\topts->branch_exists = validate_new_branchname(\n+\t\t\t\tthe_repository, opts->new_branch, &buf, 0);\n \t\tstrbuf_release(&buf);\n \t}\n \n-- \n2.33.GIT\n\n"},{"id":"440935","messageId":"xmqq7ddel9te.fsf@gitster.g","threadId":"56874","inReplyTo":"20211111171643.13805-3-chooglen@google.com","subject":"Re: [PATCH v1 2/3] config: introduce repo_config_set* functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-11T20:24:45Z","receivedAt":"2021-11-11T20:24:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo <chooglen@google.com> writes:\n\n> We have git_config_set() that sets a config value for the_repository's\n> config file, and repo_config_get* that reads config values from a struct\n> repository. Thus, it seems reasonable to have to have\n> repo_git_config_set() that can set values for a config file of a struct\n> repository.\n>\n> Implement repo_config_set() and repo_config_set_gently(), which\n> take struct repository. However, unlike other instances where struct\n> repository is added to a repo_* function, this patch does not change the\n> implementations of git_config_set() or git_config_set_gently(); those\n> functions use the_repository much deeper in their call chain through\n> git_pathdup(). Mark this inconsistency as NEEDSWORK.\n\nBeing able to only affect \"config\" in the_repository->gitdir is less\nflexible than being able to affect \"config\" in repo->gitdir for any\nrepository is good.  Do we need a similar thing for repo->commondir\nas well?\n\n> +/*\n> + * Sets a config value in a repository.\n> + */\n> +int repo_config_set_gently(struct repository *r, const char *key,\n> +\t\t\t   const char *value)\n> +{\n> +\tint ret;\n> +\tchar *path;\n> +\n> +\tpath = repo_git_path(r, \"config\");\n> +\tret = git_config_set_in_file_gently(path, key, value);\n> +\tfree(path);\n> +\treturn ret;\n> +}\n> +\n> +void repo_config_set(struct repository *r, const char *key, const char *value)\n> +{\n> +\tchar *path;\n> +\n> +\tpath = repo_git_path(r, \"config\");\n> +\tgit_config_set_in_file(path, key, value);\n> +\tfree(path);\n> +}\n\nMany questions:\n\n - What do these do for an existing key?  Add another value?\n   Replace existing one?  If the latter, what do we plan to do with\n   multi-valued keys?\n\n - Don't we need an interface to remove, rename, etc.?\n\n - If we call repo_config_set(repo, \"key\", \"value\") and then call\n   repo_git_config_string(repo, \"key\", &value) on the same\n   repository, do we read the value back or do we give a stale\n   value?\n\n - A change like this should make existing config_set() that only\n   works on the_repository into a thin wrapper, e.g.\n\n\tvoid git_config_set(const char *keyu, const char **value)\n\t{\n\t\trepo_config_set(the_repository, key, value);\n\t}\n\n   But that is not happening here.  What prevents us from doing so?\n\nThanks.\n"},{"id":"440951","messageId":"kl6lv90ytd4v.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"56874","inReplyTo":"xmqq7ddel9te.fsf@gitster.g","subject":"Re: [PATCH v1 2/3] config: introduce repo_config_set* functions","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-11-12T00:45:52Z","receivedAt":"2021-11-12T00:45:57Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Glen Choo <chooglen@google.com> writes:\n>\n>> We have git_config_set() that sets a config value for the_repository's\n>> config file, and repo_config_get* that reads config values from a struct\n>> repository. Thus, it seems reasonable to have to have\n>> repo_git_config_set() that can set values for a config file of a struct\n>> repository.\n>>\n>> Implement repo_config_set() and repo_config_set_gently(), which\n>> take struct repository. However, unlike other instances where struct\n>> repository is added to a repo_* function, this patch does not change the\n>> implementations of git_config_set() or git_config_set_gently(); those\n>> functions use the_repository much deeper in their call chain through\n>> git_pathdup(). Mark this inconsistency as NEEDSWORK.\n>\n> Being able to only affect \"config\" in the_repository->gitdir is less\n> flexible than being able to affect \"config\" in repo->gitdir for any\n> repository is good.  Do we need a similar thing for repo->commondir\n> as well?\n\ngit_config_set() can only affect repo->gitdir, so from the perspective\nof \"what should a repo_* variant of git_config_set() do\", then no, we do\nnot need a similar thing. As far as I can tell, config.c only works with\nthe gitdir, not the commondir.\n\nI cannot comment on whether we will ever need to set config values in\nthe commondir.\n\n> Many questions:\n>\n>  - What do these do for an existing key?  Add another value?\n>    Replace existing one?  If the latter, what do we plan to do with\n>    multi-valued keys?\n\nFor an existing key, this should replace the first instance found.\nThis eventually calls of git_config_set_multivar_in_file_gently() with\nvalue_pattern=NULL, flags = 0. According to the comments:\n\n * if flags contains the CONFIG_FLAGS_MULTI_REPLACE flag, all matching\n *     key/values are removed before a single new pair is written. If the\n *     flag is not present, then replace only the first match.\n\nWe pass flags=0, so only the first instance is replaced.\n\n * - [value_pattern] as a string. It will disregard key/value pairs where value\n *   does not match.\n\nWe pass value_pattern=NULL, so we consider all instances.\n\n>  - Don't we need an interface to remove, rename, etc.?\n\nThis function supports 'remove' by passing value=NULL. By rename, I\nbelieve you're referring to renaming a config section, e.g. a repo_*\nversion of git_config_copy_or_rename_section_in_file()?\n\nI think this is warranted if we perform a full plumbing of struct\nrepository through config.c. But I think it would be prudent to do this\nplumbing piecemeal - perhaps starting with \"set\", and then proceeding to\nthe other operations. \n\n>  - If we call repo_config_set(repo, \"key\", \"value\") and then call\n>    repo_git_config_string(repo, \"key\", &value) on the same\n>    repository, do we read the value back or do we give a stale\n>    value?\n\nWe read the correct value if repo == the_repository but we do not if r\n!= the_repository. Thanks for spotting this bug.\n\nI believe your concern comes from the fact that struct repository\ncaches config values in repo->config and thus we are not guaranteed to\nread the value back from the file. Following this train of thought, we\ncan see that git_config_set_multivar_in_file_gently() clears the cache\nfor the_repository, by calling git_config_clear(). Because this is\nhardcoded to the_repository, git_config_set_multivar_in_file_gently()\ncannot be safely called from repo_config_set() and my implementation is\nbuggy.\n\n>  - A change like this should make existing config_set() that only\n>    works on the_repository into a thin wrapper, e.g.\n>\n> \tvoid git_config_set(const char *keyu, const char **value)\n> \t{\n> \t\trepo_config_set(the_repository, key, value);\n> \t}\n>\n>    But that is not happening here.  What prevents us from doing so?\n\nI think it is _possible_, as long as we plumb struct repository through\nthe call chain all the way to git_config_set_multivar_in_file_gently().\n\nI didn't do so because I thought that I had an implementation of\nrepo_config_set() without introducing a major overhaul of config.c.\nBecause it is an alternative implementation, I decided not to replace\nthe existing one.\n\nHowever, as noted above, this alternative implementation is wrong\nbecause git_config_set_multivar_in_file_gently() makes use of\nthe_repository (i.e. I didn't read the function carefully enough).\n\nI will attempt this plumbing, which will allow us to make\ngit_config_set() a thin wrapper. However, if this turns out to be too\ndifficult, I will implement branch --recurse-submodules with child\nprocesses and leave this for a future clean up (as hinted at in the\ncover letter).\n"},{"id":"441174","messageId":"kl6lczn1jc6j.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"56874","inReplyTo":"kl6lv90ytd4v.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v1 2/3] config: introduce repo_config_set* functions","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-11-15T22:17:56Z","receivedAt":"2021-11-15T23:16:11Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Glen Choo <chooglen@google.com> writes:\n\n>>  - A change like this should make existing config_set() that only\n>>    works on the_repository into a thin wrapper, e.g.\n>>\n>> \tvoid git_config_set(const char *keyu, const char **value)\n>> \t{\n>> \t\trepo_config_set(the_repository, key, value);\n>> \t}\n>>\n>>    But that is not happening here.  What prevents us from doing so?\n>\n> I think it is _possible_, as long as we plumb struct repository through\n> the call chain all the way to git_config_set_multivar_in_file_gently().\n>\n> [...]\n>\n> I will attempt this plumbing, which will allow us to make\n> git_config_set() a thin wrapper. However, if this turns out to be too\n> difficult, I will implement branch --recurse-submodules with child\n> processes and leave this for a future clean up (as hinted at in the\n> cover letter).\n\nI believe that this plumbing is already doable but it would take an\nextensive overhaul of config.c as well as some cleanup of path.c.\n\nThis is more work than I am willing to take on right now, but I'll\nrevisit this at some point.\n"}]}