git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] tests: move test_cmp_rev to test-lib-functions
@ 2012-12-21 19:10 Martin von Zweigbergk
  2012-12-21 19:10 ` [PATCH 2/2] learn to pick/revert into unborn branch Martin von Zweigbergk
  0 siblings, 1 reply; 11+ messages in thread
From: Martin von Zweigbergk @ 2012-12-21 19:10 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Ramkumar Ramachandra, Martin von Zweigbergk

A function for checking that two given parameters refer to the same
revision was defined in several places, so move the definition to
test-lib-functions.sh instead.

Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
---
 t/t1505-rev-parse-last.sh           | 18 +++++-------------
 t/t3404-rebase-interactive.sh       |  6 ------
 t/t3507-cherry-pick-conflict.sh     |  6 ------
 t/t3508-cherry-pick-many-commits.sh |  8 ++------
 t/t3510-cherry-pick-sequence.sh     |  6 ------
 t/t6030-bisect-porcelain.sh         |  4 +---
 t/test-lib-functions.sh             |  7 +++++++
 7 files changed, 15 insertions(+), 40 deletions(-)

diff --git a/t/t1505-rev-parse-last.sh b/t/t1505-rev-parse-last.sh
index d709ecf..4969edb 100755
--- a/t/t1505-rev-parse-last.sh
+++ b/t/t1505-rev-parse-last.sh
@@ -32,32 +32,24 @@ test_expect_success 'setup' '
 #
 # and 'side' should be the last branch
 
-test_rev_equivalent () {
-
-	git rev-parse "$1" > expect &&
-	git rev-parse "$2" > output &&
-	test_cmp expect output
-
-}
-
 test_expect_success '@{-1} works' '
-	test_rev_equivalent side @{-1}
+	test_cmp_rev side @{-1}
 '
 
 test_expect_success '@{-1}~2 works' '
-	test_rev_equivalent side~2 @{-1}~2
+	test_cmp_rev side~2 @{-1}~2
 '
 
 test_expect_success '@{-1}^2 works' '
-	test_rev_equivalent side^2 @{-1}^2
+	test_cmp_rev side^2 @{-1}^2
 '
 
 test_expect_success '@{-1}@{1} works' '
-	test_rev_equivalent side@{1} @{-1}@{1}
+	test_cmp_rev side@{1} @{-1}@{1}
 '
 
 test_expect_success '@{-2} works' '
-	test_rev_equivalent master @{-2}
+	test_cmp_rev master @{-2}
 '
 
 test_expect_success '@{-3} fails' '
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 32fdc99..8462be1 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -29,12 +29,6 @@ Initial setup:
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
-test_cmp_rev () {
-	git rev-parse --verify "$1" >expect.rev &&
-	git rev-parse --verify "$2" >actual.rev &&
-	test_cmp expect.rev actual.rev
-}
-
 set_fake_editor
 
 # WARNING: Modifications to the initial repository can change the SHA ID used
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index c82f721..223b984 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -11,12 +11,6 @@ test_description='test cherry-pick and revert with conflicts
 
 . ./test-lib.sh
 
-test_cmp_rev () {
-	git rev-parse --verify "$1" >expect.rev &&
-	git rev-parse --verify "$2" >actual.rev &&
-	test_cmp expect.rev actual.rev
-}
-
 pristine_detach () {
 	git checkout -f "$1^0" &&
 	git read-tree -u --reset HEAD &&
diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh
index 340afc7..4e7136b 100755
--- a/t/t3508-cherry-pick-many-commits.sh
+++ b/t/t3508-cherry-pick-many-commits.sh
@@ -5,15 +5,11 @@ test_description='test cherry-picking many commits'
 . ./test-lib.sh
 
 check_head_differs_from() {
-	head=$(git rev-parse --verify HEAD) &&
-	arg=$(git rev-parse --verify "$1") &&
-	test "$head" != "$arg"
+	! test_cmp_rev HEAD "$1"
 }
 
 check_head_equals() {
-	head=$(git rev-parse --verify HEAD) &&
-	arg=$(git rev-parse --verify "$1") &&
-	test "$head" = "$arg"
+	test_cmp_rev HEAD "$1"
 }
 
 test_expect_success setup '
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index b5fb527..7b7a89d 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -24,12 +24,6 @@ pristine_detach () {
 	git clean -d -f -f -q -x
 }
 
-test_cmp_rev () {
-	git rev-parse --verify "$1" >expect.rev &&
-	git rev-parse --verify "$2" >actual.rev &&
-	test_cmp expect.rev actual.rev
-}
-
 test_expect_success setup '
 	git config advice.detachedhead false &&
 	echo unrelated >unrelated &&
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 72e28ee..3e0e15f 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -676,9 +676,7 @@ test_expect_success 'bisect fails if tree is broken on trial commit' '
 check_same()
 {
 	echo "Checking $1 is the same as $2" &&
-	git rev-parse "$1" > expected.same &&
-	git rev-parse "$2" > expected.actual &&
-	test_cmp expected.same expected.actual
+	test_cmp_rev "$1" "$2"
 }
 
 test_expect_success 'bisect: --no-checkout - start commit bad' '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 22a4f8f..fa62d01 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -602,6 +602,13 @@ test_cmp() {
 	$GIT_TEST_CMP "$@"
 }
 
+# Tests that its two parameters refer to the same revision
+test_cmp_rev () {
+	git rev-parse --verify "$1" >expect.rev &&
+	git rev-parse --verify "$2" >actual.rev &&
+	test_cmp expect.rev actual.rev
+}
+
 # Print a sequence of numbers or letters in increasing order.  This is
 # similar to GNU seq(1), but the latter might not be available
 # everywhere (and does not do letters).  It may be used like:
-- 
1.8.0.1.240.ge8a1f5a

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] learn to pick/revert into unborn branch
  2012-12-21 19:10 [PATCH 1/2] tests: move test_cmp_rev to test-lib-functions Martin von Zweigbergk
@ 2012-12-21 19:10 ` Martin von Zweigbergk
  2012-12-23  3:24   ` Junio C Hamano
  2012-12-23  4:02   ` Junio C Hamano
  0 siblings, 2 replies; 11+ messages in thread
