[1/3] reflog: libify delete reflog function and helpers

Commit Message

John Cai Feb. 18, 2022, 6:40 p.m. UTC

From: John Cai <[e-mail]>

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 <[e-mail]>
Signed-off-by: John Cai <[e-mail]>
---
 Makefile         |   1 +
 builtin/reflog.c | 409 +-------------------------------------------
 reflog.c         | 432 +++++++++++++++++++++++++++++++++++++++++++++++
 reflog.h         |  49 ++++++
 4 files changed, 484 insertions(+), 407 deletions(-)
 create mode 100644 reflog.c
 create mode 100644 reflog.h

Comments

Ævar Arnfjörð Bjarmason Feb. 18, 2022, 7:10 p.m. UTC | #1

On Fri, Feb 18 2022, John Cai via GitGitGadget wrote:

> From: John Cai <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: John Cai <[e-mail]>

Yay! This looks great. Even though I'll need to deal with some conflicts
locally .. :)

> +int reflog_delete(const char *rev, int flags, int verbose)
> +{
> +	struct cmd_reflog_expire_cb cmd = { 0 };
> +	int status = 0;
> +	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
> +	const char *spec = strstr(rev, "@{");
> +	char *ep, *ref;
> +	int recno;
> +	struct expire_reflog_policy_cb cb = {
> +		.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
> +	};
> +
> +	if (verbose)
> +		should_prune_fn = should_expire_reflog_ent_verbose;
> +
> +	if (!spec) {
> +		status |= error(_("not a reflog: %s"), rev);
> +	}
> +
> +	if (!dwim_log(rev, spec - rev, NULL, &ref)) {
> +		status |= error(_("no reflog for '%s'"), rev);
> +	}

For these let's follow our usual style of not having braces for
single-line if's.

Buuuut in this case doing so will make the diff move detection less
useful for 1..2.

So probably best to leave it, or do some post-cleanup at the end maybe.

> +int reflog_delete(const char*, int, int);
> +void reflog_expiry_cleanup(void *);
> +void reflog_expiry_prepare(const char*, const struct object_id*,
> +			   void *);
> +int should_expire_reflog_ent(struct object_id *, struct object_id*,
> +				    const char *, timestamp_t, int,
> +				    const char *, void *);
> +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 *,
> +				     struct object_id *,
> +				     const char *,
> +				     timestamp_t, int,
> +				     const char *, void *);
> +#endif /* REFLOG_H */

Just a style preference, but I for one prefer the style where we retain
the parameter names, it helps to read these, especially when we add API
docs here.

Some of these are mis-indented. We align with the opening "(" with "\t"
= 8 chars, so e.g. 2x \t + 5 SP for the count_reflog_ent() arguments
etc.

Taylor Blau Feb. 18, 2022, 7:35 p.m. UTC | #2

On Fri, Feb 18, 2022 at 06:40:45PM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <[e-mail]>
>
> 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.

Sounds great. For other reviewers, looking at this with `--color-moved`
if you have the patches applied locally makes it much easier to see what
is going on here, which is basically:

  - All of the implementation that used to be in builtin/reflog.c moved
    to reflog.c.

  - The function signatures and structure declarations moved to
    reflog.h.

> 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.

As you mentioned, the `reflog_delete()` implementation is indeed new. It
looks like Ævar reviewed most of it already, so I'll take a look at his
message before I end up repeating everything he already said ;).

It's worth noting that the subsequent clean-up to rewrite
cmd_reflog_delete() in terms of your new `reflog_delete()` happens in
the subsequent commit. If you end up rerolling this series, mentioning
that here may be worthwhile.

One question that I had which I don't see answered already is what the
plan is for existing reflog-related functions that live in refs.h.
Should or will those functions be moved to the new reflog-specific
header, too?

Thanks,
Taylor

Taylor Blau Feb. 18, 2022, 7:39 p.m. UTC | #3

On Fri, Feb 18, 2022 at 08:10:07PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > +int reflog_delete(const char *rev, int flags, int verbose)
> > +{
> > +	struct cmd_reflog_expire_cb cmd = { 0 };
> > +	int status = 0;
> > +	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
> > +	const char *spec = strstr(rev, "@{");
> > +	char *ep, *ref;
> > +	int recno;
> > +	struct expire_reflog_policy_cb cb = {
> > +		.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
> > +	};
> > +
> > +	if (verbose)
> > +		should_prune_fn = should_expire_reflog_ent_verbose;
> > +
> > +	if (!spec) {
> > +		status |= error(_("not a reflog: %s"), rev);
> > +	}
> > +
> > +	if (!dwim_log(rev, spec - rev, NULL, &ref)) {
> > +		status |= error(_("no reflog for '%s'"), rev);
> > +	}
>
> For these let's follow our usual style of not having braces for
> single-line if's.
>
> Buuuut in this case doing so will make the diff move detection less
> useful for 1..2.
>
> So probably best to leave it, or do some post-cleanup at the end maybe.

Hmm. I don't think the diff detection mechanism would have an
opportunity to kick in here, since the code is added in one patch and
then removed in another. I think I may be missing what you're trying to
say here ;).

In any case, I don't think it's a huge deal if we can't accurately
colorize this with `--color-moved`, so I'd probably just as soon clean
up the style nits in this patch.

> > +int reflog_delete(const char*, int, int);
> > +void reflog_expiry_cleanup(void *);
> > +void reflog_expiry_prepare(const char*, const struct object_id*,
> > +			   void *);
> > +int should_expire_reflog_ent(struct object_id *, struct object_id*,
> > +				    const char *, timestamp_t, int,
> > +				    const char *, void *);
> > +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 *,
> > +				     struct object_id *,
> > +				     const char *,
> > +				     timestamp_t, int,
> > +				     const char *, void *);
> > +#endif /* REFLOG_H */
>
> Just a style preference, but I for one prefer the style where we retain
> the parameter names, it helps to read these, especially when we add API
> docs here.
>
> Some of these are mis-indented. We align with the opening "(" with "\t"
> = 8 chars, so e.g. 2x \t + 5 SP for the count_reflog_ent() arguments
> etc.

Thanks for noting on both.

Thanks,
Taylor

Ævar Arnfjörð Bjarmason Feb. 18, 2022, 7:48 p.m. UTC | #4

On Fri, Feb 18 2022, Taylor Blau wrote:

