Re: [PATCH 3/3] stash: call reflog_delete from reflog.c

> On Fri, Feb 18 2022, John Cai via GitGitGadget wrote:
>
>> From: John Cai <johncai86@gmail.com>
>>
>> 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@gmail.com>
>> Signed-off-by: John Cai <johncai86@gmail.com>
>
> 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).

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