From: Martin von Zweigbergk @ 2012-12-21 19:10 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Ramkumar Ramachandra, Martin von Zweigbergk

>From the user's point of view, it seems natural to think that
cherry-picking into an unborn branch should work, so make it work,
with or without --ff.

Cherry-picking anything other than a commit that only adds files, will
naturally result in conflicts. Similarly, revert also works, but will
result in conflicts unless the specified revision only deletes files.

Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>

---

The plan is to use this for fixing "git rebase --root" as discussed in
http://thread.gmane.org/gmane.comp.version-control.git/205796

Is there a better way of creating an unborn branch than what I do in
the test cases?

 sequencer.c                   | 19 +++++++++++--------
 t/t3501-revert-cherry-pick.sh |  9 +++++++++
 t/t3506-cherry-pick-ff.sh     |  8 ++++++++
 3 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 2260490..1ac1ceb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -186,14 +186,15 @@ static int error_dirty_index(struct replay_opts *opts)
 	return -1;
 }
 
-static int fast_forward_to(const unsigned char *to, const unsigned char *from)
+static int fast_forward_to(const unsigned char *to, const unsigned char *from,
+			   int unborn)
 {
 	struct ref_lock *ref_lock;
 
 	read_cache();
 	if (checkout_fast_forward(from, to, 1))
 		exit(1); /* the callee should have complained already */
-	ref_lock = lock_any_ref_for_update("HEAD", from, 0);
+	ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from, 0);
 	return write_ref_sha1(ref_lock, to, "cherry-pick");
 }
 
@@ -390,7 +391,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
 	char *defmsg = NULL;
 	struct strbuf msgbuf = STRBUF_INIT;
