From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
"Han-Wen Nienhuys" <hanwen@google.com>,
"Bagas Sanjaya" <bagasdotme@gmail.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v3 0/9] reftable prep: have builtin/reflog.c handle "--verbose"
Date: Wed, 22 Dec 2021 05:06:39 +0100 [thread overview]
Message-ID: <cover-v3-0.9-00000000000-20211222T040557Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-v2-0.9-00000000000-20211216T134028Z-avarab@gmail.com>
This series refactors various small bits of builtin/reflog.c
(e.g. using a "struct string_list" now), and finally makes it handle
the "--verbose" output, instead of telling the files backend to emit
that same verbose output.
This means that when we start to integrate "reftable" the new backend
won't need to implement verbose reflog output, it will just work.
This v3 addresses comments Han-Wen had on the v2[1] in [2]. Those are
all addressed here.
I thought that eliminating the "goto" as he suggested might lead to
needlessly verbose code, but I think this new approach is much
better. We now have a "verbose" callback that's a wrapper around the
non-verbose version. For the common case of non-verbose we won't
execute any "verbose" code at all, which makes this code easier to
follow.
1. https://lore.kernel.org/git/cover-v2-0.9-00000000000-20211216T134028Z-avarab@gmail.com/
2. https://lore.kernel.org/git/CAFQ2z_McOfm545Xd8hF7YDgzyOjDmcGxpWZ6pQ-yaKAEWMMbgg@mail.gmail.com/
Ævar Arnfjörð Bjarmason (9):
reflog delete: narrow scope of "cmd" passed to count_reflog_ent()
reflog expire: narrow scope of "cb" in cmd_reflog_expire()
reflog: change one->many worktree->refnames to use a string_list
reflog expire: use "switch" over enum values
reflog expire: refactor & use "tip_commit" only for UE_NORMAL
reflog expire: don't use lookup_commit_reference_gently()
reflog: reduce scope of "struct rev_info"
refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN
reflog + refs-backend: move "verbose" out of the backend
builtin/reflog.c | 223 +++++++++++++++++++++++++------------------
refs.h | 3 +-
refs/files-backend.c | 44 ++++-----
3 files changed, 150 insertions(+), 120 deletions(-)
Range-diff against v2:
1: 22c8119640c = 1: 7fac198f485 reflog delete: narrow scope of "cmd" passed to count_reflog_ent()
2: b8e84538427 = 2: 1ffc8ef8a8b reflog expire: narrow scope of "cb" in cmd_reflog_expire()
3: c0e190e46cf ! 3: ba7679e6fc0 reflog: change one->many worktree->refnames to use a string_list
@@ Commit message
free, as a result we need just one struct to keep track of this data,
instead of two.
+ The "DUP" -> "string_list_append_nodup(..., strbuf_detach(...))"
+ pattern here is the same as that used in a recent memory leak fix in
+ b202e51b154 (grep: fix a "path_list" memory leak, 2021-10-22).
+
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## builtin/reflog.c ##
@@ builtin/reflog.c: static void reflog_expiry_cleanup(void *cb_data)
- FLEX_ALLOC_STR(e, reflog, newref.buf);
- strbuf_release(&newref);
+ strbuf_worktree_ref(worktree, &newref, ref);
-+ string_list_append(&cb->reflogs, strbuf_detach(&newref, NULL));
++ string_list_append_nodup(&cb->reflogs, strbuf_detach(&newref, NULL));
- oidcpy(&e->oid, oid);
- ALLOC_GROW(cb->e, cb->nr + 1, cb->alloc);
@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, cons
if (do_all) {
- struct collect_reflog_cb collected;
+ struct worktree_reflogs collected = {
-+ .reflogs = STRING_LIST_INIT_NODUP,
++ .reflogs = STRING_LIST_INIT_DUP,
+ };
+ struct string_list_item *item;
struct worktree **worktrees, **p;
@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, cons
- free(e);
}
- free(collected.e);
-+ collected.reflogs.strdup_strings = 1;
+ string_list_clear(&collected.reflogs, 0);
}
4: e42fac1b518 ! 4: 74447de0413 reflog expire: don't do negative comparison on enum values
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- reflog expire: don't do negative comparison on enum values
+ reflog expire: use "switch" over enum values
Change code added in 03cb91b18cc (reflog --expire-unreachable: special
- case entries in "HEAD" reflog, 2010-04-09) to not do positive instead
- of negative comparisons on enum values, i.e. not to assume that "x !=
- UE_ALWAYS" means "(x == UE_HEAD || x || UE_NORMAL)".
+ case entries in "HEAD" reflog, 2010-04-09) to use a "switch" statement
+ with an exhaustive list of "case" statements instead of doing numeric
+ comparisons against the enum labels.
- That assumption is true now, but we'd introduce subtle bugs here if
- that were to change, now the compiler will notice and error out on
- such errors.
+ Now we won't assume that "x != UE_ALWAYS" means "(x == UE_HEAD || x ||
+ UE_NORMAL)". That assumption is true now, but we'd introduce subtle
+ bugs here if that were to change, now the compiler will notice and
+ error out on such errors.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
5: 39263cd00ae ! 5: 896ae9f73eb reflog expire: refactor & use "tip_commit" only for UE_NORMAL
@@ Commit message
The code behaves the same way as before, but this makes the control
flow clearer, and the shorter name allows us to fold a 4-line i/else
- int a one-line terany instead.
+ into a one-line ternary instead.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
6: c71aab5845e = 6: cfa80e84c6d reflog expire: don't use lookup_commit_reference_gently()
7: 2fb33ef2546 = 7: b3a62b9b177 reflog: reduce scope of "struct rev_info"
8: f9fe6a2cfb0 = 8: 6748298a782 refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN
9: 28aa0aa6e30 ! 9: 2fb9de8ae51 reflog + refs-backend: move "verbose" out of the backend
@@ builtin/reflog.c: struct expire_reflog_policy_cb {
struct cmd_reflog_expire_cb cmd;
struct commit *tip_commit;
struct commit_list *tips;
-+ unsigned int dry_run:1,
-+ verbose:1;
++ unsigned int dry_run:1;
};
struct worktree_reflogs {
@@ builtin/reflog.c: static int should_expire_reflog_ent(struct object_id *ooid, struct object_id *no
- struct commit *old_commit, *new_commit;
-
- if (timestamp < cb->cmd.expire_total)
-- return 1;
-+ goto expire;
-
- old_commit = new_commit = NULL;
- if (cb->cmd.stalefix &&
- (!keep_entry(&old_commit, ooid) || !keep_entry(&new_commit, noid)))
-- return 1;
-+ goto expire;
-
- if (timestamp < cb->cmd.expire_unreachable) {
- switch (cb->unreachable_expire_kind) {
- case UE_ALWAYS:
-- return 1;
-+ goto expire;
- case UE_NORMAL:
- case UE_HEAD:
- if (unreachable(cb, old_commit, ooid) || unreachable(cb, new_commit, noid))
-- return 1;
-+ goto expire;
- break;
- }
- }
+ return 0;
+ }
- if (cb->cmd.recno && --(cb->cmd.recno) == 0)
-- return 1;
-+ goto expire;
++static int should_expire_reflog_ent_verbose(struct object_id *ooid,
++ struct object_id *noid,
++ const char *email,
++ timestamp_t timestamp, int tz,
++ const char *message, void *cb_data)
++{
++ struct expire_reflog_policy_cb *cb = cb_data;
++ int expire;
+
-+ if (cb->verbose)
++ expire = should_expire_reflog_ent(ooid, noid, email, timestamp, tz,
++ message, cb);
++
++ if (!expire)
+ printf("keep %s", message);
-
- return 0;
-+expire:
-+ if (cb->dry_run)
++ else if (cb->dry_run)
+ printf("would prune %s", message);
-+ else if (cb->verbose)
++ else
+ printf("prune %s", message);
-+ return 1;
- }
-
++
++ return expire;
++}
++
static int push_tip_to_list(const char *refname, const struct object_id *oid,
+ int flags, void *cb_data)
+ {
@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
int i, status, do_all, all_worktrees = 1;
int explicit_expiry = 0;
unsigned int flags = 0;
+ int verbose = 0;
++ reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
default_reflog_expire_unreachable = now - 30 * 24 * 3600;
default_reflog_expire = now - 90 * 24 * 3600;
@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, cons
else if (!strcmp(arg, "--")) {
i++;
break;
+@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
+ break;
+ }
+
++ if (verbose)
++ should_prune_fn = should_expire_reflog_ent_verbose;
++
+ /*
+ * We can trust the commits and objects reachable from refs
+ * even in older repository. We cannot trust what's reachable
@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
revs.do_not_die_on_missing_tree = 1;
revs.ignore_missing = 1;
@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, cons
+ struct expire_reflog_policy_cb cb = {
+ .cmd = cmd,
+ .dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
-+ .verbose = verbose,
+ };
set_reflog_expiry_param(&cb.cmd, explicit_expiry, item->string);
status |= reflog_expire(item->string, flags,
+ reflog_expiry_prepare,
+- should_expire_reflog_ent,
++ should_prune_fn,
+ reflog_expiry_cleanup,
+ &cb);
+ }
+@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
+ set_reflog_expiry_param(&cb.cmd, explicit_expiry, ref);
+ status |= reflog_expire(ref, flags,
+ reflog_expiry_prepare,
+- should_expire_reflog_ent,
++ should_prune_fn,
+ reflog_expiry_cleanup,
+ &cb);
+ free(ref);
@@ builtin/reflog.c: static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
struct cmd_reflog_expire_cb cmd = { 0 };
int i, status = 0;
unsigned int flags = 0;
+ int verbose = 0;
++ reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
for (i = 1; i < argc; i++) {
const char *arg = argv[i];
@@ builtin/reflog.c: static int cmd_reflog_delete(int argc, const char **argv, cons
else if (!strcmp(arg, "--")) {
i++;
break;
+@@ builtin/reflog.c: static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
+ break;
+ }
+
++ if (verbose)
++ should_prune_fn = should_expire_reflog_ent_verbose;
++
+ if (argc - i < 1)
+ return error(_("no reflog specified to delete"));
+
@@ builtin/reflog.c: static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
const char *spec = strstr(argv[i], "@{");
char *ep, *ref;
int recno;
- struct expire_reflog_policy_cb cb = { 0 };
+ struct expire_reflog_policy_cb cb = {
-+ .verbose = verbose,
+ .dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
+ };
if (!spec) {
status |= error(_("not a reflog: %s"), argv[i]);
+@@ builtin/reflog.c: static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
+ cb.cmd = cmd;
+ status |= reflog_expire(ref, flags,
+ reflog_expiry_prepare,
+- should_expire_reflog_ent,
++ should_prune_fn,
+ reflog_expiry_cleanup,
+ &cb);
+ free(ref);
## refs.h ##
@@ refs.h: enum ref_type ref_type(const char *refname);
--
2.34.1.1146.gb52885e7c44
next prev parent reply other threads:[~2021-12-22 4:07 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-30 21:38 [PATCH 00/12] reflog + gc: refactor, progress output, reftable-readyness Ævar Arnfjörð Bjarmason
2021-11-30 21:38 ` [PATCH 01/12] reflog delete: narrow scope of "cmd" passed to count_reflog_ent() Ævar Arnfjörð Bjarmason
2021-11-30 21:38 ` [PATCH 02/12] reflog expire: narrow scope of "cb" in cmd_reflog_expire() Ævar Arnfjörð Bjarmason
2021-11-30 21:38 ` [PATCH 03/12] reflog: change one->many worktree->refnames to use a string_list Ævar Arnfjörð Bjarmason
2021-11-30 21:38 ` [PATCH 04/12] reflog expire: don't do negative comparison on enum values Ævar Arnfjörð Bjarmason
2021-11-30 21:38 ` [PATCH 05/12] reflog expire: refactor & use "tip_commit" only for UE_NORMAL Ævar Arnfjörð Bjarmason
2021-11-30 21:38 ` [PATCH 06/12] reflog expire: don't use lookup_commit_reference_gently() Ævar Arnfjörð Bjarmason
2021-11-30 21:38 ` [PATCH 07/12] reflog: reduce scope of "struct rev_info" Ævar Arnfjörð Bjarmason
2021-11-30 21:38 ` [PATCH 08/12] refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN Ævar Arnfjörð Bjarmason
2021-11-30 21:38 ` [PATCH 09/12] reflog + refs-backend: move "verbose" out of the backend Ævar Arnfjörð Bjarmason
2021-11-30 21:38 ` [PATCH 10/12] reflog expire: add progress output on --stale-fix Ævar Arnfjörð Bjarmason
2021-11-30 21:38 ` [PATCH 11/12] gc + reflog: emit progress output from "reflog expire --all" Ævar Arnfjörð Bjarmason
2021-11-30 21:38 ` [PATCH 12/12] gc + reflog: don't stall before initial "git gc" progress output Ævar Arnfjörð Bjarmason
2021-12-16 13:45 ` [PATCH v2 0/9] reftable prep: have builtin/reflog.c handle "--verbose" Ævar Arnfjörð Bjarmason
2021-12-16 13:45 ` [PATCH v2 1/9] reflog delete: narrow scope of "cmd" passed to count_reflog_ent() Ævar Arnfjörð Bjarmason
2021-12-16 13:45 ` [PATCH v2 2/9] reflog expire: narrow scope of "cb" in cmd_reflog_expire() Ævar Arnfjörð Bjarmason
2021-12-16 13:45 ` [PATCH v2 3/9] reflog: change one->many worktree->refnames to use a string_list Ævar Arnfjörð Bjarmason
2021-12-16 13:45 ` [PATCH v2 4/9] reflog expire: don't do negative comparison on enum values Ævar Arnfjörð Bjarmason
2021-12-16 13:45 ` [PATCH v2 5/9] reflog expire: refactor & use "tip_commit" only for UE_NORMAL Ævar Arnfjörð Bjarmason
2021-12-16 13:45 ` [PATCH v2 6/9] reflog expire: don't use lookup_commit_reference_gently() Ævar Arnfjörð Bjarmason
2021-12-16 13:45 ` [PATCH v2 7/9] reflog: reduce scope of "struct rev_info" Ævar Arnfjörð Bjarmason
2021-12-16 13:45 ` [PATCH v2 8/9] refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN Ævar Arnfjörð Bjarmason
2021-12-16 13:45 ` [PATCH v2 9/9] reflog + refs-backend: move "verbose" out of the backend Ævar Arnfjörð Bjarmason
2021-12-16 22:27 ` [PATCH v2 0/9] reftable prep: have builtin/reflog.c handle "--verbose" Junio C Hamano
2021-12-16 23:31 ` Ævar Arnfjörð Bjarmason
2021-12-21 14:24 ` Han-Wen Nienhuys
2021-12-21 15:21 ` Ævar Arnfjörð Bjarmason
2021-12-22 4:06 ` Ævar Arnfjörð Bjarmason [this message]
2021-12-22 4:06 ` [PATCH v3 1/9] reflog delete: narrow scope of "cmd" passed to count_reflog_ent() Ævar Arnfjörð Bjarmason
2021-12-22 4:06 ` [PATCH v3 2/9] reflog expire: narrow scope of "cb" in cmd_reflog_expire() Ævar Arnfjörð Bjarmason
2021-12-22 4:06 ` [PATCH v3 3/9] reflog: change one->many worktree->refnames to use a string_list Ævar Arnfjörð Bjarmason
2021-12-22 4:06 ` [PATCH v3 4/9] reflog expire: use "switch" over enum values Ævar Arnfjörð Bjarmason
2021-12-22 4:06 ` [PATCH v3 5/9] reflog expire: refactor & use "tip_commit" only for UE_NORMAL Ævar Arnfjörð Bjarmason
2021-12-22 4:06 ` [PATCH v3 6/9] reflog expire: don't use lookup_commit_reference_gently() Ævar Arnfjörð Bjarmason
2021-12-22 4:06 ` [PATCH v3 7/9] reflog: reduce scope of "struct rev_info" Ævar Arnfjörð Bjarmason
2021-12-22 4:06 ` [PATCH v3 8/9] refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN Ævar Arnfjörð Bjarmason
2021-12-22 4:06 ` [PATCH v3 9/9] reflog + refs-backend: move "verbose" out of the backend Ævar Arnfjörð Bjarmason
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cover-v3-0.9-00000000000-20211222T040557Z-avarab@gmail.com \
--to=avarab@gmail.com \
--cc=bagasdotme@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=hanwen@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).