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

[John Cai <johncai86@gmail.com>]

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