> On Fri, Feb 18, 2022 at 08:10:07PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> > +int reflog_delete(const char *rev, int flags, int verbose)
>> > +{
>> > +	struct cmd_reflog_expire_cb cmd = { 0 };
>> > +	int status = 0;
>> > +	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
>> > +	const char *spec = strstr(rev, "@{");
>> > +	char *ep, *ref;
>> > +	int recno;
>> > +	struct expire_reflog_policy_cb cb = {
>> > +		.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
>> > +	};
>> > +
>> > +	if (verbose)
>> > +		should_prune_fn = should_expire_reflog_ent_verbose;
>> > +
>> > +	if (!spec) {
>> > +		status |= error(_("not a reflog: %s"), rev);
>> > +	}
>> > +
>> > +	if (!dwim_log(rev, spec - rev, NULL, &ref)) {
>> > +		status |= error(_("no reflog for '%s'"), rev);
>> > +	}
>>
>> For these let's follow our usual style of not having braces for
>> single-line if's.
>>
>> Buuuut in this case doing so will make the diff move detection less
>> useful for 1..2.
>>
>> So probably best to leave it, or do some post-cleanup at the end maybe.
>
> Hmm. I don't think the diff detection mechanism would have an
> opportunity to kick in here, since the code is added in one patch and
> then removed in another. I think I may be missing what you're trying to
> say here ;).
>
> In any case, I don't think it's a huge deal if we can't accurately
> colorize this with `--color-moved`, so I'd probably just as soon clean
> up the style nits in this patch.

Yes, this should really be read in combination with my comment on 2/3 to
squash that commit into 1/3 to take full advantage of that, sorry.

Junio C Hamano Feb. 18, 2022, 8 p.m. UTC | #5

"John Cai via GitGitGadget" <[e-mail]> writes:

> diff --git a/reflog.h b/reflog.h
> new file mode 100644
> index 00000000000..e4a8a104f45
> --- /dev/null
> +++ b/reflog.h
> @@ -0,0 +1,49 @@
> +#ifndef REFLOG_H
> +#define REFLOG_H
> +
> +#include "cache.h"
> +#include "commit.h"
> +
> +/* Remember to update object flag allocation in object.h */
> +#define INCOMPLETE	(1u<<10)
> +#define STUDYING	(1u<<11)
> +#define REACHABLE	(1u<<12)

These names were unique enough back when they were part of internal
implementation detail inside builtin/reflog.c but it no longer is in
the scope it is visible.  builtin/stash.c (and whatever other things
that may want to deal with reflogs in the future) is about stash
entries and wants to have a way to differentiate these symbols from
others and hint that these are about reflog operation.

Perhaps prefix them with REFLOG_ or something?

Or, do we even NEED to expose these bits outside the implementation
of reflog.c?  If these three constants are used ONLY by reflog.c and
not by builtin/reflog.c (or builtin/stash.c), then moving them to
where they are used, i.e. in reflog.c, without exposing them to
others in reflog.h, would be a far better thing to do.  That way,
they can stay with their original name, without having to add a
differentiating prefix.

I also checked if any of the following struct's can be left as a
private implementation detail, but they are shared between reflog.c
and builtin/reflog.c and need to be in reflog.h



> +struct cmd_reflog_expire_cb {
> +	int stalefix;
> +	int explicit_expiry;
> +	timestamp_t expire_total;
> +	timestamp_t expire_unreachable;
> +	int recno;
> +};
> +
> +struct expire_reflog_policy_cb {
> +	enum {
> +		UE_NORMAL,
> +		UE_ALWAYS,
> +		UE_HEAD
> +	} unreachable_expire_kind;
> +	struct commit_list *mark_list;
> +	unsigned long mark_limit;
> +	struct cmd_reflog_expire_cb cmd;
> +	struct commit *tip_commit;
> +	struct commit_list *tips;
> +	unsigned int dry_run:1;
> +};
> +
> +int reflog_delete(const char*, int, int);
> +void reflog_expiry_cleanup(void *);
> +void reflog_expiry_prepare(const char*, const struct object_id*,
> +			   void *);
> +int should_expire_reflog_ent(struct object_id *, struct object_id*,
> +				    const char *, timestamp_t, int,
> +				    const char *, void *);
> +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 *,
> +				     struct object_id *,
> +				     const char *,
> +				     timestamp_t, int,
> +				     const char *, void *);
> +#endif /* REFLOG_H */

Junio C Hamano Feb. 18, 2022, 8:21 p.m. UTC | #6

"John Cai via GitGitGadget" <[e-mail]> writes:

> From: John Cai <[e-mail]>
>
> 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.

I do not quite get this step.  Yes, eventually cmd_reflog_delete()
and cmd_stash() can both call the new reflog_delete() function, but
at this step, cmd_reflog_delete() should ALREADY be able to call it,
but I see duplicated code.

Why?

