| Message ID | cover.1723540226.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | Stop using `the_repository` in "config.c" | expand |
Patrick Steinhardt <ps@pks.im> writes: > this is the second version of my patch series that drops the dependency > on `the_repository` in both "path.c" and "config.c". > > Changes compared to v1: > > - Various typo fixes in commit messages. > > - Rename `strbuf_git_common_pathv()` to `repo_common_pathv()`. > > - Reorder arguments of `strbuf_edit_interactively()` such that the > `struct repository` gets passed as first argument. > > - Document behaviour of `worktree_git_path()` when no worktree is > given. > > - Wrap some overly long lines. > > Thanks! Nicely done. I found that both of the two larger changes relative to the previous iteration are sensible.
Hi Patrick,
Glad to see us slowly getting rid of the global, `the_repository`! I had
missed your original series[1] introducing the
USE_THE_REPOSITORY_AVAILABLE macro, but going thru it provided all the
context necessary for this series.
I see there have been a few cleanups since the first version, and I
didn't spot any typos on my end. Overall, this is a good step towards
reducing our dependency on `the_repository` by removing it from the
config subsystem. Additionally, this makes libifying that subsystem much
easier since removing globals usage is a prerequisite for libification.
I only left a non-blocking comment on one of the patches, but besides
that this series LGTM. Feel free to cc me on any related series in the
future!
[1] https://lore.kernel.org/git/cover.1718106284.git.ps@pks.im/
Reviewed-by: Calvin Wan <calvinwan@google.com>
On 24/08/13 11:13AM, Patrick Steinhardt wrote: > Hi, > > this is the second version of my patch series that drops the dependency > on `the_repository` in both "path.c" and "config.c". > > Changes compared to v1: > > - Various typo fixes in commit messages. > > - Rename `strbuf_git_common_pathv()` to `repo_common_pathv()`. > > - Reorder arguments of `strbuf_edit_interactively()` such that the > `struct repository` gets passed as first argument. > > - Document behaviour of `worktree_git_path()` when no worktree is > given. > > - Wrap some overly long lines. > > Thanks! After reviewing the changes in this version, the series looks good to me! Thanks :) -Justin
On Wed, Aug 14, 2024 at 07:29:57PM +0000, Calvin Wan wrote: > Hi Patrick, > > Glad to see us slowly getting rid of the global, `the_repository`! I had > missed your original series[1] introducing the > USE_THE_REPOSITORY_AVAILABLE macro, but going thru it provided all the > context necessary for this series. > > I see there have been a few cleanups since the first version, and I > didn't spot any typos on my end. Overall, this is a good step towards > reducing our dependency on `the_repository` by removing it from the > config subsystem. Additionally, this makes libifying that subsystem much > easier since removing globals usage is a prerequisite for libification. > I only left a non-blocking comment on one of the patches, but besides > that this series LGTM. Feel free to cc me on any related series in the > future! > > [1] https://lore.kernel.org/git/cover.1718106284.git.ps@pks.im/ Thanks, will do! Patrick
Hi, this is the second version of my patch series that drops the dependency on `the_repository` in both "path.c" and "config.c". Changes compared to v1: - Various typo fixes in commit messages. - Rename `strbuf_git_common_pathv()` to `repo_common_pathv()`. - Reorder arguments of `strbuf_edit_interactively()` such that the `struct repository` gets passed as first argument. - Document behaviour of `worktree_git_path()` when no worktree is given. - Wrap some overly long lines. Thanks! Patrick Patrick Steinhardt (20): path: expose `do_git_path()` as `repo_git_pathv()` path: expose `do_git_common_path()` as `repo_common_pathv()` editor: do not rely on `the_repository` for interactive edits hooks: remove implicit dependency on `the_repository` path: stop relying on `the_repository` when reporting garbage path: stop relying on `the_repository` in `worktree_git_path()` path: hide functions using `the_repository` by default config: introduce missing setters that take repo as parameter config: expose `repo_config_clear()` config: pass repo to `git_config_get_index_threads()` config: pass repo to `git_config_get_split_index()` config: pass repo to `git_config_get_max_percent_split_change()` config: pass repo to `git_config_get_expiry()` config: pass repo to `git_config_get_expiry_in_days()` config: pass repo to `git_die_config()` config: pass repo to functions that rename or copy sections config: don't have setters depend on `the_repository` config: don't depend on `the_repository` with branch conditions global: prepare for hiding away repo-less config functions config: hide functions using `the_repository` by default add-patch.c | 3 +- builtin/am.c | 9 +- builtin/branch.c | 7 +- builtin/bugreport.c | 2 +- builtin/checkout.c | 2 +- builtin/clone.c | 2 +- builtin/config.c | 16 +- builtin/count-objects.c | 2 +- builtin/fast-import.c | 4 +- builtin/fsck.c | 2 +- builtin/gc.c | 8 +- builtin/hook.c | 2 +- builtin/merge.c | 2 +- builtin/notes.c | 2 +- builtin/rebase.c | 2 +- builtin/receive-pack.c | 10 +- builtin/remote.c | 4 +- builtin/submodule--helper.c | 2 +- builtin/update-index.c | 4 +- builtin/worktree.c | 6 +- commit.c | 2 +- compat/fsmonitor/fsm-ipc-darwin.c | 2 + compat/precompose_utf8.c | 1 + config.c | 225 ++++++++--------------- config.h | 285 +++++++++++++++++++++--------- connect.c | 2 + credential.c | 2 + daemon.c | 2 + editor.c | 15 +- editor.h | 5 +- fsmonitor.c | 2 + gpg-interface.c | 2 + graph.c | 2 + hook.c | 21 +-- hook.h | 13 +- imap-send.c | 2 + mailinfo.c | 2 + merge-ll.c | 2 + parallel-checkout.c | 2 + path.c | 97 +++------- path.h | 170 ++++++++++++------ protocol.c | 2 + read-cache.c | 22 +-- refs.c | 4 +- refs/packed-backend.c | 2 + refs/reftable-backend.c | 2 + rerere.c | 6 +- reset.c | 2 +- revision.c | 2 +- sequencer.c | 6 +- sideband.c | 2 + submodule.c | 2 +- t/helper/test-advise.c | 2 + t/helper/test-config.c | 2 + t/helper/test-userdiff.c | 2 + trailer.c | 2 + transport.c | 2 +- versioncmp.c | 2 + worktree.c | 2 +- wt-status.c | 14 +- 60 files changed, 570 insertions(+), 457 deletions(-) Range-diff against v1: 1: 7ce3278f64 = 1: d0f5f2b17f path: expose `do_git_path()` as `repo_git_pathv()` 2: e7a143c30d ! 2: 2e3f474e5d path: expose `do_git_common_path()` as `strbuf_git_common_pathv()` @@ Metadata Author: Patrick Steinhardt <ps@pks.im> ## Commit message ## - path: expose `do_git_common_path()` as `strbuf_git_common_pathv()` + path: expose `do_git_common_path()` as `repo_common_pathv()` With the same reasoning as the preceding commit, expose the function - `do_git_common_path()` as `strbuf_git_common_pathv()`. + `do_git_common_path()` as `repo_common_pathv()`. While at it, reorder + parameters such that they match the order we have in `repo_git_pathv()`. Signed-off-by: Patrick Steinhardt <ps@pks.im> @@ path.c: int strbuf_git_path_submodule(struct strbuf *buf, const char *path, - struct strbuf *buf, - const char *fmt, - va_list args) -+void strbuf_git_common_pathv(struct strbuf *sb, -+ const struct repository *repo, -+ const char *fmt, -+ va_list args) ++void repo_common_pathv(const struct repository *repo, ++ struct strbuf *sb, ++ const char *fmt, ++ va_list args) { - strbuf_addstr(buf, repo->commondir); - if (buf->len && !is_dir_sep(buf->buf[buf->len - 1])) @@ path.c: const char *git_common_path(const char *fmt, ...) va_list args; va_start(args, fmt); - do_git_common_path(the_repository, pathname, fmt, args); -+ strbuf_git_common_pathv(pathname, the_repository, fmt, args); ++ repo_common_pathv(the_repository, pathname, fmt, args); va_end(args); return pathname->buf; } @@ path.c: void strbuf_git_common_path(struct strbuf *sb, va_list args; va_start(args, fmt); - do_git_common_path(repo, sb, fmt, args); -+ strbuf_git_common_pathv(sb, repo, fmt, args); ++ repo_common_pathv(repo, sb, fmt, args); va_end(args); } @@ path.h: void strbuf_git_common_path(struct strbuf *sb, const struct repository *repo, const char *fmt, ...) __attribute__((format (printf, 3, 4))); -+void strbuf_git_common_pathv(struct strbuf *sb, -+ const struct repository *repo, -+ const char *fmt, -+ va_list args); ++void repo_common_pathv(const struct repository *repo, ++ struct strbuf *buf, ++ const char *fmt, ++ va_list args); /* * Return a statically allocated path into the main repository's 3: c2556fff9e ! 3: 2f73e0efcd editor: do not rely on `the_repository` for interactive edits @@ Commit message We implicitly rely on `the_repository` when editing a file interactively because we call `git_path()`. Adapt the function to instead take a - `sturct repository` as parameter so that we can remove this hidden + `struct repository` as a parameter so that we can remove this hidden dependency. Signed-off-by: Patrick Steinhardt <ps@pks.im> @@ add-patch.c: static int edit_hunk_manually(struct add_p_state *s, struct hunk *h "aborted and the hunk is left unchanged.\n")); - if (strbuf_edit_interactively(&s->buf, "addp-hunk-edit.diff", NULL) < 0) -+ if (strbuf_edit_interactively(&s->buf, "addp-hunk-edit.diff", NULL, -+ the_repository) < 0) ++ if (strbuf_edit_interactively(the_repository, &s->buf, ++ "addp-hunk-edit.diff", NULL) < 0) return -1; /* strip out commented lines */ ## editor.c ## @@ editor.c: int launch_sequence_editor(const char *path, struct strbuf *buffer, + return launch_specified_editor(git_sequence_editor(), path, buffer, env); } - int strbuf_edit_interactively(struct strbuf *buffer, const char *path, -- const char *const *env) -+ const char *const *env, struct repository *r) +-int strbuf_edit_interactively(struct strbuf *buffer, const char *path, ++int strbuf_edit_interactively(struct repository *r, ++ struct strbuf *buffer, const char *path, + const char *const *env) { - char *path2 = NULL; + struct strbuf sb = STRBUF_INIT; @@ editor.h const char *git_editor(void); @@ editor.h: int launch_sequence_editor(const char *path, struct strbuf *buffer, + * * If `path` is relative, it refers to a file in the `.git` directory. */ - int strbuf_edit_interactively(struct strbuf *buffer, const char *path, +-int strbuf_edit_interactively(struct strbuf *buffer, const char *path, - const char *const *env); -+ const char *const *env, struct repository *r); ++int strbuf_edit_interactively(struct repository *r, struct strbuf *buffer, ++ const char *path, const char *const *env); #endif 4: 9a0964aff3 = 4: 688d705179 hooks: remove implicit dependency on `the_repository` 5: 4368b32f65 = 5: bbaa85ebad path: stop relying on `the_repository` when reporting garbage 6: 67405dcd0a ! 6: 29302f4b14 path: stop relying on `the_repository` in `worktree_git_path()` @@ path.c: const char *mkpath(const char *fmt, ...) ## path.h ## @@ path.h: const char *git_path(const char *fmt, ...) + + /* * Similar to git_path() but can produce paths for a specified - * worktree instead of current one +- * worktree instead of current one ++ * worktree instead of current one. When no worktree is given, then the path is ++ * computed relative to main worktree of the given repository. */ -const char *worktree_git_path(const struct worktree *wt, +const char *worktree_git_path(struct repository *r, 7: b4e973a280 ! 7: 1b6651770a path: hide functions using `the_repository` by default @@ Metadata ## Commit message ## path: hide functions using `the_repository` by default - The path subsytem provides a bunch of legacy functions that compute + The path subsystem provides a bunch of legacy functions that compute paths relative to the "gitdir" and "commondir" directories of the global `the_repository` variable. Use of those functions is discouraged, and it is easy to miss the implicit dependency on `the_repository` that calls to those functions may cause. With `USE_THE_REPOSITORY_VARIABLE`, we have recently introduced a tool - that allows us to get rid of such functions over time. With this define, + that allows us to get rid of such functions over time. With this macro, we can hide away functions that have such implicit dependency such that other subsystems that want to be free of `the_repository` will not use them by accident. @@ path.c: void strbuf_repo_git_path(struct strbuf *sb, char *mkpathdup(const char *fmt, ...) { struct strbuf sb = STRBUF_INIT; -@@ path.c: void strbuf_git_common_pathv(struct strbuf *sb, +@@ path.c: void repo_common_pathv(const struct repository *repo, strbuf_cleanup_path(sb); } @@ path.c: void strbuf_git_common_pathv(struct strbuf *sb, - struct strbuf *pathname = get_pathname(); - va_list args; - va_start(args, fmt); -- strbuf_git_common_pathv(pathname, the_repository, fmt, args); +- repo_common_pathv(the_repository, pathname, fmt, args); - va_end(args); - return pathname->buf; -} @@ path.h: char *mkpathdup(const char *fmt, ...) * repository's common git directory, which is shared by all worktrees. */ -@@ path.h: void strbuf_git_common_pathv(struct strbuf *sb, - va_list args); +@@ path.h: void repo_common_pathv(const struct repository *repo, + va_list args); /* - * Return a statically allocated path into the main repository's @@ path.h: void strbuf_repo_git_path(struct strbuf *sb, -/* - * Similar to git_path() but can produce paths for a specified + * Similar to repo_git_path() but can produce paths for a specified - * worktree instead of current one + * worktree instead of current one. When no worktree is given, then the path is + * computed relative to main worktree of the given repository. */ - const char *worktree_git_path(struct repository *r, @@ path.h: const char *worktree_git_path(struct repository *r, const char *fmt, ...) __attribute__((format (printf, 3, 4))); @@ path.h: char *xdg_cache_home(const char *filename); + struct strbuf *pathname = get_pathname(); + va_list args; + va_start(args, fmt); -+ strbuf_git_common_pathv(pathname, the_repository, fmt, args); ++ repo_common_pathv(the_repository, pathname, fmt, args); + va_end(args); + return pathname->buf; +} 8: feae2ad31a ! 8: e316491e56 config: introduce missing setters that take repo as parameter @@ Commit message While we already provide some of the config-setting interfaces with a `struct repository` as parameter, others only have a variant that - implicitly depend on `the_repository`. Fill in those gaps such that we + implicitly depends on `the_repository`. Fill in those gaps such that we can start to deprecate the repo-less variants. Signed-off-by: Patrick Steinhardt <ps@pks.im> 9: 63da87696e = 9: 5e73ef6cec config: expose `repo_config_clear()` 10: 03df1277f7 = 10: 35e0f4579c config: pass repo to `git_config_get_index_threads()` 11: 85311d686e = 11: a88ed83b97 config: pass repo to `git_config_get_split_index()` 12: b95ed05f88 = 12: 17c5420d63 config: pass repo to `git_config_get_max_percent_split_change()` 13: 412cc514e8 = 13: f39d4a3244 config: pass repo to `git_config_get_expiry()` 14: cf7942479f ! 14: 111f77e653 config: pass repo to `git_config_get_expiry_in_days()` @@ rerere.c: void rerere_gc(struct repository *r, struct string_list *rr) - git_config_get_expiry_in_days("gc.rerereresolved", &cutoff_resolve, now); - git_config_get_expiry_in_days("gc.rerereunresolved", &cutoff_noresolve, now); -+ repo_config_get_expiry_in_days(the_repository, "gc.rerereresolved", &cutoff_resolve, now); -+ repo_config_get_expiry_in_days(the_repository, "gc.rerereunresolved", &cutoff_noresolve, now); ++ repo_config_get_expiry_in_days(the_repository, "gc.rerereresolved", ++ &cutoff_resolve, now); ++ repo_config_get_expiry_in_days(the_repository, "gc.rerereunresolved", ++ &cutoff_noresolve, now); git_config(git_default_config, NULL); dir = opendir(git_path("rr-cache")); if (!dir) 15: d70d9bfa7e = 15: 6adee5633a config: pass repo to `git_die_config()` 16: 980533972c = 16: b8c54b751c config: pass repo to functions that rename or copy sections 17: 1c1b27cbac = 17: 1e5cd92e8e config: don't have setters depend on `the_repository` 18: d8530a300b = 18: 96963364e7 config: don't depend on `the_repository` with branch conditions 19: f7617beaa5 ! 19: 124f5794e6 global: prepare for hiding away repo-less config functions @@ Commit message We're about to hide config functions that implicitly depend on `the_repository` behind the `USE_THE_REPOSITORY_VARIABLE` macro. This will uncover a bunch of dependents that transitively relied on the - global variable, but didn'd define the macro yet. + global variable, but didn't define the macro yet. Adapt them such that we define the macro to prepare for this change. 20: 43757f3077 ! 20: 467cd481f5 config: hide functions using `the_repository` by default @@ Metadata ## Commit message ## config: hide functions using `the_repository` by default - The config subsytem provides a bunch of legacy functions that read or + The config subsystem provides a bunch of legacy functions that read or set configuration for `the_repository`. The use of those functions is discouraged, and it is easy to miss the implicit dependency on `the_repository` that calls to those functions may cause.