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

Erick Mattos venit, vidit, dixit 26.05.2010 20:04:

Hi,

2010/5/26 Michael J Gruber [off-list ref]:

quoted

quoted

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.

quoted

quoted

quoted

quoted

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

quoted

quoted

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.

quoted

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

quoted

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