> @@ -726,6 +320,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
>  	int i, status = 0;
>  	unsigned int flags = 0;
>  	int verbose = 0;
> +
>  	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
>  	const struct option options[] = {
>  		OPT_BIT(0, "dry-run", &flags, N_("do not actually prune any entries"),

In other words, why the only change this step makes to
cmd_reflog_delete() is a style change here, instead of replacing the
logic inside its "while loop" with a call to the new helper
function, which is introduced ...

> diff --git a/reflog.c b/reflog.c
> new file mode 100644
> index 00000000000..227ed83b3da
> --- /dev/null
> +++ b/reflog.c
> ...
> +int reflog_delete(const char *rev, int flags, int verbose)
> +{

... here?

> +	struct cmd_reflog_expire_cb cmd = { 0 };
> +	int status = 0;
> +	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
> +	const char *spec = strstr(rev, "@{");
> +	char *ep, *ref;
> +	int recno;
> +	struct expire_reflog_policy_cb cb = {
> +		.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
> +	};
> +
> +	if (verbose)
> +		should_prune_fn = should_expire_reflog_ent_verbose;
> +
> +	if (!spec) {
> +		status |= error(_("not a reflog: %s"), rev);
> +	}

If this were "we moved one iteration of an existing loop to a
separate function, while trying to keep what the existing code
looked like as pristine as possible", then it is fine, but it does
not look like that is what is going on (and I do not think it is
feasible to do so, as the original being an interation in a loop,
it has "continue" here, instead you'd need to arrange to return an
error from here to allow the caller to work in a similar way).

So, if the patch is adding an adjusted implementation by removing
"continue;" that is not suitable in the context of this helper, it
should remove the now-unnecessary {braces} around the single-statement
block.

But I think you should keep {braces} around, because the "if spec is
NULL" case should not be a single-statement block.  You'd need to
return or this code is buggy.

> +	if (!dwim_log(rev, spec - rev, NULL, &ref)) {
> +		status |= error(_("no reflog for '%s'"), rev);
> +	}

I already hinted that the previous "if spec is NULL" code is buggy
because it does not return to allow the original caller to keep
working as before.  Because you lack an early return there, you'll
end up calling dwim_log with spec==NULL here, which does not end
well.

Ævar Arnfjörð Bjarmason Feb. 19, 2022, 2:53 a.m. UTC | #7

On Fri, Feb 18 2022, Junio C Hamano wrote:

> "John Cai via GitGitGadget" <[e-mail]> writes:
>
>> diff --git a/reflog.h b/reflog.h
>> new file mode 100644
>> index 00000000000..e4a8a104f45
>> --- /dev/null
>> +++ b/reflog.h
>> @@ -0,0 +1,49 @@
>> +#ifndef REFLOG_H
>> +#define REFLOG_H
>> +
>> +#include "cache.h"
>> +#include "commit.h"
>> +
>> +/* Remember to update object flag allocation in object.h */
>> +#define INCOMPLETE	(1u<<10)
>> +#define STUDYING	(1u<<11)
>> +#define REACHABLE	(1u<<12)
>
> These names were unique enough back when they were part of internal
> implementation detail inside builtin/reflog.c but it no longer is in
> the scope it is visible.  builtin/stash.c (and whatever other things
> that may want to deal with reflogs in the future) is about stash
> entries and wants to have a way to differentiate these symbols from
> others and hint that these are about reflog operation.
>
> Perhaps prefix them with REFLOG_ or something?
>
> Or, do we even NEED to expose these bits outside the implementation
> of reflog.c?  If these three constants are used ONLY by reflog.c and
> not by builtin/reflog.c (or builtin/stash.c), then moving them to
> where they are used, i.e. in reflog.c, without exposing them to
> others in reflog.h, would be a far better thing to do.  That way,
> they can stay with their original name, without having to add a
> differentiating prefix.

No objection to this, but FWIW the general pattern for these object.h
flags is to use these un-prefixed names:

     git grep -A20 'Remember to update object flag allocation' | grep define

To be fair the only one that's really comparable is the revision.h ones,
but those are very non-namespace-y, with names like SEEN, ADDED, SHOWN
etc.

I'd be fine with just leaving these as-is, especially with compilers
warning about macro re-definitions.

Taylor Blau Feb. 19, 2022, 3:02 a.m. UTC | #8

On Sat, Feb 19, 2022 at 03:53:14AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> On Fri, Feb 18 2022, Junio C Hamano wrote:
>
> > "John Cai via GitGitGadget" <[e-mail]> writes:
> >
> >> diff --git a/reflog.h b/reflog.h
> >> new file mode 100644
> >> index 00000000000..e4a8a104f45
> >> --- /dev/null
> >> +++ b/reflog.h
> >> @@ -0,0 +1,49 @@
> >> +#ifndef REFLOG_H
> >> +#define REFLOG_H
> >> +
> >> +#include "cache.h"
> >> +#include "commit.h"
> >> +
> >> +/* Remember to update object flag allocation in object.h */
> >> +#define INCOMPLETE	(1u<<10)
> >> +#define STUDYING	(1u<<11)
> >> +#define REACHABLE	(1u<<12)
> >
> > These names were unique enough back when they were part of internal
> > implementation detail inside builtin/reflog.c but it no longer is in
> > the scope it is visible.  builtin/stash.c (and whatever other things
> > that may want to deal with reflogs in the future) is about stash
> > entries and wants to have a way to differentiate these symbols from
> > others and hint that these are about reflog operation.
> >
> > Perhaps prefix them with REFLOG_ or something?
> >
> > Or, do we even NEED to expose these bits outside the implementation
> > of reflog.c?  If these three constants are used ONLY by reflog.c and
> > not by builtin/reflog.c (or builtin/stash.c), then moving them to
> > where they are used, i.e. in reflog.c, without exposing them to
> > others in reflog.h, would be a far better thing to do.  That way,
> > they can stay with their original name, without having to add a
> > differentiating prefix.
>
> No objection to this, but FWIW the general pattern for these object.h
> flags is to use these un-prefixed names:
>
>      git grep -A20 'Remember to update object flag allocation' | grep define
>
> To be fair the only one that's really comparable is the revision.h ones,
> but those are very non-namespace-y, with names like SEEN, ADDED, SHOWN
> etc.
>
> I'd be fine with just leaving these as-is, especially with compilers
> warning about macro re-definitions.

I think I have a mild preference to avoid polluting the set of macros
with something like INCOMPLETE outside of reflog.c's compilation unit
when possible. Though I don't really feel strongly either way.

In any case, let's make sure to update object.h's flag allocation table,
which as of this patch still indicates that these bits belong to
"builtin/reflog.c" (instead of "reflog.h" or "reflog.c", depending on
what the outcome of this discussion is).

Thanks,
Taylor

Junio C Hamano Feb. 20, 2022, 7:49 a.m. UTC | #9

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

>> Perhaps prefix them with REFLOG_ or something?
>>
>> Or, do we even NEED to expose these bits outside the implementation
>> of reflog.c?  If these three constants are used ONLY by reflog.c and
>> not by builtin/reflog.c (or builtin/stash.c), then moving them to
>> where they are used, i.e. in reflog.c, without exposing them to
>> others in reflog.h, would be a far better thing to do.  That way,
>> they can stay with their original name, without having to add a
>> differentiating prefix.
>
> No objection to this, but FWIW the general pattern for these object.h
> flags is to use these un-prefixed names:
>
>      git grep -A20 'Remember to update object flag allocation' | grep define
>
> To be fair the only one that's really comparable is the revision.h ones,
> but those are very non-namespace-y, with names like SEEN, ADDED, SHOWN
> etc.

Yes, but I consider that revision.h owns the flag bits, and all
others that are local to a single *.c files borrow leftover bits
(and that is why the allocations can be overlapping).

And that is why my preference is to keep these within reflog.c if we
are separating it out of builtin/reflog.c to make sure they do not
pollute global namespace.

John Cai Feb. 21, 2022, 1:43 a.m. UTC | #10

Hey Taylor,

On 18 Feb 2022, at 14:35, Taylor Blau wrote:

> On Fri, Feb 18, 2022 at 06:40:45PM +0000, John Cai via GitGitGadget wrote:
>> From: John Cai <[e-mail]>
>>
>> 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.
>
> Sounds great. For other reviewers, looking at this with `--color-moved`
> if you have the patches applied locally makes it much easier to see what
> is going on here, which is basically:
>
>   - All of the implementation that used to be in builtin/reflog.c moved
>     to reflog.c.
>
>   - The function signatures and structure declarations moved to
>     reflog.h.
>
>> 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.
>
> As you mentioned, the `reflog_delete()` implementation is indeed new. It
> looks like Ævar reviewed most of it already, so I'll take a look at his
> message before I end up repeating everything he already said ;).
>
> It's worth noting that the subsequent clean-up to rewrite
> cmd_reflog_delete() in terms of your new `reflog_delete()` happens in
> the subsequent commit. If you end up rerolling this series, mentioning
> that here may be worthwhile.
>
> One question that I had which I don't see answered already is what the
> plan is for existing reflog-related functions that live in refs.h.
> Should or will those functions be moved to the new reflog-specific
> header, too?

Thanks for bringing ths up! Maybe this cleanup can be included in a separate
patch series since this one is mainly about getting rid of the shell-out in
stash.c. What do you think?

>
> Thanks,
> Taylor

Taylor Blau Feb. 21, 2022, 1:50 a.m. UTC | #11

On Sun, Feb 20, 2022 at 08:43:14PM -0500, John Cai wrote:
> > One question that I had which I don't see answered already is what the
> > plan is for existing reflog-related functions that live in refs.h.
> > Should or will those functions be moved to the new reflog-specific
> > header, too?
>
> Thanks for bringing ths up! Maybe this cleanup can be included in a separate
> patch series since this one is mainly about getting rid of the shell-out in
> stash.c. What do you think?

Yeah; I think that's fine. We don't need to get all reflog-related
functions moved into reflog.h in the first series, so I think it's fine
to move things as you discover them.

I was just wondering whether it was something you planned to do at some
point (as part of this series, or a future one).

Thanks,
Taylor

John Cai Feb. 23, 2022, 7:50 p.m. UTC | #12

Hi Taylor,

On 20 Feb 2022, at 20:50, Taylor Blau wrote:

> On Sun, Feb 20, 2022 at 08:43:14PM -0500, John Cai wrote:
>>> One question that I had which I don't see answered already is what the
>>> plan is for existing reflog-related functions that live in refs.h.
>>> Should or will those functions be moved to the new reflog-specific
>>> header, too?
>>
>> Thanks for bringing ths up! Maybe this cleanup can be included in a separate
>> patch series since this one is mainly about getting rid of the shell-out in
>> stash.c. What do you think?
>
> Yeah; I think that's fine. We don't need to get all reflog-related
> functions moved into reflog.h in the first series, so I think it's fine
> to move things as you discover them.
>
> I was just wondering whether it was something you planned to do at some
> point (as part of this series, or a future one).

Yes I will open a new series for this. I've been looking through refs.h, and as you
alluded to there are a number of reflog related functions. But, they also relate to refs.
I'm not sure which ones should go into reflog.h versus staying in refs.h. What are your
thoughts?

>
> Thanks,
> Taylor

Thanks!
John

Patch

diff --git a/Makefile b/Makefile
index 186f9ab6190..f1e295539ab 100644
--- a/Makefile
+++ b/Makefile
@@ -990,6 +990,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.o
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 85b838720c3..65198320cd2 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"
 
 static const char reflog_exists_usage[] =
 N_("git reflog exists <ref>");
@@ -18,404 +15,11 @@  N_("git reflog exists <ref>");
 static timestamp_t default_reflog_expire;
 static timestamp_t default_reflog_expire_unreachable;
 
-struct cmd_reflog_expire_cb {
-	int stalefix;
-	int explicit_expiry;
-	timestamp_t expire_total;
-	timestamp_t expire_unreachable;
-	int recno;
-};
-
-struct expire_reflog_policy_cb {
-	enum {
-		UE_NORMAL,
-		UE_ALWAYS,
-		UE_HEAD
-	} unreachable_expire_kind;
-	struct commit_list *mark_list;
-	unsigned long mark_limit;
-	struct cmd_reflog_expire_cb cmd;
-	struct commit *tip_commit;
-	struct commit_list *tips;
-	unsigned int dry_run:1;
-};
-
 struct worktree_reflogs {
 	struct worktree *worktree;
 	struct string_list reflogs;
 };
 
-/* Remember to update object flag allocation in object.h */
-#define INCOMPLETE	(1u<<10)
-#define STUDYING	(1u<<11)
-#define REACHABLE	(1u<<12)
-
-static int tree_is_complete(const struct object_id *oid)
-{
-	struct tree_desc desc;
-	struct name_entry entry;
-	int complete;
-	struct tree *tree;
-
-	tree = lookup_tree(the_repository, oid);
-	if (!tree)
-		return 0;
-	if (tree->object.flags & SEEN)
-		return 1;
-	if (tree->object.flags & INCOMPLETE)
-		return 0;
-
-	if (!tree->buffer) {
-		enum object_type type;
-		unsigned long size;
-		void *data = read_object_file(oid, &type, &size);
-		if (!data) {
-			tree->object.flags |= INCOMPLETE;
-			return 0;
-		}
-		tree->buffer = data;
-		tree->size = size;
-	}
-	init_tree_desc(&desc, tree->buffer, tree->size);
-	complete = 1;
-	while (tree_entry(&desc, &entry)) {
-		if (!has_object_file(&entry.oid) ||
-		    (S_ISDIR(entry.mode) && !tree_is_complete(&entry.oid))) {
-			tree->object.flags |= INCOMPLETE;
-			complete = 0;
-		}
-	}
-	free_tree_buffer(tree);
-
-	if (complete)
-		tree->object.flags |= SEEN;
-	return complete;
-}
-
-static int commit_is_complete(struct commit *commit)
-{
-	struct object_array study;
-	struct object_array found;
-	int is_incomplete = 0;
-	int i;
-
-	/* early return */
-	if (commit->object.flags & SEEN)
-		return 1;
-	if (commit->object.flags & INCOMPLETE)
-		return 0;
-	/*
-	 * Find all commits that are reachable and are not marked as
-	 * SEEN.  Then make sure the trees and blobs contained are
-	 * complete.  After that, mark these commits also as SEEN.
-	 * If some of the objects that are needed to complete this
-	 * commit are missing, mark this commit as INCOMPLETE.
-	 */
-	memset(&study, 0, sizeof(study));
-	memset(&found, 0, sizeof(found));
-	add_object_array(&commit->object, NULL, &study);
-	add_object_array(&commit->object, NULL, &found);
-	commit->object.flags |= STUDYING;
-	while (study.nr) {
-		struct commit *c;
-		struct commit_list *parent;
-
-		c = (struct commit *)object_array_pop(&study);
-		if (!c->object.parsed && !parse_object(the_repository, &c->object.oid))
-			c->object.flags |= INCOMPLETE;
-
-		if (c->object.flags & INCOMPLETE) {
-			is_incomplete = 1;
-			break;
-		}
-		else if (c->object.flags & SEEN)
-			continue;
-		for (parent = c->parents; parent; parent = parent->next) {
-			struct commit *p = parent->item;
-			if (p->object.flags & STUDYING)
-				continue;
-			p->object.flags |= STUDYING;
-			add_object_array(&p->object, NULL, &study);
-			add_object_array(&p->object, NULL, &found);
-		}
-	}
-	if (!is_incomplete) {
-		/*
-		 * make sure all commits in "found" array have all the
-		 * necessary objects.
-		 */
-		for (i = 0; i < found.nr; i++) {
-			struct commit *c =
-				(struct commit *)found.objects[i].item;
-			if (!tree_is_complete(get_commit_tree_oid(c))) {
-				is_incomplete = 1;
-				c->object.flags |= INCOMPLETE;
-			}
-		}
-		if (!is_incomplete) {
-			/* mark all found commits as complete, iow SEEN */
-			for (i = 0; i < found.nr; i++)
-				found.objects[i].item->flags |= SEEN;
-		}
-	}
-	/* clear flags from the objects we traversed */
-	for (i = 0; i < found.nr; i++)
-		found.objects[i].item->flags &= ~STUDYING;
-	if (is_incomplete)
-		commit->object.flags |= INCOMPLETE;
-	else {
-		/*
-		 * If we come here, we have (1) traversed the ancestry chain
-		 * from the "commit" until we reach SEEN commits (which are
-		 * known to be complete), and (2) made sure that the commits
-		 * encountered during the above traversal refer to trees that
-		 * are complete.  Which means that we know *all* the commits
-		 * we have seen during this process are complete.
-		 */
-		for (i = 0; i < found.nr; i++)
-			found.objects[i].item->flags |= SEEN;
-	}
-	/* free object arrays */
-	object_array_clear(&study);
-	object_array_clear(&found);
-	return !is_incomplete;
-}
-
-static int keep_entry(struct commit **it, struct object_id *oid)
-{
-	struct commit *commit;
-
-	if (is_null_oid(oid))
-		return 1;
-	commit = lookup_commit_reference_gently(the_repository, oid, 1);
-	if (!commit)
-		return 0;
-
-	/*
-	 * Make sure everything in this commit exists.
-	 *
-	 * We have walked all the objects reachable from the refs
-	 * and cache earlier.  The commits reachable by this commit
-	 * must meet SEEN commits -- and then we should mark them as
-	 * SEEN as well.
-	 */
-	if (!commit_is_complete(commit))
-		return 0;
-	*it = commit;
-	return 1;
-}
-
-/*
- * Starting from commits in the cb->mark_list, mark commits that are
- * reachable from them.  Stop the traversal at commits older than
- * the expire_limit and queue them back, so that the caller can call
- * us again to restart the traversal with longer expire_limit.
- */
-static void mark_reachable(struct expire_reflog_policy_cb *cb)
-{
-	struct commit_list *pending;
-	timestamp_t expire_limit = cb->mark_limit;
-	struct commit_list *leftover = NULL;
-
-	for (pending = cb->mark_list; pending; pending = pending->next)
-		pending->item->object.flags &= ~REACHABLE;
-
-	pending = cb->mark_list;
-	while (pending) {
-		struct commit_list *parent;
-		struct commit *commit = pop_commit(&pending);
-		if (commit->object.flags & REACHABLE)
-			continue;
-		if (parse_commit(commit))
-			continue;
-		commit->object.flags |= REACHABLE;
-		if (commit->date < expire_limit) {
-			commit_list_insert(commit, &leftover);
-			continue;
-		}
-		commit->object.flags |= REACHABLE;
-		parent = commit->parents;
-		while (parent) {
-			commit = parent->item;
-			parent = parent->next;
-			if (commit->object.flags & REACHABLE)
-				continue;
-			commit_list_insert(commit, &pending);
-		}
-	}
-	cb->mark_list = leftover;
-}
-
-static int unreachable(struct expire_reflog_policy_cb *cb, struct commit *commit, struct object_id *oid)
-{
-	/*
-	 * We may or may not have the commit yet - if not, look it
-	 * up using the supplied sha1.
-	 */
-	if (!commit) {
-		if (is_null_oid(oid))
-			return 0;
-
-		commit = lookup_commit_reference_gently(the_repository, oid,
-							1);
-
-		/* Not a commit -- keep it */
-		if (!commit)
-			return 0;
-	}
-
-	/* Reachable from the current ref?  Don't prune. */
-	if (commit->object.flags & REACHABLE)
-		return 0;
-
-	if (cb->mark_list && cb->mark_limit) {
-		cb->mark_limit = 0; /* dig down to the root */
-		mark_reachable(cb);
-	}
-
-	return !(commit->object.flags & REACHABLE);
-}
-
-/*
- * Return true iff the specified reflog entry should be expired.
- */
-static 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)
-{
-	struct expire_reflog_policy_cb *cb = cb_data;
-	struct commit *old_commit, *new_commit;
-
-	if (timestamp < cb->cmd.expire_total)
-		return 1;
-
-	old_commit = new_commit = NULL;
-	if (cb->cmd.stalefix &&
-	    (!keep_entry(&old_commit, ooid) || !keep_entry(&new_commit, noid)))
-		return 1;
-
-	if (timestamp < cb->cmd.expire_unreachable) {
-		switch (cb->unreachable_expire_kind) {
-		case UE_ALWAYS:
-			return 1;
-		case UE_NORMAL:
-		case UE_HEAD:
-			if (unreachable(cb, old_commit, ooid) || unreachable(cb, new_commit, noid))
-				return 1;
-			break;
-		}
-	}
-
-	if (cb->cmd.recno && --(cb->cmd.recno) == 0)
-		return 1;
-
-	return 0;
-}
-
-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;
-
-	expire = should_expire_reflog_ent(ooid, noid, email, timestamp, tz,
-					  message, cb);
-
-	if (!expire)
-		printf("keep %s", message);
-	else if (cb->dry_run)
-		printf("would prune %s", message);
-	else
-		printf("prune %s", message);
-
-	return expire;
-}
-
-static int push_tip_to_list(const char *refname, const struct object_id *oid,
-			    int flags, void *cb_data)
-{
-	struct commit_list **list = cb_data;
-	struct commit *tip_commit;
-	if (flags & REF_ISSYMREF)
-		return 0;
-	tip_commit = lookup_commit_reference_gently(the_repository, oid, 1);
-	if (!tip_commit)
-		return 0;
-	commit_list_insert(tip_commit, list);
-	return 0;
-}
-
-static int is_head(const char *refname)
-{
-	switch (ref_type(refname)) {
-	case REF_TYPE_OTHER_PSEUDOREF:
-	case REF_TYPE_MAIN_PSEUDOREF:
-		if (parse_worktree_ref(refname, NULL, NULL, &refname))
-			BUG("not a worktree ref: %s", refname);
-		break;
-	default:
-		break;
-	}
-	return !strcmp(refname, "HEAD");
-}
-
-static void reflog_expiry_prepare(const char *refname,
-				  const struct object_id *oid,
-				  void *cb_data)
-{
-	struct expire_reflog_policy_cb *cb = cb_data;
-	struct commit_list *elem;
-	struct commit *commit = NULL;
-
-	if (!cb->cmd.expire_unreachable || is_head(refname)) {
-		cb->unreachable_expire_kind = UE_HEAD;
-	} else {
-		commit = lookup_commit(the_repository, oid);
-		cb->unreachable_expire_kind = commit ? UE_NORMAL : UE_ALWAYS;
-	}
-
-	if (cb->cmd.expire_unreachable <= cb->cmd.expire_total)
-		cb->unreachable_expire_kind = UE_ALWAYS;
-
-	switch (cb->unreachable_expire_kind) {
-	case UE_ALWAYS:
-		return;
-	case UE_HEAD:
-		for_each_ref(push_tip_to_list, &cb->tips);
-		for (elem = cb->tips; elem; elem = elem->next)
-			commit_list_insert(elem->item, &cb->mark_list);
-		break;
-	case UE_NORMAL:
-		commit_list_insert(commit, &cb->mark_list);
-		/* For reflog_expiry_cleanup() below */
-		cb->tip_commit = commit;
-	}
-	cb->mark_limit = cb->cmd.expire_total;
-	mark_reachable(cb);
-}
-
-static void reflog_expiry_cleanup(void *cb_data)
-{
-	struct expire_reflog_policy_cb *cb = cb_data;
-	struct commit_list *elem;
-
-	switch (cb->unreachable_expire_kind) {
-	case UE_ALWAYS:
-		return;
-	case UE_HEAD:
-		for (elem = cb->tips; elem; elem = elem->next)
-			clear_commit_marks(elem->item, REACHABLE);
-		free_commit_list(cb->tips);
-		break;
-	case UE_NORMAL:
-		clear_commit_marks(cb->tip_commit, REACHABLE);
-		break;
-	}
-}
-
 static int collect_reflog(const char *ref, const struct object_id *oid, int unused, void *cb_data)
 {
 	struct worktree_reflogs *cb = cb_data;
@@ -704,16 +308,6 @@  static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	return status;
 }
 
