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

From: Taylor Blau <me@ttaylorr.com>
Date: Feb 18, 2022, 19:35 UTC
Message-ID: <Yg/09XYTruPJQVV/@nand.local>
In-Reply-To: <9e17ece8d8956c7fd41b7be2f5c0475b1f9af6ec.1645209647.git.gitgitgadget@gmail.com>

On Fri, Feb 18, 2022 at 06:40:45PM +0000, John Cai via GitGitGadget wrote:

Show 5 quoted lines

> From: John Cai <johncai86@gmail.com>
>
> 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