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