[PATCH 1/2] reflog test: add more tests for 'reflog delete'

[PATCH 1/2] reflog test: add more tests for 'reflog delete'

[Pieter de Bie]

This adds more tests for 'reflog delete' and marks it as
broken, as currently a call to 'git reflog delete HEAD@{1}'
deletes entries in the currently checked out branch's log,
not the HEAD log.

Noticed by John Wiegley

Signed-off-by: Pieter de Bie <pdebie@ai.rug.nl>
---

	johnw on IRC noticed this. This adds a test that shows the
	problem. The next patch fixes the issue but I'm not sure of
	the implementation. Perhaps we just shouldn't resolve symbolic
	refs? I'm not really sure which functions to use then.

 t/t1410-reflog.sh |   22 ++++++++++++++++++----
 1 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 73f830d..3b9860e 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -175,7 +175,7 @@ test_expect_success 'recover and check' '
 
 '
 
-test_expect_success 'delete' '
+test_expect_failure 'delete' '
 	echo 1 > C &&
 	test_tick &&
 	git commit -m rat C &&
@@ -188,16 +188,30 @@ test_expect_success 'delete' '
 	test_tick &&
 	git commit -m tiger C &&
 
-	test 5 = $(git reflog | wc -l) &&
+	HEAD_entry_count=$(git reflog | wc -l)
+	master_entry_count=$(git reflog show master | wc -l)
+
+	test $HEAD_entry_count = 5 &&
+	test $master_entry_count = 5 &&
+
 
 	git reflog delete master@{1} &&
 	git reflog show master > output &&
-	test 4 = $(wc -l < output) &&
+	test $(($master_entry_count - 1)) = $(wc -l < output) &&
+	test $HEAD_entry_count = $(git reflog | wc -l) &&
 	! grep ox < output &&
 
+	master_entry_count=$(wc -l < output)
+
+	git reflog delete HEAD@{1} &&
+	test $(($HEAD_entry_count -1)) = $(git reflog | wc -l) &&
+	test $master_entry_count = $(git reflog show master | wc -l) &&
+
+	HEAD_entry_count=$(git reflog | wc -l)
+
 	git reflog delete master@{07.04.2005.15:15:00.-0700} &&
 	git reflog show master > output &&
-	test 3 = $(wc -l < output) &&
+	test $(($master_entry_count - 1)) = $(wc -l < output) &&
 	! grep dragon < output
 
 '
-- 
1.6.0.rc0.320.g49281

[PATCH 2/2] builtin-reflog: fix deletion of HEAD entries

[Pieter de Bie]

dwim_ref() used to resolve HEAD to its symlink (like refs/heads/master),
making a call to 'git reflog delete HEAD@{1}' to actually delete the second
entry in the master reflog.

This patch makes a special case for HEAD (as that's the only non-branch
reflog we keep), fixing the issue.

