Re: [PATCH 3/5] checkout --orphan: respect -l option always

From: Erick Mattos <erick.mattos@gmail.com>
Date: May 26, 2010, 18:04 UTC
Message-ID: <AANLkTiksYeRzqNTdOMxb3oliuVna6kAxbHM8nxx6gNCO@mail.gmail.com>
In-Reply-To: <4BFD3ED3.3000709@drmicha.warpmail.net>

Hi,

2010/5/26 Michael J Gruber <git@drmicha.warpmail.net>:

>> But that is not a fix.
>
> There's a "-" line with "cannot" and a "+" line with "should not". So
> you certainly changed what was there before.

Everybody know what a minus or a plus sign means in a diff. ;-)

What I have meant was that I had typed the whole line myself after some previous removal while I was making the changes during "deletion/moving lines" actions. No big deal, just a mistake.

The real message change here is from blocking -t an -l to blocking only -t. As I had told I have not realized the 'should not/cannot' issue.

Show 30 quoted lines

>>>> +     git checkout master &&
>>>> +     git checkout -l --orphan eta &&
>>>> +     test -f .git/logs/refs/heads/eta &&
>>>> +     test_must_fail PAGER= git reflog show eta &&
>>>> +     git checkout master &&
>>>> +     ! test -f .git/logs/refs/heads/eta &&
>>>> +     test_must_fail PAGER= git reflog show eta
>>>> +'
>>>
>>> I don't quite understand the title of this test, nor am I convinced that
>>> testing for .git/logs/refs/heads/eta is necessarily a good thing to do
>>> here.  "eta" branch is first prepared in an unborn state with the working
>>> tree and the index prepared to commit what is in 'master', and the first
>>> "git reflog" would fail because there is no eta branch at that point yet.
>>> Moving to 'master' from that state would still leave "eta" branch unborn
>>> and we will not see "git reflog" for that branch (we will fail "git log
>>> eta" too for that matter).  Perhaps two "test -f .git/logs/refs/heads/eta"
>>> shouldn't be there?  It feels that it is testing a bit too low level an
>>> implementation detail.
>>
>> So I need to explain the solution:
>>
>> When config core.logAllRefUpdates is set to false what really happens
>> is that the reflog is not created and any reflog change is saved only
>> when you have an existent reflog.
>>
>> What I did was to make a "touch reflog".  Creating it, when the new
>
> You mean checkout -l --orphan does that touch? There is none in the
> test. Does ordinary checkout with -l does that, too?

This is not done by a test. It is part of the whole implementation. It is done only when needed: on that special corner case.

Please read the patches mainly the 2/5 and 3/5.

Show 25 quoted lines

>> branch get eventually saved then the reflog would be written normally.
>>  But in case somebody give up this new branch before the first save,
>> moving back to a regular branch would leave a ghost reflog.
>
> The touched entry (is left), not a reflog, I assume, otherwise the
> reflog command should not fail.
>
>>
>> I have coded the cleaning commands for that and the test is just a
>> check of this behavior.
>
> Which command does the cleaning? "reflog show" or "checkout master"?
>
>>
>> The first "test -f .git/logs/refs/heads/eta" tests if reflog was
>> created and the second if it was deleted.  No big deal.
>>
>> Regards
>
> I haven't followed this series due to earlier worries about --orphan but
> I'm wondering about this cleaning up behind the back. Maybe it's just a
> matter of explanations, though.
>
> Michael
>

Your questions are too unaware of the code. ;-) As I don't think you are asking me to explain each single line then I imagine you have not read the patches, just the chat. Please read the patch series. I will be very glad to answer any further questions then.

Best regards