git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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: Wed, 26 May 2010 17:31:31 +0200	[thread overview]
Message-ID: <4BFD3ED3.3000709@drmicha.warpmail.net> (raw)
In-Reply-To: <AANLkTimT3sI3yuM8RZai-eWDk8Z5Rmc28RLGOx_i-RXa@mail.gmail.com>

Erick Mattos venit, vidit, dixit 26.05.2010 16:52:
> 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 ;)

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

  parent reply	other threads:[~2010-05-26 15:32 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 [this message]
2010-05-26 18:04         ` Erick Mattos
2010-05-27  7:50           ` Michael J Gruber
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=4BFD3ED3.3000709@drmicha.warpmail.net \
    --to=git@drmicha.warpmail.net \
    --cc=erick.mattos@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).