Signed-off-by: Pieter de Bie <pdebie@ai.rug.nl>
---
 builtin-reflog.c  |   15 ++++++++++++---
 t/t1410-reflog.sh |    2 +-
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/builtin-reflog.c b/builtin-reflog.c
index 0c34e37..5af3f28 100644
--- a/builtin-reflog.c
+++ b/builtin-reflog.c
@@ -604,9 +604,18 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
-		if (!dwim_ref(argv[i], spec - argv[i], sha1, &ref)) {
-			status |= error("%s points nowhere!", argv[i]);
-			continue;
+		if (!strncmp(argv[i], "HEAD", 4)) {
+			ref = xstrdup("HEAD");
+			if (!resolve_ref(ref, sha1, 1, NULL)) {
+				status |= error("%s points nowhere!", argv[i]);
+				continue;
+			}
+		}
+		else {
+			if (!dwim_ref(argv[i], spec - argv[i], sha1, &ref)) {
+				status |= error("%s points nowhere!", argv[i]);
+				continue;
+			}
 		}
 
 		recno = strtoul(spec + 2, &ep, 10);
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 3b9860e..5b24f05 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -175,7 +175,7 @@ test_expect_success 'recover and check' '
 
 '
 
-test_expect_failure 'delete' '
+test_expect_success 'delete' '
 	echo 1 > C &&
 	test_tick &&
 	git commit -m rat C &&
-- 
1.6.0.rc0.320.g49281

Re: [PATCH 2/2] builtin-reflog: fix deletion of HEAD entries

[Junio C Hamano]

Pieter de Bie <pdebie@ai.rug.nl> writes:

> dwim_ref() used to resolve HEAD to its symlink (like refs/heads/master),
> making a call to 'git reflog delete HEAD@{1}' to actually delete the second
> entry in the master reflog.
>
> This patch makes a special case for HEAD (as that's the only non-branch
> reflog we keep), fixing the issue.

What happens to remotes/origin/HEAD that points at remotes/origin/master?

Re: [PATCH 2/2] builtin-reflog: fix deletion of HEAD entries

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Pieter de Bie <pdebie@ai.rug.nl> writes:
>
>> dwim_ref() used to resolve HEAD to its symlink (like refs/heads/master),
>> making a call to 'git reflog delete HEAD@{1}' to actually delete the second
>> entry in the master reflog.
>>
>> This patch makes a special case for HEAD (as that's the only non-branch
>> reflog we keep), fixing the issue.
>
> What happens to remotes/origin/HEAD that points at remotes/origin/master?

Perhaps this might work better?

 builtin-reflog.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-reflog.c b/builtin-reflog.c
index 0c34e37..a48f664 100644
--- a/builtin-reflog.c
+++ b/builtin-reflog.c
@@ -604,7 +604,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
-		if (!dwim_ref(argv[i], spec - argv[i], sha1, &ref)) {
+		if (!dwim_log(argv[i], spec - argv[i], sha1, &ref)) {
 			status |= error("%s points nowhere!", argv[i]);
 			continue;
 		}

Re: [PATCH 2/2] builtin-reflog: fix deletion of HEAD entries

[Pieter de Bie]

On Aug 10, 2008, at 3:01 AM, Junio C Hamano wrote:
>-		if (!dwim_ref(argv[i], spec - argv[i], sha1, &ref)) {
>+		if (!dwim_log(argv[i], spec - argv[i], sha1, &ref)) {

This is also what add_reflog_for_walk() does, but that function tries to resolve
the argv[i] part first, without doing the dwim_log().

Perhaps we can also do this to allow "git reflog expire master" instead of
"git reflog expire refs/heads/master"? 

--<8--
Subject: [PATCH] builtin-reflog: Allow reflog expire to name partial ref

This allows you to specify 'git reflog expire master' without needing
to give the full refname like 'git reflog expire refs/heads/master'

Signed-off-by: Pieter de Bie <pdebie@ai.rug.nl>
---
 builtin-reflog.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin-reflog.c b/builtin-reflog.c
index 5af3f28..a8311a6 100644
--- a/builtin-reflog.c
+++ b/builtin-reflog.c
@@ -541,14 +541,15 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	}
 
 	while (i < argc) {
-		const char *ref = argv[i++];
+		char *ref;
 		unsigned char sha1[20];
-		if (!resolve_ref(ref, sha1, 1, NULL)) {
-			status |= error("%s points nowhere!", ref);
+		if (!dwim_log(argv[i], strlen(argv[i]), sha1, &ref)) {
+			status |= error("%s points nowhere!", argv[i]);
 			continue;
 		}
 		set_reflog_expiry_param(&cb, explicit_expiry, ref);
 		status |= expire_reflog(ref, sha1, 0, &cb);
+		i++;
 	}
 	return status;
 }
-- 
1.6.0.rc0.320.g49281

Re: [PATCH 2/2] builtin-reflog: fix deletion of HEAD entries

[Johannes Sixt]

On Sonntag, 10. August 2008, Pieter de Bie wrote:
> diff --git a/builtin-reflog.c b/builtin-reflog.c
> index 5af3f28..a8311a6 100644
> --- a/builtin-reflog.c
> +++ b/builtin-reflog.c
> @@ -541,14 +541,15 @@ static int cmd_reflog_expire(int argc, const char
> **argv, const char *prefix) }
>
>  	while (i < argc) {
> -		const char *ref = argv[i++];
> +		char *ref;
>  		unsigned char sha1[20];
> -		if (!resolve_ref(ref, sha1, 1, NULL)) {
> -			status |= error("%s points nowhere!", ref);
> +		if (!dwim_log(argv[i], strlen(argv[i]), sha1, &ref)) {
> +			status |= error("%s points nowhere!", argv[i]);
>  			continue;
>  		}
>  		set_reflog_expiry_param(&cb, explicit_expiry, ref);
>  		status |= expire_reflog(ref, sha1, 0, &cb);
> +		i++;
>  	}
>  	return status;
>  }

This runs into an endless loop in the error case because it doesn't increase 
i.

-- Hannes

Re: [PATCH 2/2] builtin-reflog: fix deletion of HEAD entries

[Junio C Hamano]

Pieter de Bie <pdebie@ai.rug.nl> writes:

> On Aug 10, 2008, at 3:01 AM, Junio C Hamano wrote:
>>-		if (!dwim_ref(argv[i], spec - argv[i], sha1, &ref)) {
>>+		if (!dwim_log(argv[i], spec - argv[i], sha1, &ref)) {
>
> This is also what add_reflog_for_walk() does, but that function tries to resolve
> the argv[i] part first, without doing the dwim_log().
>
> Perhaps we can also ...

Sorry, I do not understand what you meant by the above comment.

 - "This is also what add_reflog_for_walk() does" -- I take it you mean
   the use of dwim_log() instead of dwim_ref()?

 - "... but that function tries to resolve the argv[i] part first" -- do
   you mean the resolve_ref("HEAD"...) call inside "if (!*branch)"
   codepath?

   That one serves different purposes than "delete HEAD@{42}".  It is
   about showing "@{42}" --- in order to show reflog for "the current
   branch", it figures out the current branch by resolving "HEAD".

In any case, what confuses me is I cannot tell if you do or do not have
issues that I did not think of with the "s/dwim_ref/dwim_log/" change.
Are you saying "no that cannot be a correct fix; see the way dwim_log() is
used in add_reflog_for_walk() -- it does more than your one-liner"?

By the way, I think the idea of "Perhaps we can also..." part is good.

Re: [PATCH 2/2] builtin-reflog: fix deletion of HEAD entries

[Pieter de Bie]

On Aug 10, 2008, at 8:52 PM, Junio C Hamano wrote:

> Pieter de Bie <pdebie@ai.rug.nl> writes:
>
>> On Aug 10, 2008, at 3:01 AM, Junio C Hamano wrote:
>>> -		if (!dwim_ref(argv[i], spec - argv[i], sha1, &ref)) {
>>> +		if (!dwim_log(argv[i], spec - argv[i], sha1, &ref)) {
>>
>> This is also what add_reflog_for_walk() does, but that function  
>> tries to resolve
>> the argv[i] part first, without doing the dwim_log().
>>
>> Perhaps we can also ...
>
> Sorry, I do not understand what you meant by the above comment.

Sorry, it was early in the morning ;)

> - "This is also what add_reflog_for_walk() does" -- I take it you mean
>   the use of dwim_log() instead of dwim_ref()?

Yes

> - "... but that function tries to resolve the argv[i] part first" --  
> do
>   you mean the resolve_ref("HEAD"...) call inside "if (!*branch)"
>   codepath?
>
>   That one serves different purposes than "delete HEAD@{42}".  It is
>   about showing "@{42}" --- in order to show reflog for "the current
>   branch", it figures out the current branch by resolving "HEAD".

No, I meant this part:

reflogs = read_complete_reflog(branch);
if (!reflogs || reflogs->nr == 0)
	if (dwim_log(branch, strlen(branch), sha1, &b) == 1) {
		branch = b;
		reflogs = read_complete_reflog(branch);
	}

Which seems to suggest that the read_complete_reflog() may produce  
different results if dwim_log() is not called. However, I did not  
follow the codepath to see why.

> In any case, what confuses me is I cannot tell if you do or do not  
> have
> issues that I did not think of with the "s/dwim_ref/dwim_log/" change.
> Are you saying "no that cannot be a correct fix; see the way  
> dwim_log() is
> used in add_reflog_for_walk() -- it does more than your one-liner"?

I think the change looks ok. The only 'problem' I had was the chunk  
above, because I do not know if the double call to  
read_complete_reflog, once without a dwim_log and optionally once  
with, is significant. However, I'm not familiar enough with the code  
to make any observation other than that, which is why my reply had no  
conclusion ;)

Hope that clears things up.

> By the way, I think the idea of "Perhaps we can also..." part is good.

I'll send in a better patch.

- Pieter

[PATCH] builtin-reflog: Allow reflog expire to name partial ref

[Pieter de Bie]

This allows you to specify 'git reflog expire master' without needing
to give the full refname like 'git reflog expire refs/heads/master'

Signed-off-by: Pieter de Bie <pdebie@ai.rug.nl>
---
 builtin-reflog.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin-reflog.c b/builtin-reflog.c
index 5af3f28..f4d1f32 100644
--- a/builtin-reflog.c
+++ b/builtin-reflog.c
@@ -540,11 +540,11 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		free(collected.e);
 	}
 
-	while (i < argc) {
-		const char *ref = argv[i++];
+	for (; i < argc; i++) {
+		char *ref;
 		unsigned char sha1[20];
-		if (!resolve_ref(ref, sha1, 1, NULL)) {
-			status |= error("%s points nowhere!", ref);
+		if (!dwim_log(argv[i], strlen(argv[i]), sha1, &ref)) {
+			status |= error("%s points nowhere!", argv[i]);
 			continue;
 		}
 		set_reflog_expiry_param(&cb, explicit_expiry, ref);
-- 
1.6.0.rc0.320.g49281