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

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

Hi,

2010/5/26 Junio C Hamano [off-list ref]

quoted

Erick Mattos [off-list ref] writes:

quoted

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

quoted

quoted

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

Really long line here ;)

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?

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