* [PATCH v1 0/3] make create_branch() accept any repository
@ 2021-11-11 17:16 Glen Choo
2021-11-11 17:16 ` [PATCH v1 1/3] refs/files-backend: remove the_repository Glen Choo
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Glen Choo @ 2021-11-11 17:16 UTC (permalink / raw)
To: git; +Cc: Glen Choo
create_branch() accepts a struct repository parameter, which seems to
suggest that it works with any repository, but it actually only works
with the_repository. This series aims to fix this discrepancy.
This series depends on gc/remote-with-fewer-static-global-variables [1]
because setup_tracking() needs tracking information from
repositories other than the_repository.
Note that this fix is not as clean as "just replace the problematic
functions with an equivalent that doesn't use the_repository".
* git_config_set() uses the_repository deep in its call chain. Patch 2
adds an alternative implementation instead of trying to fix the entire
call chain.
* We cannot check if a non-the_repository is bare (yet). In patch 3,
just die instead of trying to perform this check.
While this series isn't perfect, I think it is close enough to an
"ideal" removal of the_repository that it is ready for review. My hope
is to use this series to implement "git branch --recurse-submodules"
in-process. If this series doesn't pass review, I'll implement the
feature with child processes instead.
[1] https://lore.kernel.org/git/20211028183101.41013-1-chooglen@google.com/
Glen Choo (3):
refs/files-backend: remove the_repository
config: introduce repo_config_set* functions
branch: remove implicit the_repository from create_branch()
branch.c | 69 ++++++++++++++++++++++++++++++--------------
branch.h | 9 ++++--
builtin/branch.c | 5 ++--
builtin/checkout.c | 7 +++--
config.c | 24 +++++++++++++++
config.h | 11 +++++++
refs/files-backend.c | 20 +++++++------
7 files changed, 107 insertions(+), 38 deletions(-)
--
2.33.GIT
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v1 1/3] refs/files-backend: remove the_repository
2021-11-11 17:16 [PATCH v1 0/3] make create_branch() accept any repository Glen Choo
@ 2021-11-11 17:16 ` Glen Choo
2021-11-11 17:16 ` [PATCH v1 2/3] config: introduce repo_config_set* functions Glen Choo
2021-11-11 17:16 ` [PATCH v1 3/3] branch: remove implicit the_repository from create_branch() Glen Choo
2 siblings, 0 replies; 7+ messages in thread
From: Glen Choo @ 2021-11-11 17:16 UTC (permalink / raw)
To: git; +Cc: Glen Choo
refs/files-backend.c references the_repository in order to validate
object ids. However, 34224e14d6 (refs: plumb repo into ref stores,
2021-10-08), added repository pointers to ref stores, so we no longer
need to hardcode the_repository.
Replace the reference to the_repository with files_ref_store.base.repo.
This allows the files backend to work with in-core repositories other
than the_repository.
Signed-off-by: Glen Choo <chooglen@google.com>
---
refs/files-backend.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4b14f30d48..10eac93cd4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1353,8 +1353,9 @@ static int rename_tmp_log(struct files_ref_store *refs, const char *newrefname)
return ret;
}
-static int write_ref_to_lockfile(struct ref_lock *lock,
- const struct object_id *oid, struct strbuf *err);
+static int write_ref_to_lockfile(struct repository *repo, struct ref_lock *lock,
+ const struct object_id *oid,
+ struct strbuf *err);
static int commit_ref_update(struct files_ref_store *refs,
struct ref_lock *lock,
const struct object_id *oid, const char *logmsg,
@@ -1501,7 +1502,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
}
oidcpy(&lock->old_oid, &orig_oid);
- if (write_ref_to_lockfile(lock, &orig_oid, &err) ||
+ if (write_ref_to_lockfile(refs->base.repo, lock, &orig_oid, &err) ||
commit_ref_update(refs, lock, &orig_oid, logmsg, &err)) {
error("unable to write current sha1 into %s: %s", newrefname, err.buf);
strbuf_release(&err);
@@ -1521,7 +1522,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
flag = log_all_ref_updates;
log_all_ref_updates = LOG_REFS_NONE;
- if (write_ref_to_lockfile(lock, &orig_oid, &err) ||
+ if (write_ref_to_lockfile(refs->base.repo, lock, &orig_oid, &err) ||
commit_ref_update(refs, lock, &orig_oid, NULL, &err)) {
error("unable to write current sha1 into %s: %s", oldrefname, err.buf);
strbuf_release(&err);
@@ -1756,14 +1757,15 @@ static int files_log_ref_write(struct files_ref_store *refs,
* Write oid into the open lockfile, then close the lockfile. On
* errors, rollback the lockfile, fill in *err and return -1.
*/
-static int write_ref_to_lockfile(struct ref_lock *lock,
- const struct object_id *oid, struct strbuf *err)
+static int write_ref_to_lockfile(struct repository *repo, struct ref_lock *lock,
+ const struct object_id *oid,
+ struct strbuf *err)
{
static char term = '\n';
struct object *o;
int fd;
- o = parse_object(the_repository, oid);
+ o = parse_object(repo, oid);
if (!o) {
strbuf_addf(err,
"trying to write ref '%s' with nonexistent object %s",
@@ -2576,8 +2578,8 @@ static int lock_ref_for_update(struct files_ref_store *refs,
* The reference already has the desired
* value, so we don't need to write it.
*/
- } else if (write_ref_to_lockfile(lock, &update->new_oid,
- err)) {
+ } else if (write_ref_to_lockfile(refs->base.repo, lock,
+ &update->new_oid, err)) {
char *write_err = strbuf_detach(err, NULL);
/*
--
2.33.GIT
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v1 2/3] config: introduce repo_config_set* functions
2021-11-11 17:16 [PATCH v1 0/3] make create_branch() accept any repository Glen Choo
2021-11-11 17:16 ` [PATCH v1 1/3] refs/files-backend: remove the_repository Glen Choo
@ 2021-11-11 17:16 ` Glen Choo
2021-11-11 20:24 ` Junio C Hamano
2021-11-11 17:16 ` [PATCH v1 3/3] branch: remove implicit the_repository from create_branch() Glen Choo
2 siblings, 1 reply; 7+ messages in thread
From: Glen Choo @ 2021-11-11 17:16 UTC (permalink / raw)
To: git; +Cc: Glen Choo
We have git_config_set() that sets a config value for the_repository's
config file, and repo_config_get* that reads config values from a struct
repository. Thus, it seems reasonable to have to have
repo_git_config_set() that can set values for a config file of a struct
repository.
Implement repo_config_set() and repo_config_set_gently(), which
take struct repository. However, unlike other instances where struct
repository is added to a repo_* function, this patch does not change the
implementations of git_config_set() or git_config_set_gently(); those
functions use the_repository much deeper in their call chain through
git_pathdup(). Mark this inconsistency as NEEDSWORK.
Signed-off-by: Glen Choo <chooglen@google.com>
---
config.c | 24 ++++++++++++++++++++++++
config.h | 11 +++++++++++
2 files changed, 35 insertions(+)
diff --git a/config.c b/config.c
index cd51efe99a..8dd00b8a13 100644
--- a/config.c
+++ b/config.c
@@ -2869,6 +2869,30 @@ void git_config_set_in_file(const char *config_filename,
git_config_set_multivar_in_file(config_filename, key, value, NULL, 0);
}
+/*
+ * Sets a config value in a repository.
+ */
+int repo_config_set_gently(struct repository *r, const char *key,
+ const char *value)
+{
+ int ret;
+ char *path;
+
+ path = repo_git_path(r, "config");
+ ret = git_config_set_in_file_gently(path, key, value);
+ free(path);
+ return ret;
+}
+
+void repo_config_set(struct repository *r, const char *key, const char *value)
+{
+ char *path;
+
+ path = repo_git_path(r, "config");
+ git_config_set_in_file(path, key, value);
+ free(path);
+}
+
int git_config_set_gently(const char *key, const char *value)
{
return git_config_set_multivar_gently(key, value, NULL, 0);
diff --git a/config.h b/config.h
index 69d733824a..4a6919b984 100644
--- a/config.h
+++ b/config.h
@@ -253,6 +253,17 @@ void git_config_set_in_file(const char *, const char *, const char *);
int git_config_set_gently(const char *, const char *);
+/*
+ * Write config values to a repo's config file.
+ *
+ * NEEDSWORK: These are non-the_repository equivalents of
+ * git_config_set*, but have a completely separate implementation. In
+ * the ideal case, we git_config_set* should just use the repo_*
+ * equivalents, just like most other repo_* functions.
+ */
+int repo_config_set_gently(struct repository *, const char *, const char *);
+void repo_config_set(struct repository *, const char *, const char *);
+
/**
* write config values to `.git/config`, takes a key/value pair as parameter.
*/
--
2.33.GIT
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v1 3/3] branch: remove implicit the_repository from create_branch()
2021-11-11 17:16 [PATCH v1 0/3] make create_branch() accept any repository Glen Choo
2021-11-11 17:16 ` [PATCH v1 1/3] refs/files-backend: remove the_repository Glen Choo
2021-11-11 17:16 ` [PATCH v1 2/3] config: introduce repo_config_set* functions Glen Choo
@ 2021-11-11 17:16 ` Glen Choo
2 siblings, 0 replies; 7+ messages in thread
From: Glen Choo @ 2021-11-11 17:16 UTC (permalink / raw)
To: git; +Cc: Glen Choo
create_branch() takes a struct repository parameter, which implies that
it behaves as expected when called on any repository. However, this is
not true because it depends on functions that ultimately use
the_repository.
Remove all implicit references to the_repository in create_branch().
This is achieved by replacing a function with a variant that accepts
struct repository or struct ref_store (typically named repo_* or
refs_*). For functions defined in branch.c, add a struct repository
parameter where necessary.
validate_new_branchname() still has an implicit reference to
the_repository via is_bare_repository(), which uses environment.c's
global state. Instead of pretending that we can tell if an arbitrary
repository is bare, validate_new_branchname() will simply die if r !=
the_repository. This shortcoming will only show itself if the user works
across multiple repositories in-process and some of them are bare -
which is probably rare in practice.
Signed-off-by: Glen Choo <chooglen@google.com>
---
branch.c | 69 +++++++++++++++++++++++++++++++---------------
branch.h | 9 ++++--
builtin/branch.c | 5 ++--
builtin/checkout.c | 7 +++--
4 files changed, 61 insertions(+), 29 deletions(-)
diff --git a/branch.c b/branch.c
index 5cc105ff8d..688da607cd 100644
--- a/branch.c
+++ b/branch.c
@@ -5,6 +5,7 @@
#include "refs.h"
#include "refspec.h"
#include "remote.h"
+#include "repository.h"
#include "sequencer.h"
#include "commit.h"
#include "worktree.h"
@@ -55,7 +56,9 @@ N_("\n"
"the remote tracking information by invoking\n"
"\"git branch --set-upstream-to=%s%s%s\".");
-int install_branch_config(int flag, const char *local, const char *origin, const char *remote)
+int repo_install_branch_config(struct repository *r, int flag,
+ const char *local, const char *origin,
+ const char *remote)
{
const char *shortname = NULL;
struct strbuf key = STRBUF_INIT;
@@ -70,18 +73,18 @@ int install_branch_config(int flag, const char *local, const char *origin, const
}
strbuf_addf(&key, "branch.%s.remote", local);
- if (git_config_set_gently(key.buf, origin ? origin : ".") < 0)
+ if (repo_config_set_gently(r, key.buf, origin ? origin : ".") < 0)
goto out_err;
strbuf_reset(&key);
strbuf_addf(&key, "branch.%s.merge", local);
- if (git_config_set_gently(key.buf, remote) < 0)
+ if (repo_config_set_gently(r, key.buf, remote) < 0)
goto out_err;
if (rebasing) {
strbuf_reset(&key);
strbuf_addf(&key, "branch.%s.rebase", local);
- if (git_config_set_gently(key.buf, "true") < 0)
+ if (repo_config_set_gently(r, key.buf, "true") < 0)
goto out_err;
}
strbuf_release(&key);
@@ -126,6 +129,13 @@ int install_branch_config(int flag, const char *local, const char *origin, const
return -1;
}
+int install_branch_config(int flag, const char *local, const char *origin,
+ const char *remote)
+{
+ return repo_install_branch_config(the_repository, flag, local, origin,
+ remote);
+}
+
static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
{
struct strbuf key = STRBUF_INIT;
@@ -164,8 +174,9 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
* to infer the settings for branch.<new_ref>.{remote,merge} from the
* config.
*/
-static void setup_tracking(const char *new_ref, const char *orig_ref,
- enum branch_track track, int quiet)
+static void setup_tracking(struct repository *r, const char *new_ref,
+ const char *orig_ref, enum branch_track track,
+ int quiet)
{
struct tracking tracking;
int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
@@ -173,7 +184,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
memset(&tracking, 0, sizeof(tracking));
tracking.spec.dst = (char *)orig_ref;
if (track != BRANCH_TRACK_INHERIT) {
- for_each_remote(find_tracked_branch, &tracking);
+ repo_for_each_remote(r, find_tracked_branch, &tracking);
} else if (inherit_tracking(&tracking, orig_ref))
return;
@@ -191,8 +202,9 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
die(_("Not tracking: ambiguous information for ref %s"),
orig_ref);
- if (install_branch_config(config_flags, new_ref, tracking.remote,
- tracking.src ? tracking.src : orig_ref) < 0)
+ if (repo_install_branch_config(
+ r, config_flags, new_ref, tracking.remote,
+ tracking.src ? tracking.src : orig_ref) < 0)
exit(-1);
free(tracking.src);
@@ -218,12 +230,13 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
* Return 1 if the named branch already exists; return 0 otherwise.
* Fill ref with the full refname for the branch.
*/
-int validate_branchname(const char *name, struct strbuf *ref)
+int validate_branchname(struct repository *r, const char *name,
+ struct strbuf *ref)
{
if (strbuf_check_branch_ref(ref, name))
die(_("'%s' is not a valid branch name."), name);
- return ref_exists(ref->buf);
+ return refs_ref_exists(get_main_ref_store(r), ref->buf);
}
/*
@@ -232,19 +245,31 @@ int validate_branchname(const char *name, struct strbuf *ref)
* Return 1 if the named branch already exists; return 0 otherwise.
* Fill ref with the full refname for the branch.
*/
-int validate_new_branchname(const char *name, struct strbuf *ref, int force)
+int validate_new_branchname(struct repository *r, const char *name,
+ struct strbuf *ref, int force)
{
const char *head;
+ int ignore_errno;
- if (!validate_branchname(name, ref))
+ if (!validate_branchname(r, name, ref))
return 0;
if (!force)
die(_("A branch named '%s' already exists."),
ref->buf + strlen("refs/heads/"));
- head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
- if (!is_bare_repository() && head && !strcmp(head, ref->buf))
+ head = refs_resolve_ref_unsafe(get_main_ref_store(r), "HEAD", 0, NULL,
+ NULL, &ignore_errno);
+
+ /*
+ * If we would move the repository head, allow the move only if
+ * the repository is bare.
+ *
+ * NEEDSWORK because we don't have an easy way to check if a
+ * non-the_repository is bare, fail if r is not the_repository.
+ */
+ if (head && !strcmp(head, ref->buf) &&
+ (r != the_repository || !is_bare_repository()))
die(_("Cannot force update the current branch."));
return 1;
@@ -294,9 +319,9 @@ void create_branch(struct repository *r,
if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
explicit_tracking = 1;
- if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok)
- ? validate_branchname(name, &ref)
- : validate_new_branchname(name, &ref, force)) {
+ if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok) ?
+ validate_branchname(r, name, &ref) :
+ validate_new_branchname(r, name, &ref, force)) {
if (!force)
dont_change_ref = 1;
else
@@ -304,7 +329,7 @@ void create_branch(struct repository *r,
}
real_ref = NULL;
- if (get_oid_mb(start_name, &oid)) {
+ if (repo_get_oid_mb(r, start_name, &oid)) {
if (explicit_tracking) {
if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {
error(_(upstream_missing), start_name);
@@ -316,7 +341,7 @@ void create_branch(struct repository *r,
die(_("Not a valid object name: '%s'."), start_name);
}
- switch (dwim_ref(start_name, strlen(start_name), &oid, &real_ref, 0)) {
+ switch (repo_dwim_ref(r, start_name, strlen(start_name), &oid, &real_ref, 0)) {
case 0:
/* Not branching from any existing branch */
if (explicit_tracking)
@@ -354,7 +379,7 @@ void create_branch(struct repository *r,
else
msg = xstrfmt("branch: Created from %s", start_name);
- transaction = ref_transaction_begin(&err);
+ transaction = ref_store_transaction_begin(get_main_ref_store(r), &err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf,
&oid, forcing ? NULL : null_oid(),
@@ -367,7 +392,7 @@ void create_branch(struct repository *r,
}
if (real_ref && track)
- setup_tracking(ref.buf + 11, real_ref, track, quiet);
+ setup_tracking(r, ref.buf + 11, real_ref, track, quiet);
strbuf_release(&ref);
free(real_ref);
diff --git a/branch.h b/branch.h
index 6484bda8a2..0177528304 100644
--- a/branch.h
+++ b/branch.h
@@ -51,7 +51,8 @@ void create_branch(struct repository *r,
* Return 1 if the named branch already exists; return 0 otherwise.
* Fill ref with the full refname for the branch.
*/
-int validate_branchname(const char *name, struct strbuf *ref);
+int validate_branchname(struct repository *r, const char *name,
+ struct strbuf *ref);
/*
* Check if a branch 'name' can be created as a new branch; die otherwise.
@@ -59,7 +60,8 @@ int validate_branchname(const char *name, struct strbuf *ref);
* Return 1 if the named branch already exists; return 0 otherwise.
* Fill ref with the full refname for the branch.
*/
-int validate_new_branchname(const char *name, struct strbuf *ref, int force);
+int validate_new_branchname(struct repository *r, const char *name,
+ struct strbuf *ref, int force);
/*
* Remove information about the merge state on the current
@@ -79,6 +81,9 @@ void remove_branch_state(struct repository *r, int verbose);
* Returns 0 on success.
*/
#define BRANCH_CONFIG_VERBOSE 01
+int repo_install_branch_config(struct repository *r, int flag,
+ const char *local, const char *origin,
+ const char *remote);
int install_branch_config(int flag, const char *local, const char *origin, const char *remote);
/*
diff --git a/builtin/branch.c b/builtin/branch.c
index 1fb4b57ca9..46fe7cb634 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -531,9 +531,10 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
* cause the worktree to become inconsistent with HEAD, so allow it.
*/
if (!strcmp(oldname, newname))
- validate_branchname(newname, &newref);
+ validate_branchname(the_repository, newname, &newref);
else
- validate_new_branchname(newname, &newref, force);
+ validate_new_branchname(the_repository, newname, &newref,
+ force);
reject_rebase_or_bisect_branch(oldref.buf);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2b9501520c..c98ba384dd 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1762,10 +1762,11 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
struct strbuf buf = STRBUF_INIT;
if (opts->new_branch_force)
- opts->branch_exists = validate_branchname(opts->new_branch, &buf);
+ opts->branch_exists = validate_branchname(
+ the_repository, opts->new_branch, &buf);
else
- opts->branch_exists =
- validate_new_branchname(opts->new_branch, &buf, 0);
+ opts->branch_exists = validate_new_branchname(
+ the_repository, opts->new_branch, &buf, 0);
strbuf_release(&buf);
}
--
2.33.GIT
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v1 2/3] config: introduce repo_config_set* functions
2021-11-11 17:16 ` [PATCH v1 2/3] config: introduce repo_config_set* functions Glen Choo
@ 2021-11-11 20:24 ` Junio C Hamano
2021-11-12 0:45 ` Glen Choo
0 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2021-11-11 20:24 UTC (permalink / raw)
To: Glen Choo; +Cc: git
Glen Choo <chooglen@google.com> writes:
> We have git_config_set() that sets a config value for the_repository's
> config file, and repo_config_get* that reads config values from a struct
> repository. Thus, it seems reasonable to have to have
> repo_git_config_set() that can set values for a config file of a struct
> repository.
>
> Implement repo_config_set() and repo_config_set_gently(), which
> take struct repository. However, unlike other instances where struct
> repository is added to a repo_* function, this patch does not change the
> implementations of git_config_set() or git_config_set_gently(); those
> functions use the_repository much deeper in their call chain through
> git_pathdup(). Mark this inconsistency as NEEDSWORK.
Being able to only affect "config" in the_repository->gitdir is less
flexible than being able to affect "config" in repo->gitdir for any
repository is good. Do we need a similar thing for repo->commondir
as well?
> +/*
> + * Sets a config value in a repository.
> + */
> +int repo_config_set_gently(struct repository *r, const char *key,
> + const char *value)
> +{
> + int ret;
> + char *path;
> +
> + path = repo_git_path(r, "config");
> + ret = git_config_set_in_file_gently(path, key, value);
> + free(path);
> + return ret;
> +}
> +
> +void repo_config_set(struct repository *r, const char *key, const char *value)
> +{
> + char *path;
> +
> + path = repo_git_path(r, "config");
> + git_config_set_in_file(path, key, value);
> + free(path);
> +}
Many questions:
- What do these do for an existing key? Add another value?
Replace existing one? If the latter, what do we plan to do with
multi-valued keys?
- Don't we need an interface to remove, rename, etc.?
- If we call repo_config_set(repo, "key", "value") and then call
repo_git_config_string(repo, "key", &value) on the same
repository, do we read the value back or do we give a stale
value?
- A change like this should make existing config_set() that only
works on the_repository into a thin wrapper, e.g.
void git_config_set(const char *keyu, const char **value)
{
repo_config_set(the_repository, key, value);
}
But that is not happening here. What prevents us from doing so?
Thanks.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 2/3] config: introduce repo_config_set* functions
2021-11-11 20:24 ` Junio C Hamano
@ 2021-11-12 0:45 ` Glen Choo
2021-11-15 22:17 ` Glen Choo
0 siblings, 1 reply; 7+ messages in thread
From: Glen Choo @ 2021-11-12 0:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Junio C Hamano <gitster@pobox.com> writes:
> Glen Choo <chooglen@google.com> writes:
>
>> We have git_config_set() that sets a config value for the_repository's
>> config file, and repo_config_get* that reads config values from a struct
>> repository. Thus, it seems reasonable to have to have
>> repo_git_config_set() that can set values for a config file of a struct
>> repository.
>>
>> Implement repo_config_set() and repo_config_set_gently(), which
>> take struct repository. However, unlike other instances where struct
>> repository is added to a repo_* function, this patch does not change the
>> implementations of git_config_set() or git_config_set_gently(); those
>> functions use the_repository much deeper in their call chain through
>> git_pathdup(). Mark this inconsistency as NEEDSWORK.
>
> Being able to only affect "config" in the_repository->gitdir is less
> flexible than being able to affect "config" in repo->gitdir for any
> repository is good. Do we need a similar thing for repo->commondir
> as well?
git_config_set() can only affect repo->gitdir, so from the perspective
of "what should a repo_* variant of git_config_set() do", then no, we do
not need a similar thing. As far as I can tell, config.c only works with
the gitdir, not the commondir.
I cannot comment on whether we will ever need to set config values in
the commondir.
> Many questions:
>
> - What do these do for an existing key? Add another value?
> Replace existing one? If the latter, what do we plan to do with
> multi-valued keys?
For an existing key, this should replace the first instance found.
This eventually calls of git_config_set_multivar_in_file_gently() with
value_pattern=NULL, flags = 0. According to the comments:
* if flags contains the CONFIG_FLAGS_MULTI_REPLACE flag, all matching
* key/values are removed before a single new pair is written. If the
* flag is not present, then replace only the first match.
We pass flags=0, so only the first instance is replaced.
* - [value_pattern] as a string. It will disregard key/value pairs where value
* does not match.
We pass value_pattern=NULL, so we consider all instances.
> - Don't we need an interface to remove, rename, etc.?
This function supports 'remove' by passing value=NULL. By rename, I
believe you're referring to renaming a config section, e.g. a repo_*
version of git_config_copy_or_rename_section_in_file()?
I think this is warranted if we perform a full plumbing of struct
repository through config.c. But I think it would be prudent to do this
plumbing piecemeal - perhaps starting with "set", and then proceeding to
the other operations.
> - If we call repo_config_set(repo, "key", "value") and then call
> repo_git_config_string(repo, "key", &value) on the same
> repository, do we read the value back or do we give a stale
> value?
We read the correct value if repo == the_repository but we do not if r
!= the_repository. Thanks for spotting this bug.
I believe your concern comes from the fact that struct repository
caches config values in repo->config and thus we are not guaranteed to
read the value back from the file. Following this train of thought, we
can see that git_config_set_multivar_in_file_gently() clears the cache
for the_repository, by calling git_config_clear(). Because this is
hardcoded to the_repository, git_config_set_multivar_in_file_gently()
cannot be safely called from repo_config_set() and my implementation is
buggy.
> - A change like this should make existing config_set() that only
> works on the_repository into a thin wrapper, e.g.
>
> void git_config_set(const char *keyu, const char **value)
> {
> repo_config_set(the_repository, key, value);
> }
>
> But that is not happening here. What prevents us from doing so?
I think it is _possible_, as long as we plumb struct repository through
the call chain all the way to git_config_set_multivar_in_file_gently().
I didn't do so because I thought that I had an implementation of
repo_config_set() without introducing a major overhaul of config.c.
Because it is an alternative implementation, I decided not to replace
the existing one.
However, as noted above, this alternative implementation is wrong
because git_config_set_multivar_in_file_gently() makes use of
the_repository (i.e. I didn't read the function carefully enough).
I will attempt this plumbing, which will allow us to make
git_config_set() a thin wrapper. However, if this turns out to be too
difficult, I will implement branch --recurse-submodules with child
processes and leave this for a future clean up (as hinted at in the
cover letter).
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 2/3] config: introduce repo_config_set* functions
2021-11-12 0:45 ` Glen Choo
@ 2021-11-15 22:17 ` Glen Choo
0 siblings, 0 replies; 7+ messages in thread
From: Glen Choo @ 2021-11-15 22:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Glen Choo <chooglen@google.com> writes:
>> - A change like this should make existing config_set() that only
>> works on the_repository into a thin wrapper, e.g.
>>
>> void git_config_set(const char *keyu, const char **value)
>> {
>> repo_config_set(the_repository, key, value);
>> }
>>
>> But that is not happening here. What prevents us from doing so?
>
> I think it is _possible_, as long as we plumb struct repository through
> the call chain all the way to git_config_set_multivar_in_file_gently().
>
> [...]
>
> I will attempt this plumbing, which will allow us to make
> git_config_set() a thin wrapper. However, if this turns out to be too
> difficult, I will implement branch --recurse-submodules with child
> processes and leave this for a future clean up (as hinted at in the
> cover letter).
I believe that this plumbing is already doable but it would take an
extensive overhaul of config.c as well as some cleanup of path.c.
This is more work than I am willing to take on right now, but I'll
revisit this at some point.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-11-15 23:16 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-11 17:16 [PATCH v1 0/3] make create_branch() accept any repository Glen Choo
2021-11-11 17:16 ` [PATCH v1 1/3] refs/files-backend: remove the_repository Glen Choo
2021-11-11 17:16 ` [PATCH v1 2/3] config: introduce repo_config_set* functions Glen Choo
2021-11-11 20:24 ` Junio C Hamano
2021-11-12 0:45 ` Glen Choo
2021-11-15 22:17 ` Glen Choo
2021-11-11 17:16 ` [PATCH v1 3/3] branch: remove implicit the_repository from create_branch() Glen Choo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.