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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 22, 2022, 10:51 UTC
Message-ID: <220222.86fsob88h7.gmgdl@evledraar.gmail.com>
In-Reply-To: <DAF4762E-6376-44BC-9E8C-B9C32B65D92B@gmail.com>

On Mon, Feb 21 2022, John Cai wrote:

Show 91 quoted lines

> 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@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).
>
> 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!

I can check it out, but to make that easier can you share the WIP diff you have for getting the test setup that far?

I think you mean that it munges the SHA-1 on the LHS of .git/refs/logs/stash, I tried to do that now locally and I didn't come up with some way where you could observably make "git stash show", "git stash list" etc. show anything different.

I.e. it'll just promiscuously show whatever OID is on the RHS, even in cases where some of them are turned into "deadbeef..." (i.e. "list", but "show" will die).

So maybe it's just observable with a subsequent "git fsck", but I just tried composing a reflog with these entries:

    0000000000000000000000000000000000000000 A
    A B
    B C

And then manually changing it to:

    0000000000000000000000000000000000000000 A
    A B
    A C

Which "git fsck" will pass, and only complain if that "A" isn't a valid OID at all.

Anyway, if there isn't a way to get fsck/reflog to spew it out, but we want to assert that it's correct wouldn't this catch it (will need to depend on REFFILES) (untested):

	cut -d' ' -f1-2 .git/logs/refs/stash >actual &&
	cat >expect <<-EOF &&
	$(test_oid zero) $(git rev-parse ...)
	$(git rev-parse ...) $(git rev-parse ...)
	EOF
	test_cmp expect actual

I.e. to simply run whatever operation we do now, check that the OIDs match what we expect, and which would be different if one of these flags wasn't given?

Show 6 quoted lines

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