-static 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)
-{
-	struct cmd_reflog_expire_cb *cb = cb_data;
-	if (!cb->expire_total || timestamp < cb->expire_total)
-		cb->recno++;
-	return 0;
-}
-
 static const char * reflog_delete_usage[] = {
 	N_("git reflog delete [--rewrite] [--updateref] "
 	   "[--dry-run | -n] [--verbose] <refs>..."),
@@ -726,6 +320,7 @@  static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 	int i, status = 0;
 	unsigned int flags = 0;
 	int verbose = 0;
+
 	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
 	const struct option options[] = {
 		OPT_BIT(0, "dry-run", &flags, N_("do not actually prune any entries"),
diff --git a/reflog.c b/reflog.c
new file mode 100644
index 00000000000..227ed83b3da
--- /dev/null
+++ b/reflog.c
@@ -0,0 +1,432 @@ 
+#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"
+
+static int tree_is_complete(const struct object_id *oid)
+{
+	struct tree_desc desc;
+	struct name_entry entry;
+	int complete;
+	struct tree *tree;
+
+	tree = lookup_tree(the_repository, oid);
+	if (!tree)
+		return 0;
+	if (tree->object.flags & SEEN)
+		return 1;
+	if (tree->object.flags & INCOMPLETE)
+		return 0;
+
+	if (!tree->buffer) {
+		enum object_type type;
+		unsigned long size;
+		void *data = read_object_file(oid, &type, &size);
+		if (!data) {
+			tree->object.flags |= INCOMPLETE;
+			return 0;
+		}
+		tree->buffer = data;
+		tree->size = size;
+	}
+	init_tree_desc(&desc, tree->buffer, tree->size);
+	complete = 1;
+	while (tree_entry(&desc, &entry)) {
+		if (!has_object_file(&entry.oid) ||
+		    (S_ISDIR(entry.mode) && !tree_is_complete(&entry.oid))) {
+			tree->object.flags |= INCOMPLETE;
+			complete = 0;
+		}
+	}
+	free_tree_buffer(tree);
+
+	if (complete)
+		tree->object.flags |= SEEN;
+	return complete;
+}
+
+static int commit_is_complete(struct commit *commit)
+{
+	struct object_array study;
+	struct object_array found;
+	int is_incomplete = 0;
+	int i;
+
+	/* early return */
+	if (commit->object.flags & SEEN)
+		return 1;
+	if (commit->object.flags & INCOMPLETE)
+		return 0;
+	/*
+	 * Find all commits that are reachable and are not marked as
+	 * SEEN.  Then make sure the trees and blobs contained are
+	 * complete.  After that, mark these commits also as SEEN.
+	 * If some of the objects that are needed to complete this
+	 * commit are missing, mark this commit as INCOMPLETE.
+	 */
+	memset(&study, 0, sizeof(study));
+	memset(&found, 0, sizeof(found));
+	add_object_array(&commit->object, NULL, &study);
+	add_object_array(&commit->object, NULL, &found);
+	commit->object.flags |= STUDYING;
+	while (study.nr) {
+		struct commit *c;
+		struct commit_list *parent;
+
+		c = (struct commit *)object_array_pop(&study);
+		if (!c->object.parsed && !parse_object(the_repository, &c->object.oid))
+			c->object.flags |= INCOMPLETE;
+
+		if (c->object.flags & INCOMPLETE) {
+			is_incomplete = 1;
+			break;
+		}
+		else if (c->object.flags & SEEN)
+			continue;
+		for (parent = c->parents; parent; parent = parent->next) {
+			struct commit *p = parent->item;
+			if (p->object.flags & STUDYING)
+				continue;
+			p->object.flags |= STUDYING;
+			add_object_array(&p->object, NULL, &study);
+			add_object_array(&p->object, NULL, &found);
+		}
+	}
+	if (!is_incomplete) {
+		/*
+		 * make sure all commits in "found" array have all the
+		 * necessary objects.
+		 */
+		for (i = 0; i < found.nr; i++) {
+			struct commit *c =
+				(struct commit *)found.objects[i].item;
+			if (!tree_is_complete(get_commit_tree_oid(c))) {
+				is_incomplete = 1;
+				c->object.flags |= INCOMPLETE;
+			}
+		}
+		if (!is_incomplete) {
+			/* mark all found commits as complete, iow SEEN */
+			for (i = 0; i < found.nr; i++)
+				found.objects[i].item->flags |= SEEN;
+		}
+	}
+	/* clear flags from the objects we traversed */
+	for (i = 0; i < found.nr; i++)
+		found.objects[i].item->flags &= ~STUDYING;
+	if (is_incomplete)
+		commit->object.flags |= INCOMPLETE;
+	else {
+		/*
+		 * If we come here, we have (1) traversed the ancestry chain
+		 * from the "commit" until we reach SEEN commits (which are
+		 * known to be complete), and (2) made sure that the commits
+		 * encountered during the above traversal refer to trees that
+		 * are complete.  Which means that we know *all* the commits
+		 * we have seen during this process are complete.
+		 */
+		for (i = 0; i < found.nr; i++)
+			found.objects[i].item->flags |= SEEN;
+	}
+	/* free object arrays */
+	object_array_clear(&study);
+	object_array_clear(&found);
+	return !is_incomplete;
+}
+
+static int keep_entry(struct commit **it, struct object_id *oid)
+{
+	struct commit *commit;
+
+	if (is_null_oid(oid))
+		return 1;
+	commit = lookup_commit_reference_gently(the_repository, oid, 1);
+	if (!commit)
+		return 0;
+
+	/*
+	 * Make sure everything in this commit exists.
+	 *
+	 * We have walked all the objects reachable from the refs
+	 * and cache earlier.  The commits reachable by this commit
+	 * must meet SEEN commits -- and then we should mark them as
+	 * SEEN as well.
+	 */
+	if (!commit_is_complete(commit))
+		return 0;
+	*it = commit;
+	return 1;
+}
+
+/*
+ * Starting from commits in the cb->mark_list, mark commits that are
+ * reachable from them.  Stop the traversal at commits older than
+ * the expire_limit and queue them back, so that the caller can call
+ * us again to restart the traversal with longer expire_limit.
+ */
+static void mark_reachable(struct expire_reflog_policy_cb *cb)
+{
+	struct commit_list *pending;
+	timestamp_t expire_limit = cb->mark_limit;
+	struct commit_list *leftover = NULL;
+
+	for (pending = cb->mark_list; pending; pending = pending->next)
+		pending->item->object.flags &= ~REACHABLE;
+
+	pending = cb->mark_list;
+	while (pending) {
+		struct commit_list *parent;
+		struct commit *commit = pop_commit(&pending);
+		if (commit->object.flags & REACHABLE)
+			continue;
+		if (parse_commit(commit))
+			continue;
+		commit->object.flags |= REACHABLE;
+		if (commit->date < expire_limit) {
+			commit_list_insert(commit, &leftover);
+			continue;
+		}
+		commit->object.flags |= REACHABLE;
+		parent = commit->parents;
+		while (parent) {
+			commit = parent->item;
+			parent = parent->next;
+			if (commit->object.flags & REACHABLE)
+				continue;
+			commit_list_insert(commit, &pending);
+		}
+	}
+	cb->mark_list = leftover;
+}
+
+static int unreachable(struct expire_reflog_policy_cb *cb, struct commit *commit, struct object_id *oid)
+{
+	/*
+	 * We may or may not have the commit yet - if not, look it
+	 * up using the supplied sha1.
+	 */
+	if (!commit) {
+		if (is_null_oid(oid))
+			return 0;
+
+		commit = lookup_commit_reference_gently(the_repository, oid,
+							1);
+
+		/* Not a commit -- keep it */
+		if (!commit)
+			return 0;
+	}
+
+	/* Reachable from the current ref?  Don't prune. */
+	if (commit->object.flags & REACHABLE)
+		return 0;
+
+	if (cb->mark_list && cb->mark_limit) {
+		cb->mark_limit = 0; /* dig down to the root */
+		mark_reachable(cb);
+	}
+
+	return !(commit->object.flags & REACHABLE);
+}
+
+/*
+ * Return true iff the specified reflog entry should be expired.
+ */
+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)
+{
+	struct expire_reflog_policy_cb *cb = cb_data;
+	struct commit *old_commit, *new_commit;
+
+	if (timestamp < cb->cmd.expire_total)
+		return 1;
+
+	old_commit = new_commit = NULL;
+	if (cb->cmd.stalefix &&
+	    (!keep_entry(&old_commit, ooid) || !keep_entry(&new_commit, noid)))
+		return 1;
+
+	if (timestamp < cb->cmd.expire_unreachable) {
+		switch (cb->unreachable_expire_kind) {
+		case UE_ALWAYS:
+			return 1;
+		case UE_NORMAL:
+		case UE_HEAD:
+			if (unreachable(cb, old_commit, ooid) || unreachable(cb, new_commit, noid))
+				return 1;
+			break;
+		}
+	}
+
+	if (cb->cmd.recno && --(cb->cmd.recno) == 0)
+		return 1;
+
+	return 0;
+}
+
+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;
+
+	expire = should_expire_reflog_ent(ooid, noid, email, timestamp, tz,
+					  message, cb);
+
+	if (!expire)
+		printf("keep %s", message);
+	else if (cb->dry_run)
+		printf("would prune %s", message);
+	else
+		printf("prune %s", message);
+
+	return expire;
+}
+
+static int push_tip_to_list(const char *refname, const struct object_id *oid,
+			    int flags, void *cb_data)
+{
+	struct commit_list **list = cb_data;
+	struct commit *tip_commit;
+	if (flags & REF_ISSYMREF)
+		return 0;
+	tip_commit = lookup_commit_reference_gently(the_repository, oid, 1);
+	if (!tip_commit)
+		return 0;
+	commit_list_insert(tip_commit, list);
+	return 0;
+}
+
+static int is_head(const char *refname)
+{
+	switch (ref_type(refname)) {
+	case REF_TYPE_OTHER_PSEUDOREF:
+	case REF_TYPE_MAIN_PSEUDOREF:
+		if (parse_worktree_ref(refname, NULL, NULL, &refname))
+			BUG("not a worktree ref: %s", refname);
+		break;
+	default:
+		break;
+	}
+	return !strcmp(refname, "HEAD");
+}
+
+void reflog_expiry_prepare(const char *refname,
+				  const struct object_id *oid,
+				  void *cb_data)
+{
+	struct expire_reflog_policy_cb *cb = cb_data;
+	struct commit_list *elem;
+	struct commit *commit = NULL;
+
+	if (!cb->cmd.expire_unreachable || is_head(refname)) {
+		cb->unreachable_expire_kind = UE_HEAD;
+	} else {
+		commit = lookup_commit(the_repository, oid);
+		cb->unreachable_expire_kind = commit ? UE_NORMAL : UE_ALWAYS;
+	}
+
+	if (cb->cmd.expire_unreachable <= cb->cmd.expire_total)
+		cb->unreachable_expire_kind = UE_ALWAYS;
+
+	switch (cb->unreachable_expire_kind) {
+	case UE_ALWAYS:
+		return;
+	case UE_HEAD:
+		for_each_ref(push_tip_to_list, &cb->tips);
+		for (elem = cb->tips; elem; elem = elem->next)
+			commit_list_insert(elem->item, &cb->mark_list);
+		break;
+	case UE_NORMAL:
+		commit_list_insert(commit, &cb->mark_list);
+		/* For reflog_expiry_cleanup() below */
+		cb->tip_commit = commit;
+	}
+	cb->mark_limit = cb->cmd.expire_total;
+	mark_reachable(cb);
+}
+
+void reflog_expiry_cleanup(void *cb_data)
+{
+	struct expire_reflog_policy_cb *cb = cb_data;
+	struct commit_list *elem;
+
+	switch (cb->unreachable_expire_kind) {
+	case UE_ALWAYS:
+		return;
+	case UE_HEAD:
+		for (elem = cb->tips; elem; elem = elem->next)
+			clear_commit_marks(elem->item, REACHABLE);
+		free_commit_list(cb->tips);
+		break;
+	case UE_NORMAL:
+		clear_commit_marks(cb->tip_commit, REACHABLE);
+		break;
+	}
+}
+
+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)
+{
+	struct cmd_reflog_expire_cb *cb = cb_data;
+	if (!cb->expire_total || timestamp < cb->expire_total)
+		cb->recno++;
+	return 0;
+}
+
+int reflog_delete(const char *rev, int flags, int verbose)
+{
+	struct cmd_reflog_expire_cb cmd = { 0 };
+	int status = 0;
+	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
+	const char *spec = strstr(rev, "@{");
+	char *ep, *ref;
+	int recno;
+	struct expire_reflog_policy_cb cb = {
+		.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
+	};
+
+	if (verbose)
+		should_prune_fn = should_expire_reflog_ent_verbose;
+
+	if (!spec) {
+		status |= error(_("not a reflog: %s"), rev);
+	}
+
+	if (!dwim_log(rev, spec - rev, NULL, &ref)) {
+		status |= error(_("no reflog for '%s'"), rev);
+	}
+
+	if (status)
+		return status;
+
+	recno = strtoul(spec + 2, &ep, 10);
+	if (*ep == '}') {
+		cmd.recno = -recno;
+		for_each_reflog_ent(ref, count_reflog_ent, &cmd);
+	} else {
+		cmd.expire_total = approxidate(spec + 2);
+		for_each_reflog_ent(ref, count_reflog_ent, &cmd);
+		cmd.expire_total = 0;
+	}
+
+	cb.cmd = cmd;
+	status |= reflog_expire(ref, flags,
+				reflog_expiry_prepare,
+				should_prune_fn,
+				reflog_expiry_cleanup,
+				&cb);
+	free(ref);
+
+	return status;
+}
diff --git a/reflog.h b/reflog.h
new file mode 100644
index 00000000000..e4a8a104f45
--- /dev/null
+++ b/reflog.h
@@ -0,0 +1,49 @@ 
+#ifndef REFLOG_H
+#define REFLOG_H
+
+#include "cache.h"
+#include "commit.h"
+
+/* Remember to update object flag allocation in object.h */
+#define INCOMPLETE	(1u<<10)
+#define STUDYING	(1u<<11)
+#define REACHABLE	(1u<<12)
+
+struct cmd_reflog_expire_cb {
+	int stalefix;
+	int explicit_expiry;
+	timestamp_t expire_total;
+	timestamp_t expire_unreachable;
+	int recno;
+};
+
+struct expire_reflog_policy_cb {
+	enum {
+		UE_NORMAL,
+		UE_ALWAYS,
+		UE_HEAD
+	} unreachable_expire_kind;
+	struct commit_list *mark_list;
+	unsigned long mark_limit;
+	struct cmd_reflog_expire_cb cmd;
+	struct commit *tip_commit;
+	struct commit_list *tips;
+	unsigned int dry_run:1;
+};
+
+int reflog_delete(const char*, int, int);
+void reflog_expiry_cleanup(void *);
+void reflog_expiry_prepare(const char*, const struct object_id*,
+			   void *);
+int should_expire_reflog_ent(struct object_id *, struct object_id*,
+				    const char *, timestamp_t, int,
+				    const char *, void *);
+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 *,
+				     struct object_id *,
+				     const char *,
+				     timestamp_t, int,
+				     const char *, void *);
+#endif /* REFLOG_H */