| Message ID | 20250306143629.1267358-9-usmanakinyemi202@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | b6e37a70b033824f389746e747eae4f8fdbcc5eb |
| Headers | show |
| Series | stop using the_repository global variable. | expand |
Usman Akinyemi <usmanakinyemi202@gmail.com> writes: > Remove the_repository global variable in favor of the repository > argument that gets passed in "builtin/checkout-index.c". > > When `-h` is passed to the command outside a Git repository, the > `run_builtin()` will call the `cmd_checkout_index()` function with `repo` > set to NULL and then early in the function, `show_usage_with_options_if_asked()` > call will give the options help and exit. > > Pass the repository available in the calling context to both `checkout_all()` > and `checkout_file()` to remove their dependency on the global > `the_repository` variable. Hmph, if we are passing anything down to these code paths, I would have expected that it would be an instance of "struct index_state". Do these two helper functions need anything other than that from the repository instance? Other than that, I think this step does look great. Will queue. Thanks.
On Thu, Mar 6, 2025 at 11:48 PM Junio C Hamano <gitster@pobox.com> wrote: > > Usman Akinyemi <usmanakinyemi202@gmail.com> writes: > > > Remove the_repository global variable in favor of the repository > > argument that gets passed in "builtin/checkout-index.c". > > > > When `-h` is passed to the command outside a Git repository, the > > `run_builtin()` will call the `cmd_checkout_index()` function with `repo` > > set to NULL and then early in the function, `show_usage_with_options_if_asked()` > > call will give the options help and exit. > > > > Pass the repository available in the calling context to both `checkout_all()` > > and `checkout_file()` to remove their dependency on the global > > `the_repository` variable. > > Hmph, if we are passing anything down to these code paths, I would > have expected that it would be an instance of "struct index_state". > > Do these two helper functions need anything other than that from the > repository instance? No, they do not. They could possibly do in the future and is there any reason why we might want to pass the "struct index_state" instead of the whole "struct repository" ? > > Other than that, I think this step does look great. > > Will queue. > > Thanks.
Usman Akinyemi <usmanakinyemi202@gmail.com> writes: >> Hmph, if we are passing anything down to these code paths, I would >> have expected that it would be an instance of "struct index_state". >> >> Do these two helper functions need anything other than that from the >> repository instance? > No, they do not. They could possibly do in the future and is there any > reason why we might want to pass the "struct index_state" instead of > the whole "struct repository" ? You are asking a wrong question. When there are two things, A and B, where B is a piece of data with a smaller scope that is contained within A, unless your function needs to access parts of A that is impossible to access if you fed it only B, limiting the interface to the smallest piece necessary (in this case, B) is always the better design. A potential caller that only has B and not A can still call your function that takes B if you designed the API that way. If you insist that the caller pass A (and infer which B to use as part of A), a potential caller that only has B cannot call your function. So, with that understanding of a general principle in mind, you need a stronger justification to pass the larger object A that goes against the best practice, saying "Even though we do not have to pass A because only B is necessary, we pass A because ...". And you just said there is no such reason why you need a repository and for these functions an index_state is sufficient. There are many functions that only need "struct index_state" instance and obtains one from their callers. They do not have a repository object, other than the one that is referred to from the index_state (to keep track of which repository it originated from). But this pointer is not designed to round-trip. There can be, and there indeed are, code paths that use multiple index_state instances associated with one repository. But a repository instance has only one ".index" member. It means if you pass a repository, you are making it impossible for your potential callers that has secondary index_state instances.
On Fri, Mar 7, 2025 at 7:45 PM Junio C Hamano <gitster@pobox.com> wrote: > > Usman Akinyemi <usmanakinyemi202@gmail.com> writes: > > >> Hmph, if we are passing anything down to these code paths, I would > >> have expected that it would be an instance of "struct index_state". > >> > >> Do these two helper functions need anything other than that from the > >> repository instance? > > No, they do not. They could possibly do in the future and is there any > > reason why we might want to pass the "struct index_state" instead of > > the whole "struct repository" ? > > You are asking a wrong question. > > When there are two things, A and B, where B is a piece of data with > a smaller scope that is contained within A, unless your function > needs to access parts of A that is impossible to access if you fed > it only B, limiting the interface to the smallest piece necessary > (in this case, B) is always the better design. A potential caller > that only has B and not A can still call your function that takes B > if you designed the API that way. If you insist that the caller > pass A (and infer which B to use as part of A), a potential caller > that only has B cannot call your function. > > So, with that understanding of a general principle in mind, you need > a stronger justification to pass the larger object A that goes > against the best practice, saying "Even though we do not have to > pass A because only B is necessary, we pass A because ...". And you > just said there is no such reason why you need a repository and for > these functions an index_state is sufficient. > > There are many functions that only need "struct index_state" > instance and obtains one from their callers. They do not have a > repository object, other than the one that is referred to from the > index_state (to keep track of which repository it originated from). > > But this pointer is not designed to round-trip. There can be, and > there indeed are, code paths that use multiple index_state instances > associated with one repository. But a repository instance has only > one ".index" member. It means if you pass a repository, you are > making it impossible for your potential callers that has secondary > index_state instances. Thanks for the explanation. I did not know or think about this before. I will make changes to this in the next iteration. Thank you. >
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index e30086c7d4..46035444eb 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -5,7 +5,6 @@ * */ -#define USE_THE_REPOSITORY_VARIABLE #define DISABLE_SIGN_COMPARE_WARNINGS #include "builtin.h" @@ -68,10 +67,10 @@ static void write_tempfile_record(const char *name, const char *prefix) } } -static int checkout_file(const char *name, const char *prefix) +static int checkout_file(struct repository *repo, const char *name, const char *prefix) { int namelen = strlen(name); - int pos = index_name_pos(the_repository->index, name, namelen); + int pos = index_name_pos(repo->index, name, namelen); int has_same_name = 0; int is_file = 0; int is_skipped = 1; @@ -81,8 +80,8 @@ static int checkout_file(const char *name, const char *prefix) if (pos < 0) pos = -pos - 1; - while (pos <the_repository->index->cache_nr) { - struct cache_entry *ce =the_repository->index->cache[pos]; + while (pos < repo->index->cache_nr) { + struct cache_entry *ce =repo->index->cache[pos]; if (ce_namelen(ce) != namelen || memcmp(ce->name, name, namelen)) break; @@ -137,13 +136,13 @@ static int checkout_file(const char *name, const char *prefix) return -1; } -static int checkout_all(const char *prefix, int prefix_length) +static int checkout_all(struct repository *repo, const char *prefix, int prefix_length) { int i, errs = 0; struct cache_entry *last_ce = NULL; - for (i = 0; i < the_repository->index->cache_nr ; i++) { - struct cache_entry *ce = the_repository->index->cache[i]; + for (i = 0; i < repo->index->cache_nr ; i++) { + struct cache_entry *ce = repo->index->cache[i]; if (S_ISSPARSEDIR(ce->ce_mode)) { if (!ce_skip_worktree(ce)) @@ -156,8 +155,8 @@ static int checkout_all(const char *prefix, int prefix_length) * first entry inside the expanded sparse directory). */ if (ignore_skip_worktree) { - ensure_full_index(the_repository->index); - ce = the_repository->index->cache[i]; + ensure_full_index(repo->index); + ce = repo->index->cache[i]; } } @@ -213,7 +212,7 @@ static int option_parse_stage(const struct option *opt, int cmd_checkout_index(int argc, const char **argv, const char *prefix, - struct repository *repo UNUSED) + struct repository *repo) { int i; struct lock_file lock_file = LOCK_INIT; @@ -253,19 +252,19 @@ int cmd_checkout_index(int argc, show_usage_with_options_if_asked(argc, argv, builtin_checkout_index_usage, builtin_checkout_index_options); - git_config(git_default_config, NULL); + repo_config(repo, git_default_config, NULL); prefix_length = prefix ? strlen(prefix) : 0; - prepare_repo_settings(the_repository); - the_repository->settings.command_requires_full_index = 0; + prepare_repo_settings(repo); + repo->settings.command_requires_full_index = 0; - if (repo_read_index(the_repository) < 0) { + if (repo_read_index(repo) < 0) { die("invalid cache"); } argc = parse_options(argc, argv, prefix, builtin_checkout_index_options, builtin_checkout_index_usage, 0); - state.istate = the_repository->index; + state.istate = repo->index; state.force = force; state.quiet = quiet; state.not_new = not_new; @@ -285,8 +284,8 @@ int cmd_checkout_index(int argc, */ if (index_opt && !state.base_dir_len && !to_tempfile) { state.refresh_cache = 1; - state.istate = the_repository->index; - repo_hold_locked_index(the_repository, &lock_file, + state.istate = repo->index; + repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR); } @@ -304,7 +303,7 @@ int cmd_checkout_index(int argc, if (read_from_stdin) die("git checkout-index: don't mix '--stdin' and explicit filenames"); p = prefix_path(prefix, prefix_length, arg); - err |= checkout_file(p, prefix); + err |= checkout_file(repo, p, prefix); free(p); } @@ -326,7 +325,7 @@ int cmd_checkout_index(int argc, strbuf_swap(&buf, &unquoted); } p = prefix_path(prefix, prefix_length, buf.buf); - err |= checkout_file(p, prefix); + err |= checkout_file(repo, p, prefix); free(p); } strbuf_release(&unquoted); @@ -334,7 +333,7 @@ int cmd_checkout_index(int argc, } if (all) - err |= checkout_all(prefix, prefix_length); + err |= checkout_all(repo, prefix, prefix_length); if (pc_workers > 1) err |= run_parallel_checkout(&state, pc_workers, pc_threshold, @@ -344,7 +343,7 @@ int cmd_checkout_index(int argc, return 1; if (is_lock_file_locked(&lock_file) && - write_locked_index(the_repository->index, &lock_file, COMMIT_LOCK)) + write_locked_index(repo->index, &lock_file, COMMIT_LOCK)) die("Unable to write new index file"); return 0; } diff --git a/t/t2006-checkout-index-basic.sh b/t/t2006-checkout-index-basic.sh index bac231b167..fedd2cc097 100755 --- a/t/t2006-checkout-index-basic.sh +++ b/t/t2006-checkout-index-basic.sh @@ -21,6 +21,13 @@ test_expect_success 'checkout-index -h in broken repository' ' test_grep "[Uu]sage" broken/usage ' +test_expect_success 'checkout-index does not crash with -h' ' + test_expect_code 129 git checkout-index -h >usage && + test_grep "[Uu]sage: git checkout-index " usage && + test_expect_code 129 nongit git checkout-index -h >usage && + test_grep "[Uu]sage: git checkout-index " usage +' + test_expect_success 'checkout-index reports errors (cmdline)' ' test_must_fail git checkout-index -- does-not-exist 2>stderr && test_grep not.in.the.cache stderr
Remove the_repository global variable in favor of the repository argument that gets passed in "builtin/checkout-index.c". When `-h` is passed to the command outside a Git repository, the `run_builtin()` will call the `cmd_checkout_index()` function with `repo` set to NULL and then early in the function, `show_usage_with_options_if_asked()` call will give the options help and exit. Pass the repository available in the calling context to both `checkout_all()` and `checkout_file()` to remove their dependency on the global `the_repository` variable. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com> --- builtin/checkout-index.c | 43 ++++++++++++++++----------------- t/t2006-checkout-index-basic.sh | 7 ++++++ 2 files changed, 28 insertions(+), 22 deletions(-)