Re: [PATCH v2 2/3] reflog: libify delete reflog function and helpers
From: John Cai <hidden>
Date: 2022-02-23 18:40:49
Hi Ævar, Thanks for the review! On 23 Feb 2022, at 4:02, Ævar Arnfjörð Bjarmason wrote:
On Tue, Feb 22 2022, John Cai via GitGitGadget wrote:quoted
From: John Cai <redacted> Currently stash shells out to reflog in order to delete refs. In an effort to reduce how much we shell out to a subprocess, libify the functionality that stash needs into reflog.c. Add a reflog_delete function that is pretty much the logic in the while loop in builtin/reflog.c cmd_reflog_delete(). This is a function that builtin/reflog.c and builtin/stash.c can both call. Also move functions needed by reflog_delete and export them. Helped-by: Ævar Arnfjörð Bjarmason [off-list ref] Signed-off-by: John Cai <redacted> --- Makefile | 1 + builtin/reflog.c | 451 +---------------------------------------------- object.h | 2 +- reflog.c | 435 +++++++++++++++++++++++++++++++++++++++++++++ reflog.h | 49 +++++ 5 files changed, 490 insertions(+), 448 deletions(-) create mode 100644 reflog.c create mode 100644 reflog.hdiff --git a/Makefile b/Makefile index 6f0b4b775fe..876d4dfd6cb 100644 --- a/Makefile +++ b/Makefile@@ -989,6 +989,7 @@ LIB_OBJS += rebase-interactive.o LIB_OBJS += rebase.o LIB_OBJS += ref-filter.o LIB_OBJS += reflog-walk.o +LIB_OBJS += reflog.o LIB_OBJS += refs.o LIB_OBJS += refs/debug.o LIB_OBJS += refs/files-backend.odiff --git a/builtin/reflog.c b/builtin/reflog.c index 85b838720c3..03d347e5832 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c@@ -1,16 +1,13 @@ #include "builtin.h" #include "config.h" #include "lockfile.h" -#include "object-store.h" #include "repository.h" -#include "commit.h" -#include "refs.h" #include "dir.h" -#include "tree-walk.h" #include "diff.h" #include "revision.h" #include "reachable.h" #include "worktree.h" +#include "reflog.h"[...]diff --git a/reflog.c b/reflog.c new file mode 100644 index 00000000000..8d57dc43503 --- /dev/null +++ b/reflog.c@@ -0,0 +1,435 @@ +#include "cache.h" +#include "commit.h" +#include "object-store.h" +#include "reachable.h" +#include "reflog.h" +#include "refs.h" +#include "revision.h" +#include "tree-walk.h" +#include "worktree.h"I think you missed some now-redundant headers, and copied over others we didn't need. This compiles for me with this on top:
Ah yeah, looks I left these in by mistake. Thanks for catching this.
quoted hunk ↗ jump to hunk
diff --git a/builtin/reflog.c b/builtin/reflog.c index 03d347e5832..940db196f62 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c@@ -1,9 +1,5 @@ #include "builtin.h" #include "config.h" -#include "lockfile.h" -#include "repository.h" -#include "dir.h" -#include "diff.h" #include "revision.h" #include "reachable.h" #include "worktree.h"diff --git a/reflog.c b/reflog.c index 8d57dc43503..333fd8708fe 100644 --- a/reflog.c +++ b/reflog.c@@ -1,11 +1,8 @@ #include "cache.h" -#include "commit.h" #include "object-store.h" -#include "reachable.h" #include "reflog.h" #include "refs.h" #include "revision.h" -#include "tree-walk.h" #include "worktree.h"But perhaps some of those are really "needed" but brought in implicitly?quoted
[...]diff --git a/reflog.h b/reflog.h new file mode 100644 index 00000000000..3427021cdc2 --- /dev/null +++ b/reflog.h@@ -0,0 +1,49 @@ +#ifndef REFLOG_H +#define REFLOG_H + +#include "refs.h"Just a nit but I think the reflog_delete() should be wrapped (ends up at 80 cols), and the usual style in this project is to not whitespace-pad so much, i.e. this on top:
sounds good!
quoted hunk ↗ jump to hunk
diff --git a/reflog.h b/reflog.h index 3427021cdc2..d2906fb9f8d 100644 --- a/reflog.h +++ b/reflog.h@@ -1,6 +1,5 @@ #ifndef REFLOG_H #define REFLOG_H - #include "refs.h" struct cmd_reflog_expire_cb {@@ -25,25 +24,20 @@ struct expire_reflog_policy_cb { unsigned int dry_run:1; }; -int reflog_delete(const char *rev, enum expire_reflog_flags flags, int verbose); - +int reflog_delete(const char *rev, enum expire_reflog_flags flags, + int verbose); void reflog_expiry_cleanup(void *cb_data); - void reflog_expiry_prepare(const char *refname, const struct object_id *oid, void *cb_data); - int should_expire_reflog_ent(struct object_id *ooid, struct object_id *noid, const char *email, timestamp_t timestamp, int tz, const char *message, void *cb_data); - int count_reflog_ent(struct object_id *ooid, struct object_id *noid, const char *email, timestamp_t timestamp, int tz, const char *message, void *cb_data); - 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); - #endif /* REFLOG_H */Other than all that I really can't find anything at all to comment on, and I see that all the points raised in previous rounds by others were addressed.