-	int res;
+	int res, unborn = 0;
 
 	if (opts->no_commit) {
 		/*
@@ -402,9 +403,10 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		if (write_cache_as_tree(head, 0, NULL))
 			die (_("Your index file is unmerged."));
 	} else {
-		if (get_sha1("HEAD", head))
-			return error(_("You do not have a valid HEAD"));
-		if (index_differs_from("HEAD", 0))
+		unborn = get_sha1("HEAD", head);
+		if (unborn)
+			hashcpy(head, EMPTY_TREE_SHA1_BIN);
+		if (index_differs_from(unborn ? EMPTY_TREE_SHA1_HEX : "HEAD", 0))
 			return error_dirty_index(opts);
 	}
 	discard_cache();
@@ -435,8 +437,9 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	else
 		parent = commit->parents->item;
 
-	if (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))
-		return fast_forward_to(commit->object.sha1, head);
+	if (opts->allow_ff &&
+	    (parent && !hashcmp(parent->object.sha1, head) || !parent && unborn))
+	     return fast_forward_to(commit->object.sha1, head, unborn);
 
 	if (parent && parse_commit(parent) < 0)
 		/* TRANSLATORS: The first %s will be "revert" or
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 34c86e5..6f489e2 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -100,4 +100,13 @@ test_expect_success 'revert forbidden on dirty working tree' '
 
 '
 
+test_expect_success 'chery-pick on unborn branch' '
+	git checkout --orphan unborn &&
+	git rm --cached -r . &&
+	rm -rf * &&
+	git cherry-pick initial &&
+	git diff --quiet initial &&
+	! test_cmp_rev initial HEAD
+'
+
 test_done
diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
index 51ca391..373aad6 100755
--- a/t/t3506-cherry-pick-ff.sh
+++ b/t/t3506-cherry-pick-ff.sh
@@ -105,4 +105,12 @@ test_expect_success 'cherry pick a root commit with --ff' '
 	test "$(git rev-parse --verify HEAD)" = "1df192cd8bc58a2b275d842cede4d221ad9000d1"
 '
 
+test_expect_success 'chery-pick --ff on unborn branch' '
+	git checkout --orphan unborn &&
+	git rm --cached -r . &&
+	rm -rf * &&
+	git cherry-pick --ff first &&
+	test_cmp_rev first HEAD
+'
+
 test_done
-- 
1.8.0.1.240.ge8a1f5a

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] learn to pick/revert into unborn branch
  2012-12-21 19:10 ` [PATCH 2/2] learn to pick/revert into unborn branch Martin von Zweigbergk
@ 2012-12-23  3:24   ` Junio C Hamano
  2012-12-23  6:24     ` Martin von Zweigbergk
  2012-12-23  4:02   ` Junio C Hamano
  1 sibling, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2012-12-23  3:24 UTC (permalink / raw)
  To: Martin von Zweigbergk; +Cc: git, Ramkumar Ramachandra

Martin von Zweigbergk <martinvonz@gmail.com> writes:

>>From the user's point of view, it seems natural to think that
> cherry-picking into an unborn branch should work, so make it work,
> with or without --ff.

I actually am having a hard time imagining how that could ever be
natural.

When you are on an unborn branch, you may have some files in your
working tree, and some of them may even be registered to the index,
but the index is merely for your convenience to create your first
commit, and as far as the history is concered, it does not matter.

By definition you do not have any history in such a state.  What
does it even mean to "cherry-pick" another commit, especially
without the --no-commit option?  The resulting commit will carry the
message taken from the original commit, but does what it says match
what you have done?

I can understand that it may sometimes make sense to do

  $ git show --diff-filter=A $that_commit | git apply

as a way to further update the uncommitted state you have in the
working tree, so I can sort of buy that --no-commit case might make
some sense (but if you make a commit after "cherry-pick --no-commit",
you still get the log message from that commit, which does not
explain the other things you have in your working tree) in a limited
situation.

It seems to me that the only case that may make sense is to grab the
contents from an existing tree, which might be better served with

  $ git checkout $that_commit -- $these_paths_I_am_interested_in

> Cherry-picking anything other than a commit that only adds files, will
> naturally result in conflicts. Similarly, revert also works, but will
> result in conflicts unless the specified revision only deletes files.

You may be able to make it "work" for some definition of "work", but
I am not sure how useful it is.

Puzzled...

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] learn to pick/revert into unborn branch
  2012-12-21 19:10 ` [PATCH 2/2] learn to pick/revert into unborn branch Martin von Zweigbergk
  2012-12-23  3:24   ` Junio C Hamano
@ 2012-12-23  4:02   ` Junio C Hamano
  1 sibling, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2012-12-23  4:02 UTC (permalink / raw)
  To: Martin von Zweigbergk; +Cc: git, Ramkumar Ramachandra

Martin von Zweigbergk <martinvonz@gmail.com> writes:

> @@ -435,8 +437,9 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>  	else
>  		parent = commit->parents->item;
>  
> -	if (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))
> -		return fast_forward_to(commit->object.sha1, head);
> +	if (opts->allow_ff &&
> +	    (parent && !hashcmp(parent->object.sha1, head) || !parent && unborn))

Style (from GNU); please avoid (A && B || C) and spell the
precedence between && and || explicitly, i.e.

	((A && B) || C)

> +	git rm --cached -r . &&
> +	rm -rf * &&

Not "git rm -rf ." and as two separate steps?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] learn to pick/revert into unborn branch
  2012-12-23  3:24   ` Junio C Hamano
@ 2012-12-23  6:24     ` Martin von Zweigbergk
  2012-12-23  7:01       ` Christian Couder
  2012-12-23 19:18       ` Junio C Hamano
  0 siblings, 2 replies; 11+ messages in thread
From: Martin von Zweigbergk @ 2012-12-23  6:24 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Ramkumar Ramachandra

On Sat, Dec 22, 2012 at 7:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Martin von Zweigbergk <martinvonz@gmail.com> writes:
>
>>>From the user's point of view, it seems natural to think that
>> cherry-picking into an unborn branch should work, so make it work,
>> with or without --ff.
>
> I actually am having a hard time imagining how that could ever be
> natural.

Fair enough. What's natural is of course very subjective. In my
opinion, whenever possible, operations on an unborn branch should
behave exactly as they would on an arbitrary commit whose tree just
happens to be empty. Of course, pretty much any operation that needs
more than the tree (indirectly) pointed to by HEAD would fail the
"whenever possible" clause. I realize that cherry-pick _does_ need the
current commit to record the parent of the resulting commit, but that
almost seems like an implementation detail, i.e whether we're adding a
parent or adding no parent (when on unborn branch) to the list of
parents.

In the same way, I think "git reset" should work on an unborn branch,
even though there is no commit that we can be "modifying index and
working tree to match" (from man page). I think many users, like me,
think of unborn branches as having an empty tree, rather than being a
special state before history is created. Sure, such thinking is not
technically correct, but it still seems to be some people's intuition
(including mine).

>> Cherry-picking anything other than a commit that only adds files, will
>> naturally result in conflicts. Similarly, revert also works, but will
>> result in conflicts unless the specified revision only deletes files.
>
> You may be able to make it "work" for some definition of "work", but
> I am not sure how useful it is.

As for use cases, I didn't consider that much more than that it might
be useful for implementing "git rebase --root". I haven't implemented
that yet, so I can't say for sure that it will work out.

One use case might be to rewrite history by creating an new unborn
branch and picking the initial commit and a subset of other commits.
Anyway, I didn't implement it because I thought it would be very
useful, but mostly because I just thought it should work (for
completeness).

I could resend as part of my rebase series (called mz/rebase-range at
some point) once that's done. Then we can discuss another solution in
the scope of that series if we don't agree on allowing on cherry-pick
on an unborn branch.

Btw, I have another series, which I'll send after 1.8.1, that teaches
"git reset" to work on an unborn branch (among other things). We might
want to decide to support both or neither of the commands on an unborn
branch.

On Sat, Dec 22, 2012 at 7:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Martin von Zweigbergk <martinvonz@gmail.com> writes:
>
>>>From the user's point of view, it seems natural to think that
>> cherry-picking into an unborn branch should work, so make it work,
>> with or without --ff.
>
> I actually am having a hard time imagining how that could ever be
> natural.
>
> When you are on an unborn branch, you may have some files in your
> working tree, and some of them may even be registered to the index,
> but the index is merely for your convenience to create your first
> commit, and as far as the history is concered, it does not matter.
>
> By definition you do not have any history in such a state.  What
> does it even mean to "cherry-pick" another commit, especially
> without the --no-commit option?  The resulting commit will carry the
> message taken from the original commit, but does what it says match
> what you have done?
>
> I can understand that it may sometimes make sense to do
>
>   $ git show --diff-filter=A $that_commit | git apply
>
> as a way to further update the uncommitted state you have in the
> working tree, so I can sort of buy that --no-commit case might make
> some sense (but if you make a commit after "cherry-pick --no-commit",
> you still get the log message from that commit, which does not
> explain the other things you have in your working tree) in a limited
> situation.
>
> It seems to me that the only case that may make sense is to grab the
> contents from an existing tree, which might be better served with
>
>   $ git checkout $that_commit -- $these_paths_I_am_interested_in
>
>> Cherry-picking anything other than a commit that only adds files, will
>> naturally result in conflicts. Similarly, revert also works, but will
>> result in conflicts unless the specified revision only deletes files.
>
> You may be able to make it "work" for some definition of "work", but
> I am not sure how useful it is.
>
> Puzzled...
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] learn to pick/revert into unborn branch
  2012-12-23  6:24     ` Martin von Zweigbergk
@ 2012-12-23  7:01       ` Christian Couder
  2012-12-23 19:20         ` Junio C Hamano
  2012-12-23 19:18       ` Junio C Hamano
  1 sibling, 1 reply; 11+ messages in thread
From: Christian Couder @ 2012-12-23  7:01 UTC (permalink / raw)
  To: Martin von Zweigbergk; +Cc: Junio C Hamano, git, Ramkumar Ramachandra

On Sun, Dec 23, 2012 at 7:24 AM, Martin von Zweigbergk
<martinvonz@gmail.com> wrote:
>
> As for use cases, I didn't consider that much more than that it might
> be useful for implementing "git rebase --root". I haven't implemented
> that yet, so I can't say for sure that it will work out.
>
> One use case might be to rewrite history by creating an new unborn
> branch and picking the initial commit and a subset of other commits.
> Anyway, I didn't implement it because I thought it would be very
> useful, but mostly because I just thought it should work (for
> completeness).

I agree that it would be nice if it worked.

Thanks,
Christian.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] learn to pick/revert into unborn branch
  2012-12-23  6:24     ` Martin von Zweigbergk
  2012-12-23  7:01       ` Christian Couder
@ 2012-12-23 19:18       ` Junio C Hamano
  2012-12-23 19:35         ` Junio C Hamano
  2012-12-24  7:20         ` Martin von Zweigbergk
  1 sibling, 2 replies; 11+ messages in thread
From: Junio C Hamano @ 2012-12-23 19:18 UTC (permalink / raw)
  To: Martin von Zweigbergk; +Cc: git, Ramkumar Ramachandra

Martin von Zweigbergk <martinvonz@gmail.com> writes:

> On Sat, Dec 22, 2012 at 7:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Martin von Zweigbergk <martinvonz@gmail.com> writes:
>>
>>>>From the user's point of view, it seems natural to think that
>>> cherry-picking into an unborn branch should work, so make it work,
>>> with or without --ff.
>>
>> I actually am having a hard time imagining how that could ever be
>> natural.
>
> Fair enough. What's natural is of course very subjective.  ...
> happens to be empty. Of course, pretty much any operation that needs
> more than the tree (indirectly) pointed to by HEAD would fail the
> "whenever possible" clause. I realize that cherry-pick _does_ need the
> current commit to record the parent of the resulting commit,...

Yes, and I do not think it is an implementation detail.

I am not opposed to an "internal use" of the cherry-pick machinery to
implement a corner case of "rebase -i":

    1. Your first commit adds "Makefile" and "hello.c", to build the
       "Hello world" program.

    2. Your second commmit adds "goodbye.c" and modifies "Makefile",
       to add the "Goodbye world" program.

    3. You run "rebase -i --root" to get this insn sheet:

	pick Add Makefile and hello.c for "Hello world"
        pick Add goodbye.c for "Goodbye world"

       and swap them:

        pick Add goodbye.c for "Goodbye world"
	pick Add Makefile and hello.c for "Hello world"

    4. The first one conflicts, as it wants to add new bits in
       "Makefile" that does not exist.  You edit it and make the
       result pretend as if "goodbye.c were the first program you
       added to the project (i.e. adding the common build
       infrastructure bits you did not change from the real first
       commit back to "Makefile", but making sure it does not yet
       mention "hello.c").

    5. "rebase --continue" will give you conflicts for the second
       one too, and your resolution is likely to match the tip
       before you started the whole "rebase -i".

In step 4., you would be internally using the cherry-pick machinery
to implement the step of "rebase -i" sequence.  That is what I would
call an implementation detail.  And that is cherry-picking to the
root.  It transplants something that used to depend on the entire
history behind it to be the beginning of the history so its log
needs to be adjusted, but "rebase -i" can choose to always make it
conflict and force the user to write a correct log message, so it
won't expose the fundamental flaw you would add if you allowed the
end-user facing "cherry-pick" to pick something to create a new root
commit without interaction.

> In the same way, I think "git reset" should work on an unborn branch,
> even though there is no commit that we can be "modifying index and
> working tree to match" (from man page).

I agree that "git reset" without any commit parameter to reset the
index and optionally the working tree (with "--hard") should reset
from an empty tree when you do not yet have any commit.  If HEAD
points at an existing commit, its tree is what you reset the
contents from.  If you do not have any commit yet, by definition
that tree is an empty tree.

But I do not think it has anything to do with "cherry-pick to empty",
so I do not agree with "In the same way" at all.

> As for use cases, I didn't consider that much more than that it might
> be useful for implementing "git rebase --root". I haven't implemented
> that yet, so I can't say for sure that it will work out.

I think it makes sense only as an internal implementation detail of
"rebase -i --root".

> One use case might be to rewrite history by creating an new unborn
> branch and picking the initial commit and a subset of other commits.

If you mean, in the above sample history, to "git cherry-pick" the
commit that starts the "Hello world" and then do something else on
top of the resulting history, how would that be different from
forking from that existing root commit?

> Anyway, I didn't implement it because I thought it would be very
> useful, but mostly because I just thought it should work (for
> completeness).

I would not exactly call X "complete" if X works in one way in most
cases and it works in quite a different way in one other case, only
because it would have to barf if it wanted to work in the same way
as in most cases, and the different behaviour is chosen only because
"X that does something is better than X that stops in an impossible
situation and barfs".

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] learn to pick/revert into unborn branch
  2012-12-23  7:01       ` Christian Couder
@ 2012-12-23 19:20         ` Junio C Hamano
  2012-12-23 20:27           ` Philip Oakley
  0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2012-12-23 19:20 UTC (permalink / raw)
  To: Christian Couder; +Cc: Martin von Zweigbergk, git, Ramkumar Ramachandra

Christian Couder <christian.couder@gmail.com> writes:

> I agree that it would be nice if it worked.

That is not saying anything.

Yes, it would be nice if everything worked.  But the question in the
thread is "with what definition of 'work'?"

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] learn to pick/revert into unborn branch
  2012-12-23 19:18       ` Junio C Hamano
@ 2012-12-23 19:35         ` Junio C Hamano
  2012-12-24  7:20         ` Martin von Zweigbergk
  1 sibling, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2012-12-23 19:35 UTC (permalink / raw)
  To: Martin von Zweigbergk; +Cc: git, Ramkumar Ramachandra

Junio C Hamano <gitster@pobox.com> writes:

> Yes, and I do not think it is an implementation detail.
>
> I am not opposed to an "internal use" of the cherry-pick machinery to
> implement a corner case of "rebase -i":
> ...
> In step 4., you would be internally using the cherry-pick machinery
> to implement the step of "rebase -i" sequence.  That is what I would
> call an implementation detail.  And that is cherry-picking to the
> root.  It transplants something that used to depend on the entire
> history behind it ...

Just to add another example, I do not think I would be opposed to
the case where you "edit" the root commit in the above example,
i.e. keeping the "Hello world" as the root commit, but modifying its
tree and/or log message. The internal impemenation detail has to
first chery-pick that existing commit on top of a void state before
it gives the user a chance to tweak the tree and commit the result
with a modified log message.  Just like "commit --amend" can be used
to amend the root commit, it logically makes sense the recreated
commit records nothing as its parent if done when HEAD is not valid
yet.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] learn to pick/revert into unborn branch
  2012-12-23 19:20         ` Junio C Hamano
@ 2012-12-23 20:27           ` Philip Oakley
  0 siblings, 0 replies; 11+ messages in thread
From: Philip Oakley @ 2012-12-23 20:27 UTC (permalink / raw)
  To: Junio C Hamano, Christian Couder
  Cc: Martin von Zweigbergk, Git List, Ramkumar Ramachandra

From: "Junio C Hamano" <gitster@pobox.com> Sent: Sunday, December 23,
2012 3:24 AM
Subject: Re: [PATCH 2/2] learn to pick/revert into unborn branch
> Martin von Zweigbergk <martinvonz@gmail.com> writes:
>
>>>From the user's point of view, it seems natural to think that
>> cherry-picking into an unborn branch should work, so make it work,
>> with or without --ff.
>
> I actually am having a hard time imagining how that could ever be
> natural.
>
> When you are on an unborn branch, you may have some files in your
> working tree, and some of them may even be registered to the index,
> but the index is merely for your convenience to create your first
> commit, and as far as the history is concered, it does not matter.
>
> By definition you do not have any history in such a state.  What
> does it even mean to "cherry-pick" another commit, especially
> without the --no-commit option?  The resulting commit will carry the
> message taken from the original commit, but does what it says match
> what you have done?


From: "Junio C Hamano"  Sent: Sunday, December 23, 2012 7:20 PM
Subject: Re: [PATCH 2/2] learn to pick/revert into unborn branch
> Christian Couder <christian.couder@gmail.com> writes:
>
>> I agree that it would be nice if it worked.
>
> That is not saying anything.
>
> Yes, it would be nice if everything worked.  But the question in the
> thread is "with what definition of 'work'?"
> --

>From the dumb user perspective, I would have thought that the first
commit to be cherry picked for an unborn branch would be the complete
commit, which is then planted as the branch's start commit. We tend to
talk of cherry picking commits, though the documentation does say 'the
changes introduced', which allows such a (mistaken) user perspective for
this particular case.

It is only in retrospect, and a bit of extra thought, that one could see
that the commit's message would not actually describe the new situation
and should have been edited.

That doesn't mean that it would be right to allow such an initilisation
of an unborn branch, it's more an explanation of how the idea may have
developed.

Philip

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] learn to pick/revert into unborn branch
  2012-12-23 19:18       ` Junio C Hamano
  2012-12-23 19:35         ` Junio C Hamano
@ 2012-12-24  7:20         ` Martin von Zweigbergk
  1 sibling, 0 replies; 11+ messages in thread
From: Martin von Zweigbergk @ 2012-12-24  7:20 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Ramkumar Ramachandra

On Sun, Dec 23, 2012 at 11:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Martin von Zweigbergk <martinvonz@gmail.com> writes:
>> On Sat, Dec 22, 2012 at 7:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I am not opposed to an "internal use" of the cherry-pick machinery to
> implement a corner case of "rebase -i":
>....
>     3. You run "rebase -i --root" to get this insn sheet:
>
>         pick Add Makefile and hello.c for "Hello world"
>         pick Add goodbye.c for "Goodbye world"
>
>        and swap them:
>
>         pick Add goodbye.c for "Goodbye world"
>         pick Add Makefile and hello.c for "Hello world"

Right, and as you point out in your later message, editing the initial
commit is another (and more useful) use case. It could of course be
special cased in "git rebase", but I think doing it "git cherry-pick"
is the right thing to do. Hopefully git-rebase.sh can then just reset
to the void state and git-rebase--interactive.sh can just continue
without knowing or caring whether it got started on an unborn branch.
Hmm... I just realized the "branch" in "unborn branch" really means we
don't have "unborn detached HEAD", do we? So some more tricks are
probably necessary after all. :-(

> [...] It transplants something that used to depend on the entire
> history behind it to be the beginning of the history so its log
> needs to be adjusted, but "rebase -i" can choose to always make it
> conflict and force the user to write a correct log message, so it
> won't expose the fundamental flaw you would add if you allowed the
> end-user facing "cherry-pick" to pick something to create a new root
> commit without interaction.

If I understand you correctly, you are suggesting that "git rebase"
should set the action from "pick" to "edit" for the first commit in
the insn sheet if it is not a root commit. "git rebase -i --root"
doesn't currently do that, but it certainly could.

> I agree that "git reset" without any commit parameter to reset the
> index and optionally the working tree (with "--hard") should reset
> from an empty tree when you do not yet have any commit.

Good to hear.

> But I do not think it has anything to do with "cherry-pick to empty",
> so I do not agree with "In the same way" at all.

See later comment.

>> One use case might be to rewrite history by creating an new unborn
>> branch and picking the initial commit and a subset of other commits.
>
> If you mean, in the above sample history, to "git cherry-pick" the
> commit that starts the "Hello world" and then do something else on
> top of the resulting history, how would that be different from
> forking from that existing root commit?

True, the result would be the same. The user's thought process might
be a little different ("let me start from scratch" vs "let me start
almost from scratch"), but that's a very minor difference that I'm
sure any user would quickly overcome.

>> Anyway, I didn't implement it because I thought it would be very
>> useful, but mostly because I just thought it should work (for
>> completeness).
>
> I would not exactly call X "complete" if X works in one way in most
> cases and it works in quite a different way in one other case, only
> because it would have to barf if it wanted to work in the same way
> as in most cases, and the different behaviour is chosen only because
> "X that does something is better than X that stops in an impossible
> situation and barfs".

I agree, of course, but I don't see the behavior as different. When
thinking about behavior around the root of the history, I imagine that
all root commits actually have a parent, and that they all have the
same parent. I also imagine that on an unborn branch, instead of being
invalid, HEAD points to that same "single root" commit with an empty
tree. Despite this model not matching git's, I find that this helps me
reason about what the behavior of various commands should be.

With this reasoning, cherry-picking into an unborn branch is no
different from cherry-picking into any commit with an empty tree
(which of course would be rare, but not forbidden).

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2012-12-24  7:20 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-12-21 19:10 [PATCH 1/2] tests: move test_cmp_rev to test-lib-functions Martin von Zweigbergk
2012-12-21 19:10 ` [PATCH 2/2] learn to pick/revert into unborn branch Martin von Zweigbergk
2012-12-23  3:24   ` Junio C Hamano
2012-12-23  6:24     ` Martin von Zweigbergk
2012-12-23  7:01       ` Christian Couder
2012-12-23 19:20         ` Junio C Hamano
2012-12-23 20:27           ` Philip Oakley
2012-12-23 19:18       ` Junio C Hamano
2012-12-23 19:35         ` Junio C Hamano
2012-12-24  7:20         ` Martin von Zweigbergk
2012-12-23  4:02   ` Junio C Hamano

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