Re: [PATCH] git checkout: create unparented branch by --orphan

[Erick Mattos <erick.mattos@gmail.com>]

Hi,

2010/3/20 Junio C Hamano <gitster@pobox.com>:
> Erick Mattos <erick.mattos@gmail.com> writes:
>
>> Complete rewrite of --orphan functionality to fit it in the new design goals
>> set by Junio.  Now --orphan is not anymore an option of -b.  It is an
>> alternative to it.
>
> You are giving me too much credit.  I just made a suggestion, you had the
> choice to agree to or disagree with it, and I am hoping that you made the
> final decision to rewrite the patch to match what I suggested not because
> you wanted a "commit count" in the project with any form of a change, but
> because you thought the suggested approach made much more sense than the
> earlier iterations.  And in that case, the final decision to submit the
> patch in this form was due to _you_.  Don't give me undue credit---I was
> just helping from the sideline.

If I was going to look for a commit count I would do spelling/grammar
corrections. :-)

This is the second feature I am trying to add to git.  Both changes of
mine are features I use my own.  On my compiled and customized version
of git.  I thought they were good and tried to share following the
best free software spirit (I have been always feeling guilty of just
using free software).  I am not a new developer trying to be known.  I
am not even "new"...  :-)

Anyway I think you will not hear of me anymore after we finish this
work.  I think I did enough for this project and I really don't feel
myself fitted to the task.

I realized your last critic was indeed good and that it worthed the changes.

But I have to confess that I am a little impatient to end this
development cycle though.  So trying not to argue was a change of
behavior. ;-)

>> Junio:  I ask you to reconsider only the giving of the "short-and-sweet" -o
>> from beginning because of the new design.
>
> No.  I don't want our hand to be tied with something that is clearly a
> corner case feature from the beginning.  It wouldn't be too late to add
> one later, after it proves something people do every day (or even every
> hour).

If it is because of its importance I think it will never deserve the upgrade.

But I was arguing because I imagine checkout will never need the 21
remaining letters and that it was good for cosmetic and similarity (to
his brother -b) reasons.

