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

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: May 26, 2010, 15:31 UTC
Message-ID: <4BFD3ED3.3000709@drmicha.warpmail.net>
In-Reply-To: <AANLkTimT3sI3yuM8RZai-eWDk8Z5Rmc28RLGOx_i-RXa@mail.gmail.com>

Erick Mattos venit, vidit, dixit 26.05.2010 16:52:

Show 20 quoted lines

> Hi,
> 
> 2010/5/26 Junio C Hamano <gitster@pobox.com>
>>
>> Erick Mattos <erick.mattos@gmail.com> writes:
>>> @@ -684,8 +709,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>>>       if (opts.new_orphan_branch) {
>>>               if (opts.new_branch)
>>>                       die("--orphan and -b are mutually exclusive");
>>> -             if (opts.track > 0 || opts.new_branch_log)
>>> -                     die("--orphan cannot be used with -t or -l");
>>> +             if (opts.track > 0)
>>> +                     die("--orphan should not be used with -t");
>>
>> Why s/cannot/should not/?  Just being curious.
> 
> I have typed that text, not changed the original so this is not a fix
> to your text.  Anyway for me "should not" is more polite, like "you
> should not yell" meaning you really can not do it.  Or "you should not
> disrespect the captain".

"should not" means you can but you should not. "die" certainly means you cannot. This is not a matter of politeness but of correctness.

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

> 
>>> +test_expect_success 'giving up --orphan not committed when -l and core.logAllRefUpdates = false deletes reflog' '

Really long line here ;)

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

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

Show 5 quoted lines

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