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

[Michael J Gruber <git@drmicha.warpmail.net>]

Erick Mattos venit, vidit, dixit 26.05.2010 20:04:
> 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.
> 
>>>>> +     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.
> 
>>> 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.

I'm not asking you to explain your code but your intentions: What is it
supposed to do? If I have to read the code to figure that out then your
commit messages and on-list explanations (or my understanding thereof)
are suboptimal. You're cleaning up some files in logs/refs and I'm wondering

- which command does that automatically (after your series) and
- in which circumstances (only --orphan or more) superfluous files were
left there before.

If you're not willing to answer that I'm simply not reading the code.
One reviewer less.

Michael