diff mbox series

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

Message ID bcc1eae053124f6e68f60a7a02cbee4744a022f0.1645209647.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series libify reflog | expand

Commit Message

John Cai Feb. 18, 2022, 6:40 p.m. UTC 
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>
---
 builtin/stash.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Feb. 18, 2022, 7:20 p.m. UTC | #1 
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.
Taylor Blau Feb. 19, 2022, 12:21 a.m. UTC | #2 
On Fri, Feb 18, 2022 at 08:20:13PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 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.

Thanks for pointing it out. Just thinking aloud here, the old code ran:

    git reflog delete --updateref --rewrite

where `--updateref` sets the `EXPIRE_REFLOGS_UPDATE_REF` bit and
`-rewrite` sets the `EXPIRE_REFLOGS_REWRITE` bit.

So yep, you and I have the same conclusion there, and one of these flags
should be the UPDATE_REF variant.

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

I think this is much more in the "lacking test coverage" category than
the "unused features we should remove" one.

Thanks,
Taylor
John Cai Feb. 22, 2022, 2:36 a.m. UTC | #3 
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!

>
> 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.
Ævar Arnfjörð Bjarmason Feb. 22, 2022, 10:51 a.m. UTC | #4 
On Mon, Feb 21 2022, John Cai wrote:

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

>>
>> 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.
diff mbox series

Patch

diff --git a/builtin/stash.c b/builtin/stash.c
index 9638c56303e..d0967b3d3c3 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -17,6 +17,7 @@ 
 #include "diffcore.h"
 #include "exec-cmd.h"
 #include "entry.h"
+#include "reflog.h"
 
 #define INCLUDE_ALL_FILES 2
 
@@ -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;
-
-	/*
-	 * 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,