Hi Ævar,
On 18 Feb 2022, at 14:20, Ævar Arnfjörð Bjarmason wrote:
> On Fri, Feb 18 2022, John Cai via GitGitGadget wrote:
>
>> From: John Cai <johncai86@xxxxxxxxx>
>>
>> Now that cmd_reflog_delete has been libified an exported it into a new
>> reflog.c library so we can call it directly from builtin/stash.c. This
>> not only gives us a performance gain since we don't need to create a
>> subprocess, but it also allows us to use the ref transactions api in the
>> future.
>>
>> Helped-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
>> Signed-off-by: John Cai <johncai86@xxxxxxxxx>
>
> Very nicely done, and nice that despite the ~500 lines added/removed in
> the diffstat that the "actual" changes in this series are so small.q
>
>> @@ -635,18 +636,9 @@ static int reflog_is_empty(const char *refname)
>> static int do_drop_stash(struct stash_info *info, int quiet)
>> {
>> int ret;
>> - struct child_process cp_reflog = CHILD_PROCESS_INIT;
>> -
>
> Nit: We usually separate variables decls with a \n\n, as is done in the
> pre-image, but you end up dropping that.
>
>> - /*
>> - * reflog does not provide a simple function for deleting refs. One will
>> - * need to be added to avoid implementing too much reflog code here
>> - */
>> -
>> - cp_reflog.git_cmd = 1;
>> - strvec_pushl(&cp_reflog.args, "reflog", "delete", "--updateref",
>> - "--rewrite", NULL);
>> - strvec_push(&cp_reflog.args, info->revision.buf);
>> - ret = run_command(&cp_reflog);
>> + ret = reflog_delete(info->revision.buf,
>> + EXPIRE_REFLOGS_REWRITE | EXPIRE_REFLOGS_REWRITE,
>> + 0);
>> if (!ret) {
>> if (!quiet)
>> printf_ln(_("Dropped %s (%s)"), info->revision.buf,
>
> I think per the above squashing this in would be nice, i.e. you get rid
> of the long line & it'sclear that "ret" is not used for anything now:
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index d0967b3d3c3..7b939576720 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -635,11 +635,9 @@ static int reflog_is_empty(const char *refname)
>
> static int do_drop_stash(struct stash_info *info, int quiet)
> {
> - int ret;
> - ret = reflog_delete(info->revision.buf,
> - EXPIRE_REFLOGS_REWRITE | EXPIRE_REFLOGS_REWRITE,
> - 0);
> - if (!ret) {
> + unsigned int flags = EXPIRE_REFLOGS_REWRITE | EXPIRE_REFLOGS_REWRITE;
> +
> + if (!reflog_delete(info->revision.buf, flags, 0)) {
> if (!quiet)
> printf_ln(_("Dropped %s (%s)"), info->revision.buf,
> oid_to_hex(&info->w_commit));
>
> But, having written that I notice that we have *_REWRITE twice there, so
> I almost just carried forward a new bug in 3/3 when composing this :)
>
> So one should be EXPIRE_REFLOGS_UPDATE_REF, presumably.
>
> And perhaps it's a big pain, but that suggests that the code isn't
> either used at all, or that we're missing a test for it.
>
> So adding a prep commit to this series where we either drop it, or add
> the missing test would be a very nice addition.
>
> See my quite recent 5ac15ad2509 (reflog tests: add --updateref tests,
> 2021-10-16) for adding such tests, but in that case I just covered "git
> reflog" itself, not "git stash". Maybe we can just add something to that
> for-loop in t1417 (or similar for a new stash test).
So I was trying to write a test to exercise the reflog --updateref and --rewrite
cases. --updateref is pretty straightforward, but with --rewrite I noticed that
it rewrites the old sha in the .git/log/refs/stash file. But, I was having
trouble finding somewhere that read this value. The test could reach into this file
and check the literal contents, but wondering if there is a better way. Any help appreciated!
>
> Also, a s/unsigned int flags/enum expire_reflog_flags/ while we're at it
> would be very nice here, but that could be done as another small prep
> commit. I.e. it's *not* a new issue since cmd_reflog_delete() had it
> before, but when converting this to a documented API it would be very
> nice to have it reflect the actual type we end up using.
