| Message ID | 20250307-493-add-command-to-purge-reflog-entries-v1-1-84ab8529cf9e@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | EDITME: cover title for 493-add-command-to-purge-reflog-entries | expand |
Karthik Nayak <karthik.188@gmail.com> writes: > The 'builtin/reflog.c' file uses the 'the_repository' global variable > directly and also via 'git_config()'. Since this is a builtin command > which has access to the 'struct repository', drop usage of the global > variable and use the available repository struct. > > With this, remove the 'USE_THE_REPOSITORY_VARIABLE' macro from the file. I suspect that this is not quite right. $ cd w/git.git/; make $ ./git-reflog list -h usage: git reflog list $ cd .. # not a repository $ git.git/git-reflog list -h fatal: not a git repository (or any of the parent directories): .git $ git.git/git-reflog -h usage: git reflog [show] ... but I also suspect that it is mostly due to the original program structure that uses OPT_SUBCOMMAND() that the subcommands fail to respond to "-h" unlike the top-level command, so this may not be a regression. I do think however that this change is making it harder to fix. In any case, when you are adding a new feature, I would strongly prefer you did *not* take it hostage to unrelated internal clean-up with a dubious value. For the library-ish parts of the system (e.g. reflog.c at the top-level), not depending on the_repository is absolutely the good thing to do, but the top level cmd_foo() are not meant to be called as helper functions repeatedly with arbirary repository instance, and a churn like this one, only to mollify the USE_THE_REPOSITORY_VARIABLE macro, does not deserve to take a more interesting (in the sense that it improves the life of end users) change hostage by pretending to be its prerequisite clean-up. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >> The 'builtin/reflog.c' file uses the 'the_repository' global variable >> directly and also via 'git_config()'. Since this is a builtin command >> which has access to the 'struct repository', drop usage of the global >> variable and use the available repository struct. >> >> With this, remove the 'USE_THE_REPOSITORY_VARIABLE' macro from the file. > > I suspect that this is not quite right. > > $ cd w/git.git/; make > $ ./git-reflog list -h > usage: git reflog list > $ cd .. # not a repository > $ git.git/git-reflog list -h > fatal: not a git repository (or any of the parent directories): .git > $ git.git/git-reflog -h > usage: git reflog [show] ... > > but I also suspect that it is mostly due to the original program > structure that uses OPT_SUBCOMMAND() that the subcommands fail to > respond to "-h" unlike the top-level command, so this may not be a > regression. I do think however that this change is making it harder > to fix. > Hmm. But this is the existing behavior, no? # Inside a git directory $ eza .git b4.template branches COMMIT_EDITMSG config description FETCH_HEAD filter-repo HEAD hooks index info objects refs reftable rr-cache $ git reflog -h usage: git reflog [show] [<log-options>] [<ref>] or: git reflog list or: git reflog expire [--expire=<time>] [--expire-unreachable=<time>] [--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...] or: git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <ref>@{<specifier>}... or: git reflog exists <ref> $ git reflog list -h usage: git reflog list $ ~/code/git/build/bin-wrappers/git reflog -h usage: git reflog [show] [<log-options>] [<ref>] or: git reflog list or: git reflog expire [--expire=<time>] [--expire-unreachable=<time>] [--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...] or: git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <ref>@{<specifier>}... or: git reflog exists <ref> or: git reflog drop [--all | <refs>...] $ ~/code/git/build/bin-wrappers/git reflog list -h usage: git reflog list # Outside a git repository $ eza .git ".git": No such file or directory (os error 2) $ git reflog -h usage: git reflog [show] [<log-options>] [<ref>] or: git reflog list or: git reflog expire [--expire=<time>] [--expire-unreachable=<time>] [--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...] or: git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <ref>@{<specifier>}... or: git reflog exists <ref> $ git reflog list -h fatal: not a git repository (or any of the parent directories): .git $ ~/code/git/build/bin-wrappers/git reflog -h usage: git reflog [show] [<log-options>] [<ref>] or: git reflog list or: git reflog expire [--expire=<time>] [--expire-unreachable=<time>] [--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...] or: git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <ref>@{<specifier>}... or: git reflog exists <ref> or: git reflog drop [--all | <refs>...] $ ~/code/git/build/bin-wrappers/git reflog list -h fatal: not a git repository (or any of the parent directories): .git Seems like the behavior is the same with as with (git 2.48.1). > In any case, when you are adding a new feature, I would strongly > prefer you did *not* take it hostage to unrelated internal clean-up > with a dubious value. For the library-ish parts of the system > (e.g. reflog.c at the top-level), not depending on the_repository is > absolutely the good thing to do, but the top level cmd_foo() are not > meant to be called as helper functions repeatedly with arbirary > repository instance, and a churn like this one, only to mollify the > USE_THE_REPOSITORY_VARIABLE macro, does not deserve to take a more > interesting (in the sense that it improves the life of end users) > change hostage by pretending to be its prerequisite clean-up. > > Thanks. But point taken, I'll drop this patch in the next version! Thanks
Karthik Nayak <karthik.188@gmail.com> writes: >> but I also suspect that it is mostly due to the original program >> structure that uses OPT_SUBCOMMAND() that the subcommands fail to >> respond to "-h" unlike the top-level command, so this may not be a >> regression. I do think however that this change is making it harder >> to fix. >> > > Hmm. But this is the existing behavior, no? Didn't I said I also suspect? > But point taken, I'll drop this patch in the next version! Thanks Yup. Take your time, as it is already deep in prerelease feature freeze. I'd prefer to see us leaving spare capacity in our minds to fix regressions introduced during this period once they are noticed, without getting distracted by shiny new toys. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >>> but I also suspect that it is mostly due to the original program >>> structure that uses OPT_SUBCOMMAND() that the subcommands fail to >>> respond to "-h" unlike the top-level command, so this may not be a >>> regression. I do think however that this change is making it harder >>> to fix. >>> >> >> Hmm. But this is the existing behavior, no? > > Didn't I said I also suspect? > Yes, I misunderstood! >> But point taken, I'll drop this patch in the next version! Thanks > > Yup. Take your time, as it is already deep in prerelease feature > freeze. I'd prefer to see us leaving spare capacity in our minds to > fix regressions introduced during this period once they are noticed, > without getting distracted by shiny new toys. > > Thanks. Fair enough. I only pushed this new topic, because I had mentioned to you that I would pick it up and I didn't want to forget about it. Either ways, I will take it slow here! :)
diff --git a/builtin/reflog.c b/builtin/reflog.c index 95f264989b..f92258f6b6 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c @@ -1,5 +1,3 @@ -#define USE_THE_REPOSITORY_VARIABLE - #include "builtin.h" #include "config.h" #include "gettext.h" @@ -236,7 +234,7 @@ static int expire_total_callback(const struct option *opt, } static int cmd_reflog_show(int argc, const char **argv, const char *prefix, - struct repository *repo UNUSED) + struct repository *repo) { struct option options[] = { OPT_END() @@ -246,7 +244,7 @@ static int cmd_reflog_show(int argc, const char **argv, const char *prefix, PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT); - return cmd_log_reflog(argc, argv, prefix, the_repository); + return cmd_log_reflog(argc, argv, prefix, repo); } static int show_reflog(const char *refname, void *cb_data UNUSED) @@ -256,7 +254,7 @@ static int show_reflog(const char *refname, void *cb_data UNUSED) } static int cmd_reflog_list(int argc, const char **argv, const char *prefix, - struct repository *repo UNUSED) + struct repository *repo) { struct option options[] = { OPT_END() @@ -268,13 +266,13 @@ static int cmd_reflog_list(int argc, const char **argv, const char *prefix, return error(_("%s does not accept arguments: '%s'"), "list", argv[0]); - ref_store = get_main_ref_store(the_repository); + ref_store = get_main_ref_store(repo); return refs_for_each_reflog(ref_store, show_reflog, NULL); } static int cmd_reflog_expire(int argc, const char **argv, const char *prefix, - struct repository *repo UNUSED) + struct repository *repo) { struct cmd_reflog_expire_cb cmd = { 0 }; timestamp_t now = time(NULL); @@ -310,7 +308,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix, default_reflog_expire_unreachable = now - 30 * 24 * 3600; default_reflog_expire = now - 90 * 24 * 3600; - git_config(reflog_expire_config, NULL); + repo_config(repo, reflog_expire_config, NULL); save_commit_buffer = 0; do_all = status = 0; @@ -332,7 +330,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix, if (cmd.stalefix) { struct rev_info revs; - repo_init_revisions(the_repository, &revs, prefix); + repo_init_revisions(repo, &revs, prefix); revs.do_not_die_on_missing_objects = 1; revs.ignore_missing = 1; revs.ignore_missing_links = 1; @@ -368,7 +366,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix, }; set_reflog_expiry_param(&cb.cmd, item->string); - status |= refs_reflog_expire(get_main_ref_store(the_repository), + status |= refs_reflog_expire(get_main_ref_store(repo), item->string, flags, reflog_expiry_prepare, should_prune_fn, @@ -382,12 +380,12 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix, char *ref; struct expire_reflog_policy_cb cb = { .cmd = cmd }; - if (!repo_dwim_log(the_repository, argv[i], strlen(argv[i]), NULL, &ref)) { + if (!repo_dwim_log(repo, argv[i], strlen(argv[i]), NULL, &ref)) { status |= error(_("%s points nowhere!"), argv[i]); continue; } set_reflog_expiry_param(&cb.cmd, ref); - status |= refs_reflog_expire(get_main_ref_store(the_repository), + status |= refs_reflog_expire(get_main_ref_store(repo), ref, flags, reflog_expiry_prepare, should_prune_fn, @@ -430,7 +428,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix, } static int cmd_reflog_exists(int argc, const char **argv, const char *prefix, - struct repository *repo UNUSED) + struct repository *repo) { struct option options[] = { OPT_END() @@ -445,7 +443,7 @@ static int cmd_reflog_exists(int argc, const char **argv, const char *prefix, refname = argv[0]; if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) die(_("invalid ref format: %s"), refname); - return !refs_reflog_exists(get_main_ref_store(the_repository), + return !refs_reflog_exists(get_main_ref_store(repo), refname); }
The 'builtin/reflog.c' file uses the 'the_repository' global variable directly and also via 'git_config()'. Since this is a builtin command which has access to the 'struct repository', drop usage of the global variable and use the available repository struct. With this, remove the 'USE_THE_REPOSITORY_VARIABLE' macro from the file. Signed-off-by: Karthik Nayak <karthik.188@gmail.com> --- builtin/reflog.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)