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
next prev 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).