From: Michael J Gruber <git@drmicha•warpmail.net>
To: Erick Mattos <erick.mattos@gmail•com>
Cc: Junio C Hamano <gitster@pobox•com>, git@vger•kernel.org
Subject: Re: [PATCH 3/5] checkout --orphan: respect -l option always
Date: Thu, 27 May 2010 09:50:57 +0200 [thread overview]
Message-ID: <4BFE2461.5080501@drmicha.warpmail.net> (raw)
In-Reply-To: <AANLkTiksYeRzqNTdOMxb3oliuVna6kAxbHM8nxx6gNCO@mail.gmail.com>
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
next prev parent reply other threads:[~2010-05-27 7:51 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-05-22 0:28 [PATCH 0/5] checkout --orphan improvements Erick Mattos
2010-05-22 0:28 ` [PATCH 1/5] Documentation: alter checkout --orphan description Erick Mattos
2010-05-22 0:28 ` [PATCH 2/5] refs: split log_ref_write logic into log_ref_setup Erick Mattos
2010-05-26 5:07 ` Junio C Hamano
2010-05-26 18:11 ` Erick Mattos
2010-06-02 18:14 ` Junio C Hamano
2010-06-02 23:16 ` Erick Mattos
2010-05-22 0:28 ` [PATCH 3/5] checkout --orphan: respect -l option always Erick Mattos
2010-05-26 5:07 ` Junio C Hamano
2010-05-26 14:52 ` Erick Mattos
2010-05-26 15:13 ` Erik Faye-Lund
2010-05-26 16:01 ` Erick Mattos
2010-06-03 16:28 ` Erick Mattos
2010-05-26 15:31 ` Michael J Gruber
2010-05-26 18:04 ` Erick Mattos
2010-05-27 7:50 ` Michael J Gruber [this message]
2010-05-22 0:28 ` [PATCH 4/5] t3200: test -l with core.logAllRefUpdates options Erick Mattos
2010-05-22 0:43 ` [PATCH 5/5] bash completion: add --orphan to 'git checkout' Erick Mattos
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4BFE2461.5080501@drmicha.warpmail.net \
--to=git@drmicha$(echo .)warpmail.net \
--cc=erick.mattos@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox