From: Pieter de Bie <pdebie@ai.rug.nl>
Date: Sun, 10 Aug 2008 19:04:56 GMT
Subject: Re: [PATCH 2/2] builtin-reflog: fix deletion of HEAD entries
Message-ID: <9746FE5D-816C-4818-B32F-EE0028918F72@ai.rug.nl>
In-Reply-To: <7vej4w3dou.fsf@gitster.siamese.dyndns.org>


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