Also because I am more used to one letter options from previous
operational systems experience (let's forget that! :-) ).

>>  Documentation/git-checkout.txt |   21 +++++++++++++++-
>>  builtin/checkout.c             |   19 +++++++++++++-
>>  t/t2017-checkout-orphan.sh     |   53 ++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 90 insertions(+), 3 deletions(-)
>>  create mode 100755 t/t2017-checkout-orphan.sh
>>
>> diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
>> index 37c1810..18df834 100644
>> --- a/Documentation/git-checkout.txt
>> +++ b/Documentation/git-checkout.txt
>> @@ -20,6 +20,19 @@ When <paths> are not given, this command switches branches by
>>  updating the index, working tree, and HEAD to reflect the specified
>>  branch.
>>
>> +When you use "--orphan", a new unparented branch is created having the
>> +index and the working tree intact.  This allows you to start a new
>> +history that records set of paths similar to that of the start-point
>> +commit, which is useful when you want to keep different branches for
>> +different audiences you are working to like when you have an open source
>> +and commercial versions of a software, for example.
>> +
>> +If you want to start a disconnected history that records set of paths
>> +totally different from the original branch, you may want to first clear
>> +the index and the working tree, by running "git rm -rf ." from the
>> +top-level of the working tree, before preparing your files (by copying
>> +from elsewhere, extracting a tarball, etc.) in the working tree.
>> +
>>  If `-b` is given, a new branch is created and checked out, as if
>>  linkgit:git-branch[1] were called; in this case you can
>>  use the --track or --no-track options, which will be passed to `git
>
> It feels wrong to talk about --orphan before readers learned -b.  Does the
> text flow nicely if we just swap the paragraphs?  Better yet, perhaps we
> shouldn't have any of this in the general description section.  Move this
> and consolidate it with the text in "OPTIONS" section under "--orphan"?

As you wish.

>> @@ -63,6 +76,12 @@ entries; instead, unmerged entries are ignored.
>>       When checking out paths from the index, check out stage #2
>>       ('ours') or #3 ('theirs') for unmerged paths.
>>
>> +--orphan::
>> +     Create a new branch named <new_branch>, unparented to any other
>> +     branch.  The new branch you switch to does not have any commit
>> +     and after the first one it will become the root of a new history
>> +     completely unconnected from all the other branches.
>> +
>>  -b::
>>       Create a new branch named <new_branch> and start it at
>>       <start_point>; see linkgit:git-branch[1] for details.
>
> Likewise.  I think "--orphan" should come after -b/-t/-l (all about the
> normal cases of switching to a different branch either new or existing).

All right.

> Oh, by the way, how does this new option work with -t?  I think the
> combination should be rejected, no?

Yes it should.  And it is.  By --track itself which is directly
connected to -b.  Test done just before --orphan test by someone else.

> I have a suspicion that you are trying "git co-o new old" case where old
> is not the current branch (so that you would want to adjust the index and
> the working tree files to that of "old" while carrying local changes
> forward), _but_ if that is the case that should have been already done
> with the logic in switch_branches() where it calls merge_working_tree(),
> and not here in the call chain.

My fault.  Forget that.  Fast unverified work in a hurry. :-\

>> @@ -677,6 +686,12 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>>               opts.new_branch = argv0 + 1;
>>       }
>>
>> +     if (opts.new_orphan_branch) {
>> +             if (opts.new_branch)
>> +                     die("--orphan and -b are mutually exclusive");
>> +             opts.new_branch = opts.new_orphan_branch;
>> +     }
>
> How should this interact with opts.track?  I think (and please correct
> me if you disagree):
>
>  - "git checkout -t --orphan new [old]" should fail; new is rootless and
>   does not track either the current branch or "old";
>
>  - opts.track that came from git_branch_track (default read from config)
>   should not cause a failure, but be silently ignored.

It is just working like you have pictured.

>> diff --git a/t/t2017-checkout-orphan.sh b/t/t2017-checkout-orphan.sh
>> new file mode 100755
>> index 0000000..e6d88b1
>> --- /dev/null
>> +++ b/t/t2017-checkout-orphan.sh
>> @@ -0,0 +1,53 @@
>> +#!/bin/sh
>> +#
>> +# Copyright (c) 2010 Erick Mattos
>> +#
>> +
>> +test_description='git checkout --orphan
>> +
>> +Main Tests for --orphan functionality.'
>> +
>> +. ./test-lib.sh
>> +
>> +TEST_FILE=foo
>> +
>> +test_expect_success 'Setup' '
>> +     echo "Initial" >"$TEST_FILE" &&
>> +     git add "$TEST_FILE" &&
>> +     git commit -m "First Commit"
>> +     test_tick &&
>> +     echo "State 1" >>"$TEST_FILE" &&
>> +     git add "$TEST_FILE" &&
>> +     git commit -m "Second Commit"
>
> No tick before this one?

I will add another then.

>> +'
>> +
>> +test_expect_success '--orphan creates a new orphan branch from HEAD' '
>> +     git checkout --orphan alpha &&
>> +     test_must_fail PAGER= git log >/dev/null 2>/dev/null &&
>> +     test "alpha" = "$(git symbolic-ref HEAD | sed "s,.*/,,")" &&
>
> What is this PAGER= doing here?
>
> I think it is more direct to check that:
>
>  - "rev-parse --verify HEAD" fails; and
>  - "symbolic-ref HEAD" says refs/heads/alpha.
>> +     test_tick &&
>> +     git commit -m "Third Commit" &&
>
> Tricky but correct ;-)

Indeed.  Easier your way though.

>> +     test 0 -eq $(git cat-file -p HEAD | grep "^parent" | wc -l) &&
>
>    test_must_fail git rev-parse --verify HEAD^
>
>> +     git cat-file -p master | grep "^tree" >base &&
>
>    git rev-parse master^{tree} >base
>
>> +     git cat-file -p HEAD | grep "^tree" >actual &&
>> +     test_cmp base actual
>> +'
>
> Or you can just replace the above three lines with:
>
>        git diff-tree --quiet master alpha
>
>> +test_expect_success '--orphan creates a new orphan branch from <start_point>' '
>> ...
>> +'
>
> Similar.
>
>> +test_expect_success '--orphan must be rejected with -b' '
>> +     test_must_fail git checkout --orphan new -b newer
>> +'

All your suggestions are going to be added.

> Ok.  You have the basics covered well; there are a few more things to
> test, I think.
>
> With local changes in the index/working tree without "start commit" (which
> should never fail) and with "start commit" (which should fail if HEAD and
> start commit has differences to the same paths as you have local changes
> to).

It is behaving like that already and that is intrinsically a
switch_branches() logic, not a special --orphan need.  It is not part
of this implementation and It is probably tested elsewhere (you
probably do know where).

> Also you would want to check with -t, --no-t, branch.autosetupmrebe set to
> always in the configuration with -t and without -t from the command line,

The actual implementation is just ignoring track and -t is not even
allowed.  So it is pointless.

> Thanks.
>

I will be sending a new patch version soon.

Regards